Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

On 10/14/21 12:34 PM, Russell King (Oracle) wrote:

On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote:

quoted

Hello Sean,

Quoting Sean Anderson (2021-10-11 18:55:16)

quoted

As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.

I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.

Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).

Some of this could be improved if we add the ability for a MAC to
specify the phy_interface_t modes that it supports as a bitmap
before calling phylink_create() - then we can have phylink check
that the mode is supported itself prior to calling the validate
handler.

You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue

and we could add to phylink_validate():

	if (!phy_interface_empty(pl->config->supported_interfaces) &&
	    !test_bit(state->interface, pl->config->supported_interfaces))
		return -EINVAL;

which should go a long way to simplifying a lot of these validation
implementations.

Any thoughts on that?

IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
tends to add complexity to validate(), because we have a lot of code
like

	if (state->interface == PHY_INTERFACE_MODE_FOO) {
		if (we_support_foo())
			phylink_set(mask, Foo);
		else if (state->interface != PHY_INTERFACE_MODE_NA) {
			linkmode_zero(supported);
			return;
		}
	}

which gets even worse when we want to have different interfaces share
logic. IMO validate() could be much cleaner if we never called it with
NA and instead did something like

	if (state->interface == PHY_INTERFACE_MODE_NA) {
		unsigned long *original;

		linkmode_copy(original, supported);
		for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
			if (test_bit(i, pl->config->supported_interfaces)) {
				unsigned long *iface_mode;

				linkmode_copy(iface_mode, original);
				state->interface = i;
				pl->mac_ops->validate(pl->config, iface_mode, state);
				linkmode_or(supported, supported, iface_mode);
			}
		}
		state->interface = PHY_INTERFACE_MODE_NA;
	}

This of course can be done in addition to/instead of your above
suggestion. I suggested something like this in v3 of this series, but it
would be even better to do this on the phylink level.

--Sean