diff mbox series

[v2,net] net: dsa: microchip: fix RGMII delay configuration on KSZ8765/KSZ8794/KSZ8795

Message ID 20230315231916.2998480-1-vladimir.oltean@nxp.com (mailing list archive)
State Accepted
Commit 5ae06327a3a5bad4ee246d81df203b1b00a7b390
Delegated to: Netdev Maintainers
Headers show
Series [v2,net] net: dsa: microchip: fix RGMII delay configuration on KSZ8765/KSZ8794/KSZ8795 | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vladimir Oltean March 15, 2023, 11:19 p.m. UTC 
From: Marek Vasut <marex@denx.de>

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 <marex@denx.de>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
v1->v2: rewrite commit message

v1 at:
https://patchwork.kernel.org/project/netdevbpf/patch/20230222031738.189025-1-marex@denx.de/

 drivers/net/dsa/microchip/ksz_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Florian Fainelli March 15, 2023, 11:23 p.m. UTC | #1 
On 3/15/23 16:19, Vladimir Oltean wrote:
> From: Marek Vasut <marex@denx.de>
> 
> 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 <marex@denx.de>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
patchwork-bot+netdevbpf@kernel.org March 17, 2023, 4:30 a.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 16 Mar 2023 01:19:16 +0200 you wrote:
> From: Marek Vasut <marex@denx.de>
> 
> 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!
diff mbox series

Patch

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[] = {