[PATCH net] net: mdio: mdio-bitbang: Fix C45 read/write protocol

[PATCH net] net: mdio: mdio-bitbang: Fix C45 read/write protocol

[Serge Semin]

Based on the original code semantic in case of Clause 45 MDIO, the address
command is supposed to be followed by the command sending the MMD address,
not the CSR address. The commit 002dd3de097c ("net: mdio: mdio-bitbang:
Separate C22 and C45 transactions") has erroneously broken that. So most
likely due to an unfortunate variable name it switched the code to sending
the CSR address. In our case it caused the protocol malfunction so the
read operation always failed with the turnaround bit always been driven to
one by PHY instead of zero. Fix that by getting back the correct
behaviour: sending MMD address command right after the regular address
command.

Fixes: 002dd3de097c ("net: mdio: mdio-bitbang: Separate C22 and C45 transactions")
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/mdio/mdio-bitbang.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/mdio-bitbang.c b/drivers/net/mdio/mdio-bitbang.c
index b83932562be2..81b7748c10ce 100644
--- a/drivers/net/mdio/mdio-bitbang.c
+++ b/drivers/net/mdio/mdio-bitbang.c
@@ -186,7 +186,7 @@ int mdiobb_read_c45(struct mii_bus *bus, int phy, int devad, int reg)
 	struct mdiobb_ctrl *ctrl = bus->priv;
 
 	mdiobb_cmd_addr(ctrl, phy, devad, reg);
-	mdiobb_cmd(ctrl, MDIO_C45_READ, phy, reg);
+	mdiobb_cmd(ctrl, MDIO_C45_READ, phy, devad);
 
 	return mdiobb_read_common(bus, phy);
 }
@@ -222,7 +222,7 @@ int mdiobb_write_c45(struct mii_bus *bus, int phy, int devad, int reg, u16 val)
 	struct mdiobb_ctrl *ctrl = bus->priv;
 
 	mdiobb_cmd_addr(ctrl, phy, devad, reg);
-	mdiobb_cmd(ctrl, MDIO_C45_WRITE, phy, reg);
+	mdiobb_cmd(ctrl, MDIO_C45_WRITE, phy, devad);
 
 	return mdiobb_write_common(bus, val);
 }
-- 
2.41.0

Re: [PATCH net] net: mdio: mdio-bitbang: Fix C45 read/write protocol

[Andrew Lunn]

On Wed, Aug 16, 2023 at 09:06:52PM +0300, Serge Semin wrote:
> Based on the original code semantic in case of Clause 45 MDIO, the address
> command is supposed to be followed by the command sending the MMD address,
> not the CSR address. The commit 002dd3de097c ("net: mdio: mdio-bitbang:
> Separate C22 and C45 transactions") has erroneously broken that. So most
> likely due to an unfortunate variable name it switched the code to sending
> the CSR address. In our case it caused the protocol malfunction so the
> read operation always failed with the turnaround bit always been driven to
> one by PHY instead of zero. Fix that by getting back the correct
> behaviour: sending MMD address command right after the regular address
> command.

I'm actually surprised we have not got more reports of me breaking
MDIO with this patchset. I did test bit-banging, but only for C22.

> Fixes: 002dd3de097c ("net: mdio: mdio-bitbang: Separate C22 and C45 transactions")
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net] net: mdio: mdio-bitbang: Fix C45 read/write protocol

[Serge Semin]

On Wed, Aug 16, 2023 at 09:39:37PM +0200, Andrew Lunn wrote:
> On Wed, Aug 16, 2023 at 09:06:52PM +0300, Serge Semin wrote:
> > Based on the original code semantic in case of Clause 45 MDIO, the address
> > command is supposed to be followed by the command sending the MMD address,
> > not the CSR address. The commit 002dd3de097c ("net: mdio: mdio-bitbang:
> > Separate C22 and C45 transactions") has erroneously broken that. So most
> > likely due to an unfortunate variable name it switched the code to sending
> > the CSR address. In our case it caused the protocol malfunction so the
> > read operation always failed with the turnaround bit always been driven to
> > one by PHY instead of zero. Fix that by getting back the correct
> > behaviour: sending MMD address command right after the regular address
> > command.
> 

> I'm actually surprised we have not got more reports of me breaking
> MDIO with this patchset. I did test bit-banging, but only for C22.

Yeah, I was also surprised not to find any fixes or reports for more
than a half of year in the mailing list or in the commits log. I was
sitting on 6.4-rc1 when the problem was discovered. Seeing the problem
was easily reproducible I decided to get to the latest kernel before
get to debugging it. It didn't help. The problem remained in 6.5-rc4
too.

> 
> > Fixes: 002dd3de097c ("net: mdio: mdio-bitbang: Separate C22 and C45 transactions")
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks.

-Serge(y)

> 
>     Andrew

Re: [PATCH net] net: mdio: mdio-bitbang: Fix C45 read/write protocol

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 16 Aug 2023 21:06:52 +0300 you wrote:
> Based on the original code semantic in case of Clause 45 MDIO, the address
> command is supposed to be followed by the command sending the MMD address,
> not the CSR address. The commit 002dd3de097c ("net: mdio: mdio-bitbang:
> Separate C22 and C45 transactions") has erroneously broken that. So most
> likely due to an unfortunate variable name it switched the code to sending
> the CSR address. In our case it caused the protocol malfunction so the
> read operation always failed with the turnaround bit always been driven to
> one by PHY instead of zero. Fix that by getting back the correct
> behaviour: sending MMD address command right after the regular address
> command.
> 
> [...]

Here is the summary with links:
  - [net] net: mdio: mdio-bitbang: Fix C45 read/write protocol
    https://git.kernel.org/netdev/net/c/2572ce62415c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html