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

[Sean Anderson <sean.anderson@seco.com>]

On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
> On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote:
>> Hello Sean,
>>
>> Quoting Sean Anderson (2021-10-11 18:55:16)
>> > 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