| Message ID | E1nMSDS-00A2Ru-6J@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: b53: convert to phylink_generic_validate() and mark as non-legacy | expand |
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?
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.) 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?
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!
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; } int b53_phylink_mac_link_state(struct dsa_switch *ds, int port,
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(+)