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

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

On Fri, Oct 15, 2021 at 06:28:10PM -0400, Sean Anderson wrote:
> On 10/14/21 7:08 PM, Russell King (Oracle) wrote:
> > 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.
> 
> 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.

I have been working on it but haven't finished the patches yet. There's
a few issues that came up with e.g. DSA and mvneta being able to switch
between different speeds with some SFP modules that have needed other
tweaks.

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