Re: [PATCH net-next 5/5] net: dsa: b53: mark as non-legacy

[Florian Fainelli <f.fainelli@gmail.com>]

On 4/22/22 00:39, Russell King (Oracle) wrote:
> On Thu, Apr 21, 2022 at 03:31:26PM -0700, Florian Fainelli wrote:
>> Hi Russell,
>>
>> On 2/22/22 02:16, Russell King (Oracle) wrote:
>>> The B53 driver does not make use of the speed, duplex, pause or
>>> advertisement in its phylink_mac_config() implementation, so it can be
>>> marked as a non-legacy driver.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>    drivers/net/dsa/b53/b53_common.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index 50a372dc32ae..83bf30349c26 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1346,6 +1346,12 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
>>>    	/* Get the implementation specific capabilities */
>>>    	if (dev->ops->phylink_get_caps)
>>>    		dev->ops->phylink_get_caps(dev, port, config);
>>> +
>>> +	/* This driver does not make use of the speed, duplex, pause or the
>>> +	 * advertisement in its mac_config, so it is safe to mark this driver
>>> +	 * as non-legacy.
>>> +	 */
>>> +	config->legacy_pre_march2020 = false;
>>
>> This patch appears to cause a regression for me, I am not sure why I did not
>> notice it back when I tested it but I suspect it had to do with me testing
>> only with a copper module and not with a fiber module.
>>
>> Now that I tested it again, the SFP port (port 5 in my set-up) link up
>> interrupt does not fire up when setting config->legacy_pre_march2020 to
>> false.
>>
>> Here is a working log with phylink debugging enabled:
>>
>> # udhcpc -i sfp
>> udhcpc: started, v1.35.0
>> [   49.479637] bgmac-enet 18024000.ethernet eth2: Link is Up - 1Gbps/Full -
>> flow control off
>> [   49.488139] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
>> [   49.488256] b53-srab-switch 18036000.ethernet-switch sfp: configuring for
>> inband/1000base-x link mode
>> [   49.504062] b53-srab-switch 18036000.ethernet-switch sfp: major config
>> 1000base-x
>> [   49.511800] b53-srab-switch 18036000.ethernet-switch sfp:
>> phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown
>> adv=0000000,00000201
>> [   49.527504] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
>> [   49.535044] sfp sfp: SM: enter present:down:down event dev_up
>> [   49.541006] sfp sfp: tx disable 1 -> 0
>> [   49.544897] sfp sfp: SM: exit present:up:wait
>> [   49.549509] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
>> udhcpc: broadcasting discover
>> [   49.595185] sfp sfp: SM: enter present:up:wait event timeout
>> [   49.601064] sfp sfp: SM: exit present:up:link_up
>> [   52.388917] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
>> [   52.396513] b53-srab-switch 18036000.ethernet-switch sfp: Link is Up -
>> 1Gbps/Full - flow control rx/tx
>> [   52.406145] IPv6: ADDRCONF(NETDEV_CHANGE): sfp: link becomes ready
>> udhcpc: broadcasting discover
>> udhcpc: broadcasting select for 192.168.3.156, server 192.168.3.1
>> udhcpc: lease of 192.168.3.156 obtained from 192.168.3.1, lease time 600
>> deleting routers
>> adding dns 192.168.1.1
>>
>> and one that is not working with phylink debugging enabled:
>>
>> # udhcpc -i sfp
>> udhcpc: started, v1.35.0
>> [   27.863529] bgmac-enet 18024000.ethernet eth2: Link is Up - 1Gbps/Full -
>> flow control off
>> [   27.872021] Generic PHY fixed-0:02: PHY state change UP -> RUNNING
>> [   27.872120] b53-srab-switch 18036000.ethernet-switch sfp: configuring for
>> inband/1000base-x link mode
>> [   27.887952] b53-srab-switch 18036000.ethernet-switch sfp: major config
>> 1000base-x
>> [   27.895689] b53-srab-switch 18036000.ethernet-switch sfp:
>> phylink_mac_config: mode=inband/1000base-x/Unknown/Unknown
>> adv=0000000,00000201
>> [   27.895802] b53-srab-switch 18036000.ethernet-switch sfp: mac link down
>> [   27.911945] sfp sfp: SM: enter present:down:down event dev_up
>> [   27.923947] sfp sfp: tx disable 1 -> 0
>> [   27.927835] sfp sfp: SM: exit present:up:wait
>> [   27.932442] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready
>> udhcpc: broadcasting discover
>> [   27.978181] sfp sfp: SM: enter present:up:wait event timeout
>> [   27.984056] sfp sfp: SM: exit present:up:link_up
>> [   30.686440] b53-srab-switch 18036000.ethernet-switch sfp: mac link up
>> udhcpc: broadcasting discover
>> udhcpc: broadcasting discover
>>
>> The mac side appears to be UP but not no carrier is set to the sfp network
>> device. Do you have any idea why that would happen?
> 
> Oh, it's because setting that flag means we're wanting the PCS methods
> rather than the legacy MAC methods for an_restart and getting the PCS
> link state - so the patch in question was submitted too early (it
> should have been _after_ the conversion to PCS.)

Meh, sorry I was really slow on this and did not even connect the dots. 
Indeed that is what it is.

> 
> If we get the patch reverted in net-next, and then convert b53 to use
> PCS support, we'll then be putting the patch back, so I wonder if it
> would just make sense to apply the PCS conversion patch, possibly
> adding a comment in the commit message pointing out that this fixes
> the b53 legacy_pre_march2020 patch. Thoughts?

I just responded to your other patch "net: dsa: b53: convert to 
phylink_pcs", so if we target that one for 'net' I think we should be 
good to go.

Thanks!
-- 
Florian