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

On 10/14/21 7:08 PM, Russell King (Oracle) wrote:

On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:

quoted

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

quoted

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.

There is always the option to use different operations structs if the
properties of the interfaces can be divided up in that way - and that
will probably be more efficient (not that the validate callback is a
performance critical path though.)

quoted

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.

In addition I think - I think we should use a non-empty
supported_interfaces as an indicator that we use the above, otherwise
we have to loop through all possible interface modes. That also
provides some encouragement to fill out the supported_interfaces
member.

I had a stab at this today [1], but it is only compile-tested. In order
to compile "net: phylink: use phy_interface_t bitmaps for optical
modules", I needed to run

	sed -ie 's/sfp_link_an_mode/cfg_link_an_mode/g' drivers/net/phy/phylink.c

Do you plan on making up a series for this? I think the end result is
much nicer that v3 of this series.

--Sean

[1] https://github.com/sean-anderson-seco/linux/commits/supported_interfaces_wip