From: Marek Vasut <[email protected]>
The blamed commit has replaced a ksz_write8() call to address
REG_PORT_5_CTRL_6 (0x56) with a ksz_set_xmii() -> ksz_pwrite8() call to
regs[P_XMII_CTRL_1], which is also defined as 0x56 for ksz8795_regs[].
The trouble is that, when compared to ksz_write8(), ksz_pwrite8() also
adjusts the register offset with the port base address. So in reality,
ksz_pwrite8(offset=0x56) accesses register 0x56 + 0x50 = 0xa6, which in
this switch appears to be unmapped, and the RGMII delay configuration on
the CPU port does nothing.
So if the switch wasn't fine with the RGMII delay configuration done
through pin strapping and relied on Linux to apply a different one in
order to pass traffic, this is now broken.
Using the offset translation logic imposed by ksz_pwrite8(), the correct
value for regs[P_XMII_CTRL_1] should have been 0x6 on ksz8795_regs[], in
order to really end up accessing register 0x56.
Static code analysis shows that, despite there being multiple other
accesses to regs[P_XMII_CTRL_1] in this driver, the only code path that
is applicable to ksz8795_regs[] and ksz8_dev_ops is ksz_set_xmii().
Therefore, the problem is isolated to RGMII delays.
In its current form, ksz8795_regs[] contains the same value for
P_XMII_CTRL_0 and for P_XMII_CTRL_1, and this raises valid suspicions
that writes made by the driver to regs[P_XMII_CTRL_0] might overwrite
writes made to regs[P_XMII_CTRL_1] or vice versa.
Again, static analysis shows that the only accesses to P_XMII_CTRL_0
from the driver are made from code paths which are not reachable with
ksz8_dev_ops. So the accesses made by ksz_set_xmii() are safe for this
switch family.
[ vladimiroltean: rewrote commit message ]
Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function")
Signed-off-by: Marek Vasut <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
Acked-by: Arun Ramadoss <[email protected]>
---
v1->v2: rewrite commit message
v1 at:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
drivers/net/dsa/microchip/ksz_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 5a7ce2aede68..50fd548c72d8 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -315,7 +315,7 @@ static const u16 ksz8795_regs[] = {
[S_BROADCAST_CTRL] = 0x06,
[S_MULTICAST_CTRL] = 0x04,
[P_XMII_CTRL_0] = 0x06,
- [P_XMII_CTRL_1] = 0x56,
+ [P_XMII_CTRL_1] = 0x06,
};
static const u32 ksz8795_masks[] = {
--
2.34.1
On 3/15/23 16:19, Vladimir Oltean wrote:
> From: Marek Vasut <[email protected]>
>
> The blamed commit has replaced a ksz_write8() call to address
> REG_PORT_5_CTRL_6 (0x56) with a ksz_set_xmii() -> ksz_pwrite8() call to
> regs[P_XMII_CTRL_1], which is also defined as 0x56 for ksz8795_regs[].
>
> The trouble is that, when compared to ksz_write8(), ksz_pwrite8() also
> adjusts the register offset with the port base address. So in reality,
> ksz_pwrite8(offset=0x56) accesses register 0x56 + 0x50 = 0xa6, which in
> this switch appears to be unmapped, and the RGMII delay configuration on
> the CPU port does nothing.
>
> So if the switch wasn't fine with the RGMII delay configuration done
> through pin strapping and relied on Linux to apply a different one in
> order to pass traffic, this is now broken.
>
> Using the offset translation logic imposed by ksz_pwrite8(), the correct
> value for regs[P_XMII_CTRL_1] should have been 0x6 on ksz8795_regs[], in
> order to really end up accessing register 0x56.
>
> Static code analysis shows that, despite there being multiple other
> accesses to regs[P_XMII_CTRL_1] in this driver, the only code path that
> is applicable to ksz8795_regs[] and ksz8_dev_ops is ksz_set_xmii().
> Therefore, the problem is isolated to RGMII delays.
>
> In its current form, ksz8795_regs[] contains the same value for
> P_XMII_CTRL_0 and for P_XMII_CTRL_1, and this raises valid suspicions
> that writes made by the driver to regs[P_XMII_CTRL_0] might overwrite
> writes made to regs[P_XMII_CTRL_1] or vice versa.
>
> Again, static analysis shows that the only accesses to P_XMII_CTRL_0
> from the driver are made from code paths which are not reachable with
> ksz8_dev_ops. So the accesses made by ksz_set_xmii() are safe for this
> switch family.
>
> [ vladimiroltean: rewrote commit message ]
>
> Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function")
> Signed-off-by: Marek Vasut <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Acked-by: Arun Ramadoss <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
On Thu, 16 Mar 2023 01:19:16 +0200 you wrote:
> From: Marek Vasut <[email protected]>
>
> The blamed commit has replaced a ksz_write8() call to address
> REG_PORT_5_CTRL_6 (0x56) with a ksz_set_xmii() -> ksz_pwrite8() call to
> regs[P_XMII_CTRL_1], which is also defined as 0x56 for ksz8795_regs[].
>
> The trouble is that, when compared to ksz_write8(), ksz_pwrite8() also
> adjusts the register offset with the port base address. So in reality,
> ksz_pwrite8(offset=0x56) accesses register 0x56 + 0x50 = 0xa6, which in
> this switch appears to be unmapped, and the RGMII delay configuration on
> the CPU port does nothing.
>
> [...]
Here is the summary with links:
- [v2,net] net: dsa: microchip: fix RGMII delay configuration on KSZ8765/KSZ8794/KSZ8795
https://git.kernel.org/netdev/net/c/5ae06327a3a5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html