| Message ID | 20240627053353.1416261-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1,1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0 | expand |
On Thu, Jun 27, 2024 at 07:33:53AM +0200, Oleksij Rempel wrote: > Do not disable broadcast if we are using address 0 (broadcast) to > communicate with this device. Otherwise we will use proper driver but no > communication will be possible and no link changes will be detected. > There are two scenarios where we can run in to this situation: > - PHY is bootstrapped for address 0 What do you mean by bootstrapped to address 0? The strapping pins set it to some other address, but the bootloader wrote to registers and moved it to address 0? > - no PHY address is known and linux is scanning the MDIO bus, so first > respond and attached device will be on address 0. So in this case, the PHY is really at address X, where X != 0. It responds to all read requests, so the scanning finds it at all addresses. It also stomps over other devices on the bus when scanning for them, or probing them. I'm not sure the current code is correct. But it is also going to be messy to not break backwards compatibility for DT blobs say the device is at address 0, when in fact it is not. Is it possible to read the devices actual address from registers? I'm wondering if probe should do something like: int actual_address = phydev_read(phydev, 0x42); if (actual_address == 0) { if (type->has_broadcast_disable) { phydev_dbg(phydev, "Disabling broadcast\n"); kszphy_broadcast_disable(phydev); } } else { if (actual_address != 0 && phydev->mdio.addr != actual_address && phydev->mdio.addr != 0) { if (type->has_broadcast_disable) { phydev_dbg(phydev, "Disabling broadcast\n"); kszphy_broadcast_disable(phydev); } return -ENODEV; } } So if the devices really has an address is zero, turn off broadcast. That will stop it stomping over other devices, but the damage is probably already done in terms of scanning. If the devices is really at some address other than 0, and we are probing at a different address, and that address is not 0, turn off broadcast and say the device does not exist. I think we need to special case 0 because there are going to be some DT descriptions which say the device is at 0, when in fact it is not. We might want to add a phydev_warn() about this, to try to get the DT fixed. > The fixes tag points to the latest refactoring, not to the initial point > where kszphy_broadcast_disable() was introduced. > > Fixes: 79e498a9c7da0 ("net: phy: micrel: Restore led_mode and clk_sel on resume") > Cc: stable@vger.kernel.org > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Do you have a board which is going wrong because of this? Do you plan to submit patches for earlier stable releases? Andrew
Hi Andrew, hm... looks like my previous answer is lost. I had here some hiccups on my side. Sending new answer, do not wunder if there will be double mail. On Thu, Jun 27, 2024 at 03:56:42PM +0200, Andrew Lunn wrote: > On Thu, Jun 27, 2024 at 07:33:53AM +0200, Oleksij Rempel wrote: > > Do not disable broadcast if we are using address 0 (broadcast) to > > communicate with this device. Otherwise we will use proper driver but no > > communication will be possible and no link changes will be detected. > > There are two scenarios where we can run in to this situation: > > - PHY is bootstrapped for address 0 > > What do you mean by bootstrapped to address 0? The strapping pins set > it to some other address, but the bootloader wrote to registers and > moved it to address 0? No no. Just strapping from HW perspective, no SW is involved. > > - no PHY address is known and linux is scanning the MDIO bus, so first > > respond and attached device will be on address 0. > > So in this case, the PHY is really at address X, where X != 0. It > responds to all read requests, so the scanning finds it at all > addresses. It also stomps over other devices on the bus when scanning > for them, or probing them. > > I'm not sure the current code is correct. But it is also going to be > messy to not break backwards compatibility for DT blobs say the device > is at address 0, when in fact it is not. It looks like this is the actual case on my board. > Is it possible to read the devices actual address from registers? Yes. > I'm wondering if probe should do something like: > > int actual_address = phydev_read(phydev, 0x42); > > if (actual_address == 0) { > if (type->has_broadcast_disable) { > phydev_dbg(phydev, "Disabling broadcast\n"); > kszphy_broadcast_disable(phydev); > } > > } else { > if (actual_address != 0 && > phydev->mdio.addr != actual_address && > phydev->mdio.addr != 0) { > if (type->has_broadcast_disable) { > phydev_dbg(phydev, "Disabling broadcast\n"); > kszphy_broadcast_disable(phydev); > } > return -ENODEV; > } > } > > So if the devices really has an address is zero, turn off > broadcast. That will stop it stomping over other devices, but the If i understand the documentation correctly, disable broadcast bit is designed to resolve conflict between two KSZ8081 PHYs on same bus in one shot: "For applications that require two KSZ8081RNA/RND PHYs to share the same MDIO interface with one PHY set to address 0h and the other PHY set to address 3h, use PHY address 0h (defaults to broadcast after power-up) to set both PHYs’ Register 16h, Bit [9] to ‘1’ to assign PHY address 0h as a unique (non-broadcast) PHY address." For this scenario, we can't read configured address from the HW in the first place, this can be done safely only if (phydev->mdio.addr != 0) > damage is probably already done in terms of scanning. > > If the devices is really at some address other than 0, and we are > probing at a different address, and that address is not 0, turn off > broadcast and say the device does not exist. I think we need to > special case 0 because there are going to be some DT descriptions > which say the device is at 0, when in fact it is not. We might want to > add a phydev_warn() about this, to try to get the DT fixed. Assuming there are no other devices on the bus listening for broadcast too. > > The fixes tag points to the latest refactoring, not to the initial point > > where kszphy_broadcast_disable() was introduced. > > > > Fixes: 79e498a9c7da0 ("net: phy: micrel: Restore led_mode and clk_sel on resume") > > Cc: stable@vger.kernel.org > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > Do you have a board which is going wrong because of this? Yes, I can avoid this bug on the DT level. But the code side seems to be broken too. > Do you plan to submit patches for earlier stable releases? Not so far. Should I?
On Thu, 27 Jun 2024 07:33:53 +0200 Oleksij Rempel wrote: > Do not disable broadcast if we are using address 0 (broadcast) to > communicate with this device. Otherwise we will use proper driver but no > communication will be possible and no link changes will be detected. > There are two scenarios where we can run in to this situation: > - PHY is bootstrapped for address 0 > - no PHY address is known and linux is scanning the MDIO bus, so first > respond and attached device will be on address 0. > > The fixes tag points to the latest refactoring, not to the initial point > where kszphy_broadcast_disable() was introduced. > > Fixes: 79e498a9c7da0 ("net: phy: micrel: Restore led_mode and clk_sel on resume") Is there a reason you're not CCing the author ?
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 81c20eb4b54b9..67c2e611150d2 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -590,7 +590,7 @@ static int kszphy_config_init(struct phy_device *phydev) type = priv->type; - if (type && type->has_broadcast_disable) + if (type && type->has_broadcast_disable && phydev->mdio.addr != 0) kszphy_broadcast_disable(phydev); if (type && type->has_nand_tree_disable)
Do not disable broadcast if we are using address 0 (broadcast) to communicate with this device. Otherwise we will use proper driver but no communication will be possible and no link changes will be detected. There are two scenarios where we can run in to this situation: - PHY is bootstrapped for address 0 - no PHY address is known and linux is scanning the MDIO bus, so first respond and attached device will be on address 0. The fixes tag points to the latest refactoring, not to the initial point where kszphy_broadcast_disable() was introduced. Fixes: 79e498a9c7da0 ("net: phy: micrel: Restore led_mode and clk_sel on resume") Cc: stable@vger.kernel.org Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/micrel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)