| Message ID | 20231121152426.4188456-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1,1/3] net: dsa: microchip: ksz8: move BMCR specific code to separate function | expand |
Hi, On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote: > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > driver. Previously, the code erroneously used Bit 7 of port register 0xD > for both chip variants, which is actually for LED configuration. This > update ensures the correct registers and bits are used for the PHY > loopback feature: > > - For KSZ8794: Use 0xF / Bit 7. > - For KSZ8873: Use 0xD / Bit 0. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> This looks like a bugfix, so possibly worth a Fixes tag? Given the dependency on the previous refactor, I think we can take it via net- next. @Andrew, Florian, Vladimir: do you have any specific preference here? Thanks! Paolo
On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote: > Hi, > > On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote: > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > > driver. Previously, the code erroneously used Bit 7 of port register 0xD > > for both chip variants, which is actually for LED configuration. This > > update ensures the correct registers and bits are used for the PHY > > loopback feature: > > > > - For KSZ8794: Use 0xF / Bit 7. > > - For KSZ8873: Use 0xD / Bit 0. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > This looks like a bugfix, so possibly worth a Fixes tag? Given the > dependency on the previous refactor, I think we can take it via net- > next. > > @Andrew, Florian, Vladimir: do you have any specific preference here? I do not think any one cares about supporting this switch variant in stable :) Regards, Oleksij
On Wed, Dec 06, 2023 at 09:55:20AM +0100, Oleksij Rempel wrote: > On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote: > > Hi, > > > > On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote: > > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > > > driver. Previously, the code erroneously used Bit 7 of port register 0xD > > > for both chip variants, which is actually for LED configuration. This > > > update ensures the correct registers and bits are used for the PHY > > > loopback feature: > > > > > > - For KSZ8794: Use 0xF / Bit 7. > > > - For KSZ8873: Use 0xD / Bit 0. > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > > This looks like a bugfix, so possibly worth a Fixes tag? Given the > > dependency on the previous refactor, I think we can take it via net- > > next. > > > > @Andrew, Florian, Vladimir: do you have any specific preference here? > > I do not think any one cares about supporting this switch variant in > stable :) > > Regards, > Oleksij Sorry, this simply fell through the cracks. How is PHY loopback even supposed to be triggered? User space flips NETIF_F_LOOPBACK on the netdev, driver ndo_set_features() catches it and calls phy_loopback() and this calls into phylib's phydev->drv->set_loopback() or the generic genphy_loopback()? I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK. The PHY is integrated, so DSA is the only relevant netdev driver. Is there any other way to test this functionality? If not, I think it's a case of "tree falling in the woods and nobody hearing it". Not "stable" material. But it definitely has nothing to do with not caring about the switch variant. If my analysis is correct, then I actually have a suggestion for you, Oleksij. Using the F word ("fix") can work against you, if you don't have enough proof that you're really fixing something which has a user visible impact. So either do a thorough analysis of the impact in the commit message, or don't use the F word.
On Wed, Dec 06, 2023 at 05:14:06PM +0200, Vladimir Oltean wrote: > On Wed, Dec 06, 2023 at 09:55:20AM +0100, Oleksij Rempel wrote: > > On Thu, Nov 23, 2023 at 11:52:57AM +0100, Paolo Abeni wrote: > > > Hi, > > > > > > On Tue, 2023-11-21 at 16:24 +0100, Oleksij Rempel wrote: > > > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > > > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > > > > driver. Previously, the code erroneously used Bit 7 of port register 0xD > > > > for both chip variants, which is actually for LED configuration. This > > > > update ensures the correct registers and bits are used for the PHY > > > > loopback feature: > > > > > > > > - For KSZ8794: Use 0xF / Bit 7. > > > > - For KSZ8873: Use 0xD / Bit 0. > > > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > > > > This looks like a bugfix, so possibly worth a Fixes tag? Given the > > > dependency on the previous refactor, I think we can take it via net- > > > next. > > > > > > @Andrew, Florian, Vladimir: do you have any specific preference here? > > > > I do not think any one cares about supporting this switch variant in > > stable :) > > > > Regards, > > Oleksij > > Sorry, this simply fell through the cracks. > > How is PHY loopback even supposed to be triggered? User space flips > NETIF_F_LOOPBACK on the netdev, driver ndo_set_features() catches it and > calls phy_loopback() and this calls into phylib's phydev->drv->set_loopback() > or the generic genphy_loopback()? correct. > I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK. > The PHY is integrated, so DSA is the only relevant netdev driver. Is > there any other way to test this functionality? yes - net_selftest() > If not, I think it's a case of "tree falling in the woods and nobody > hearing it". Not "stable" material. But it definitely has nothing to do > with not caring about the switch variant. Sorry, my intention is not to criticize anyone. I am not getting feedbacks or bug reports for ksz88xx variants, so it seems like not many people use it in upstream. When I have time slots to work on this driver, I try to use them to do fixes and also clean up the code. Since there is some sort of fog of uncertainty about when I get the next time slot, or even if I get it at all, I am trying to push both fixes and cleanups together. But, you are right, it is not a good reason for not caring about stable :) What is the decision about this patch set? Regards, Oleksij
On Wed, Dec 06, 2023 at 04:54:40PM +0100, Oleksij Rempel wrote: > > I don't see DSA implementing ndo_set_features(), nor offering NETIF_F_LOOPBACK. > > The PHY is integrated, so DSA is the only relevant netdev driver. Is > > there any other way to test this functionality? > > yes - net_selftest() Ok, I didn't notice net_test_phy_loopback_enable(). So it can be triggered after all, it seems. But I mean, if it's exclusively a selftest that fails, and has always failed since its introduction, I think it can be considered new development work when it stops failing? I don't believe that the impact of the bug is relevant for users. It's not a production functionality. Documentation/process/stable-kernel-rules.rst doesn't specifically say this, but it does imply that we should triage the "real bugs that bother people" as much as possible. > > If not, I think it's a case of "tree falling in the woods and nobody > > hearing it". Not "stable" material. But it definitely has nothing to do > > with not caring about the switch variant. > > Sorry, my intention is not to criticize anyone. I am not getting > feedbacks or bug reports for ksz88xx variants, so it seems like not many > people use it in upstream. > > When I have time slots to work on this driver, I try to use them to do > fixes and also clean up the code. Since there is some sort of fog of > uncertainty about when I get the next time slot, or even if I get it at > all, I am trying to push both fixes and cleanups together. > > But, you are right, it is not a good reason for not caring about stable :) > > What is the decision about this patch set? I wouldn't bend over backwards for this, and reorder the patches. I would spend my time doing more meaningful things.
On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote: > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > driver. Previously, the code erroneously used Bit 7 of port register 0xD > for both chip variants, which is actually for LED configuration. This > update ensures the correct registers and bits are used for the PHY > loopback feature: > > - For KSZ8794: Use 0xF / Bit 7. > - For KSZ8873: Use 0xD / Bit 0. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- How did you find, and how did you test this, and on which one of the switches? Opening the KSZ8873 datasheet, I am confused about their description of the "far-end loopback". They make it sound as if this loops the packets _received_ from the media side of PHY port A back to the transmit side of PHY port A. But the route that these packets take is through the MAC of PHY port A, then the switching fabric, then PHY port B which reflects them back to PHY port A, where they finally egress. Actually, they even go as far as saying that if you set the loopback bit of port 1, the packets that will be looped back will be from port 2's RXP/RXM to TXP/TXM pins, and viceversa. If true, I believe this isn't the behavior expected by phy_loopback(), where the TX signals from the media side of the PHY are looped back into the RX side.
On Thu, Dec 07, 2023 at 02:28:23AM +0200, Vladimir Oltean wrote: > On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote: > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > > driver. Previously, the code erroneously used Bit 7 of port register 0xD > > for both chip variants, which is actually for LED configuration. This > > update ensures the correct registers and bits are used for the PHY > > loopback feature: > > > > - For KSZ8794: Use 0xF / Bit 7. > > - For KSZ8873: Use 0xD / Bit 0. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > How did you find, and how did you test this, and on which one of the switches? I tested it by using "ethtool -t lanX" command on KSZ8873. Before this patch the link will stop to work _after_ end of the selftest. The selftest will fail too. After this patch, the selftest is passed, except of the TCP test. And link is working _after_ the selftest, > Opening the KSZ8873 datasheet, I am confused about their description of > the "far-end loopback". They make it sound as if this loops the packets > _received_ from the media side of PHY port A back to the transmit side > of PHY port A. But the route that these packets take is through the MAC > of PHY port A, then the switching fabric, then PHY port B which reflects > them back to PHY port A, where they finally egress. > > Actually, they even go as far as saying that if you set the loopback bit > of port 1, the packets that will be looped back will be from port 2's > RXP/RXM to TXP/TXM pins, and viceversa. > > If true, I believe this isn't the behavior expected by phy_loopback(), > where the TX signals from the media side of the PHY are looped back into > the RX side. > >
On Thu, Dec 07, 2023 at 06:15:02AM +0100, Oleksij Rempel wrote: > On Thu, Dec 07, 2023 at 02:28:23AM +0200, Vladimir Oltean wrote: > > On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote: > > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > > > driver. Previously, the code erroneously used Bit 7 of port register 0xD > > > for both chip variants, which is actually for LED configuration. This > > > update ensures the correct registers and bits are used for the PHY > > > loopback feature: > > > > > > - For KSZ8794: Use 0xF / Bit 7. > > > - For KSZ8873: Use 0xD / Bit 0. > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > --- > > > > How did you find, and how did you test this, and on which one of the switches? > > I tested it by using "ethtool -t lanX" command on KSZ8873. Before this > patch the link will stop to work _after_ end of the selftest. The > selftest will fail too. > > After this patch, the selftest is passed, except of the TCP test. And > link is working _after_ the selftest, So you are suggesting that this far-end loopback mode does work as expected by the kernel. But is that consistent with the description from the datasheet? It speaks about an "originating PHY port", but maybe this is confusing, because based on your test, even the CPU port could be originating the traffic that gets looped back? I see it says that far-end loopback goes through the switching fabric. So the packet, on its return path from the loopback port, gets forwarded by its MAC DA? That can't be, because the MAC DA lookup has already determined the destination to be the loopback port (and no MAC SA<->DA swapping should take place). Or it is forced by the switch to return specifically to the originating port? With a bridge between the 2 LAN ports, and lan1 put in loopback, what happens if you send a broadcast packet towards lan1? Will you also see it on lan2's link partner, or only on the CPU port? It's not your fault, but this is all a bit confusing, and I'm not quite able to match up the documentation with your results. I will trust the experimental results, however.
On Thu, Dec 07, 2023 at 04:00:30PM +0200, Vladimir Oltean wrote: > On Thu, Dec 07, 2023 at 06:15:02AM +0100, Oleksij Rempel wrote: > > On Thu, Dec 07, 2023 at 02:28:23AM +0200, Vladimir Oltean wrote: > > > On Tue, Nov 21, 2023 at 04:24:26PM +0100, Oleksij Rempel wrote: > > > > Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and > > > > ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 > > > > driver. Previously, the code erroneously used Bit 7 of port register 0xD > > > > for both chip variants, which is actually for LED configuration. This > > > > update ensures the correct registers and bits are used for the PHY > > > > loopback feature: > > > > > > > > - For KSZ8794: Use 0xF / Bit 7. > > > > - For KSZ8873: Use 0xD / Bit 0. > > > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > > --- > > > > > > How did you find, and how did you test this, and on which one of the switches? > > > > I tested it by using "ethtool -t lanX" command on KSZ8873. Before this > > patch the link will stop to work _after_ end of the selftest. The > > selftest will fail too. > > > > After this patch, the selftest is passed, except of the TCP test. And > > link is working _after_ the selftest, > > So you are suggesting that this far-end loopback mode does work as > expected by the kernel. > > But is that consistent with the description from the datasheet? It speaks > about an "originating PHY port", but maybe this is confusing, because > based on your test, even the CPU port could be originating the traffic > that gets looped back? > > I see it says that far-end loopback goes through the switching fabric. > So the packet, on its return path from the loopback port, gets forwarded > by its MAC DA? That can't be, because the MAC DA lookup has already > determined the destination to be the loopback port (and no MAC SA<->DA > swapping should take place). Or it is forced by the switch to return > specifically to the originating port? > > With a bridge between the 2 LAN ports, and lan1 put in loopback, what > happens if you send a broadcast packet towards lan1? Will you also see > it on lan2's link partner, or only on the CPU port? > > It's not your fault, but this is all a bit confusing, and I'm not quite > able to match up the documentation with your results. I will trust the > experimental results, however. I did following tests: ip l a name br0 type bridge ip l s dev br0 up ip l s lan1 master br0 ip l s dev lan1 up ip l s lan2 master br0 ip l s dev lan2 up # to avoid link drop with the loopback mode ethtool -s lan1 speed 100 duplex full autoneg off # currently no tool support loopback configuration, so set it directly # to the register mii -i lan1 -p 0 -r 0 -v 6120 ################# Test 1 ########################### # on DUT ping 192.168.2.200 -I lan1 on eth0/cpu interface: 00:03:20.656445 AF Unknown (4294967295), length 61: 0x0000: ffff 000e cd00 cdbe 0806 0001 0800 0604 ................ 0x0010: 0001 000e cd00 cdbe c0a8 010e 0000 0000 ................ 0x0020: 0000 c0a8 02c8 0000 0000 0000 0000 0000 ................ 0x0030: 0000 0000 0000 0000 01 ......... 00:03:20.656548 AF Unknown (4294967295), length 61: 0x0000: ffff 000e cd00 cdbe 0806 0001 0800 0604 ................ 0x0010: 0001 000e cd00 cdbe c0a8 010e 0000 0000 ................ 0x0020: 0000 c0a8 02c8 0000 0000 0000 0000 0000 ................ 0x0030: 0000 0000 0000 0000 00 ......... on lan1 remote system: 100 701.752654927 00:0e:cd:00:cd:be → ff:ff:ff:ff:ff:ff ARP 60 Who has 192.168.2.200? Tell 192.168.1.14 on lan2 remote system: 347 1071.154437549 00:0e:cd:00:cd:be → ff:ff:ff:ff:ff:ff ARP 60 Who has 192.168.2.200? Tell 192.168.1.14 ################# Test 2 ########################### # send ping on on lan1 remote system: ping 172.17.0.1 on eth0/cpu interface DUT: -- nothing -- on lan1 remote system: 109 946.617862621 80:61:5f:0c:29:54 → ff:ff:ff:ff:ff:ff ARP 42 Who has 172.17.0.1? Tell 172.17.0.12 on lan2 remote system: -- nothing -- ################# Test 3 ########################### # send ping on on lan2 remote system: ping 172.17.0.122 on eth0/cpu interface DUT: 00:12:18.034220 AF Unknown (4294967295), length 61: 0x0000: ffff 8061 5f0c 2955 0806 0001 0800 0604 ...a_.)U........ 0x0010: 0001 8061 5f0c 2955 ac11 000d 0000 0000 ...a_.)U........ 0x0020: 0000 ac11 007a 0000 0000 0000 0000 0000 .....z.......... 0x0030: 0000 0000 0000 0000 01 ......... 00:12:18.034228 AF Unknown (4294967295), length 61: 0x0000: ffff 8061 5f0c 2955 0806 0001 0800 0604 ...a_.)U........ 0x0010: 0001 8061 5f0c 2955 ac11 000d 0000 0000 ...a_.)U........ 0x0020: 0000 ac11 007a 0000 0000 0000 0000 0000 .....z.......... 0x0030: 0000 0000 0000 0000 00 on lan1 remote system: -- nothing -- on lan2 remote system: 476 1608.560311470 80:61:5f:0c:29:55 → ff:ff:ff:ff:ff:ff ARP 42 Who has 172.17.0.122? Tell 172.17.0.13 477 1608.560361808 80:61:5f:0c:29:55 → ff:ff:ff:ff:ff:ff ARP 60 Who has 172.17.0.122? Tell 172.17.0.13 I also retest selftest results and noted that it is not working if port is part of bridge. Regards, Oleksij
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 4c1e21fd87da..5b223d472548 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -731,16 +731,25 @@ static int ksz8_r_phy_bmcr(struct ksz_device *dev, int port, u16 *val) if (ret) return ret; - if (restart & PORT_PHY_LOOPBACK) - *val |= BMCR_LOOPBACK; - if (ctrl & PORT_FORCE_100_MBIT) *val |= BMCR_SPEED100; if (ksz_is_ksz88x3(dev)) { + if (restart & KSZ8873_PORT_PHY_LOOPBACK) + *val |= BMCR_LOOPBACK; + if ((ctrl & PORT_AUTO_NEG_ENABLE)) *val |= BMCR_ANENABLE; } else { + u8 stat3; + + ret = ksz_pread8(dev, port, REG_PORT_STATUS_3, &stat3); + if (ret) + return ret; + + if (stat3 & PORT_PHY_LOOPBACK) + *val |= BMCR_LOOPBACK; + if (!(ctrl & PORT_AUTO_NEG_DISABLE)) *val |= BMCR_ANENABLE; } @@ -994,8 +1003,7 @@ static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val) restart = 0; restart_mask = PORT_LED_OFF | PORT_TX_DISABLE | PORT_AUTO_NEG_RESTART | - PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX | - PORT_PHY_LOOPBACK; + PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX; if (val & KSZ886X_BMCR_DISABLE_LED) restart |= PORT_LED_OFF; if (val & KSZ886X_BMCR_DISABLE_TRANSMIT) @@ -1008,8 +1016,23 @@ static int ksz8_w_phy_bmcr(struct ksz_device *dev, int port, u16 val) restart |= PORT_AUTO_MDIX_DISABLE; if (val & KSZ886X_BMCR_FORCE_MDI) restart |= PORT_FORCE_MDIX; - if (val & BMCR_LOOPBACK) - restart |= PORT_PHY_LOOPBACK; + + if (ksz_is_ksz88x3(dev)) { + restart_mask |= KSZ8873_PORT_PHY_LOOPBACK; + + if (val & BMCR_LOOPBACK) + restart |= KSZ8873_PORT_PHY_LOOPBACK; + } else { + u8 stat3 = 0; + + if (val & BMCR_LOOPBACK) + stat3 |= PORT_PHY_LOOPBACK; + + ret = ksz_prmw8(dev, port, REG_PORT_STATUS_3, PORT_PHY_LOOPBACK, + stat3); + if (ret) + return ret; + } return ksz_prmw8(dev, port, regs[P_NEG_RESTART_CTRL], restart_mask, restart); @@ -1029,8 +1052,6 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val) ret = ksz8_w_phy_bmcr(dev, p, val); if (ret) return ret; - - break; case MII_ADVERTISE: ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl); diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h index 3c9dae53e4d8..4c07e0e1fcca 100644 --- a/drivers/net/dsa/microchip/ksz8795_reg.h +++ b/drivers/net/dsa/microchip/ksz8795_reg.h @@ -262,6 +262,7 @@ #define PORT_AUTO_MDIX_DISABLE BIT(2) #define PORT_FORCE_MDIX BIT(1) #define PORT_MAC_LOOPBACK BIT(0) +#define KSZ8873_PORT_PHY_LOOPBACK BIT(0) #define REG_PORT_1_STATUS_2 0x1E #define REG_PORT_2_STATUS_2 0x2E
Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795 driver. Previously, the code erroneously used Bit 7 of port register 0xD for both chip variants, which is actually for LED configuration. This update ensures the correct registers and bits are used for the PHY loopback feature: - For KSZ8794: Use 0xF / Bit 7. - For KSZ8873: Use 0xD / Bit 0. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz8795.c | 39 +++++++++++++++++++------ drivers/net/dsa/microchip/ksz8795_reg.h | 1 + 2 files changed, 31 insertions(+), 9 deletions(-)