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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
> On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
> > 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.)

> 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!