Re: Regression: supported_interfaces filling enforcement

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Jul 25, 2023 at 12:26:00PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 25, 2023 at 12:58:31PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> > [CCing the regression list, as it should be in the loop for regressions:
> > https://docs.kernel.org/admin-guide/reporting-regressions.html]
> > 
> > On 13.07.23 17:08, Russell King (Oracle) wrote:
> > > On Mon, Jul 10, 2023 at 03:35:56PM +0300, Vladimir Oltean wrote:
> > >> On Tue, Jul 04, 2023 at 05:28:47PM +0300, Sergei Antonov wrote:
> > >>> This commit seems to break the mv88e6060 dsa driver:
> > >>> de5c9bf40c4582729f64f66d9cf4920d50beb897    "net: phylink: require
> > >>> supported_interfaces to be filled"
> > >>>
> > >>> The driver does not fill 'supported_interfaces'. What is the proper
> > >>> way to fix it? I managed to fix it by the following quick code.
> > >>> Comments? Recommendations?
> > >>
> > >> Ok, it seems that commit de5c9bf40c45 ("net: phylink: require
> > >> supported_interfaces to be filled") was based on a miscalculation.
> > > 
> > > Yes, it seems so. I'm not great with dealing with legacy stuff - which
> > > is something I've stated time and time again when drivers fall behind
> > > with phylink development. There's only so much that I can hold in my
> > > head, and I can't runtime test the legacy stuff.
> > > 
> > > I suspect two other DSA drivers are also broken by this:
> > > 
> > > drivers/net/dsa/dsa_loop.c
> > > drivers/net/dsa/realtek/rtl8366rb.c
> > > 
> > > based upon:
> > > 
> > > $ grep -lr dsa_switch_ops drivers/net/dsa | xargs grep -L '\.phylink_get_caps.*=' | xargs grep -L '\.adjust_link'
> > 
> > What happened to this regression? From here it looks like things
> > stalled, but I might have missed something, hence allow me to ask:
> > 
> > Is this still happening? Is anyone still working on fixing this?
> 
> I think the discussion got side-tracked into whether mv88e6060 should
> be merged into mv88e6xxx, and then just petered out with no further
> patch(es) - plus I was on holiday so obviously wasn't paying much
> attention.
> 
> I suppose the sane thing to do would be to fix all drivers in one
> go - maybe something like this:
> 
> -	if (ds->ops->phylink_get_caps)
> +	if (ds->ops->phylink_get_caps) {
>  		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
> +	} else {
> +		/* For legacy drivers */
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  &dp->pl_config.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  &dp->pl_config.supported_interfaces);
> +	}
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Yup, dsa_loop is broken too, I've just tested it by enabling
CONFIG_NET_DSA_LOOP=y and booting on a board which has an existing
interface named eth0 (this will be used as fake DSA master):

[    4.936197] dsa-loop fixed-0:1f: skipping link registration for CPU port 5
[    4.944316]  (null): phylink: error: empty supported_interfaces
[    4.950551] error creating PHYLINK: -22
[    4.954467] dsa-loop fixed-0:1f lan1 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 0
[    4.964869]  (null): phylink: error: empty supported_interfaces
[    4.970932] error creating PHYLINK: -22
[    4.974833] dsa-loop fixed-0:1f lan2 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 1
[    4.985131]  (null): phylink: error: empty supported_interfaces
[    4.991120] error creating PHYLINK: -22
[    4.995002] dsa-loop fixed-0:1f lan3 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 2
[    5.005277]  (null): phylink: error: empty supported_interfaces
[    5.011267] error creating PHYLINK: -22
[    5.015148] dsa-loop fixed-0:1f lan4 (uninitialized): error -22 setting up PHY for tree 0, switch 0, port 3
[    5.026292] DSA: tree 0 setup
[    5.029282] dsa-loop fixed-0:1f: DSA mockup driver: 0x1f

I've also tested that your patch (fixed so that it doesn't take the "&"
address of supported_interfaces, which is already an array type)
restores the functionality:

[    4.949944] dsa-loop fixed-0:1f: skipping link registration for CPU port 5
[    4.957757] dsa-loop fixed-0:1f lan1 (uninitialized): PHY [dsa-0.0:00] driver [Generic PHY] (irq=POLL)
[    4.973085] dsa-loop fixed-0:1f lan2 (uninitialized): PHY [dsa-0.0:01] driver [Generic PHY] (irq=POLL)
[    4.986189] dsa-loop fixed-0:1f lan3 (uninitialized): PHY [dsa-0.0:02] driver [Generic PHY] (irq=POLL)
[    4.998763] dsa-loop fixed-0:1f lan4 (uninitialized): PHY [dsa-0.0:03] driver [Generic PHY] (irq=POLL)
[    5.012044] DSA: tree 0 setup
[    5.015040] dsa-loop fixed-0:1f: DSA mockup driver: 0x1f

Russell, can you please go ahead and turn the short-term fix into a
formal patch?

Once I get rid of some of the pending stuff that's keeping me busy,
I've made a note to not let the short-term workarounds (this and
dsa_port_phylink_validate() avoiding phylink_generic_validate() if
mac_capabilities isn't provided) turn into long-term workarounds that
new drivers, too, might start relying on.