[PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

[PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

[Oleksij Rempel]

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(-)

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)
-- 
2.39.2

Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

[Andrew Lunn]

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

Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

[Oleksij Rempel]

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?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

[Jakub Kicinski]

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 ?