netdev - Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function for KSZ87xx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <35a4df8a-7178-20de-f433-e2c01e5eaaf7@denx.de>
Date:   Thu, 23 Feb 2023 00:55:08 +0100
From:   Marek Vasut <marex@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        UNGLinuxDriver@...rochip.com,
        Woojung Huh <woojung.huh@...rochip.com>, stable@...r.kernel.org
Subject: Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function for
 KSZ87xx

On 2/23/23 00:21, Vladimir Oltean wrote:

[...]

, and I really have nothing else to base my judgement
> on, than your hint that there is a bug there, and the code. But the
> driver might behave in much more subtle ways which I may be completely
> missing, and I may think that I'm fixing something when I'm not. I have
> no way to know that except by booting a board, which I do not have (but
> you do).

The old code, removed in:
c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function")
used ksz_write8() (this part is important):
ksz_write8(dev, REG_PORT_5_CTRL_6, data8);
where:
drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6 
        0x56

The new code, where the relevant part is added in (see Fixes tag)
46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get 
function")
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = {
+       [P_XMII_CTRL_1]                 = 0x56,
uses ksz_pwrite8() (with p in the function name, p means PORT):
ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
which per drivers/net/dsa/microchip/ksz_common.h translates to
ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data);
and that dev->dev_ops->get_port_addr(port, offset) remapping function is 
per drivers/net/dsa/microchip/ksz8795.c really call to the following macro:
PORT_CTRL_ADDR(port, offset)
which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes
#define PORT_CTRL_ADDR(port, addr)
((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - 
REG_PORT_1_CTRL_0))

That means:
ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
writes register 0xa6 instead of register 0x56, because it calls the 
PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call 
PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, 
the value 0x56 is already remapped .

All the call-sites which do
ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
or
ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8)
are affected by this, all six, that means the ksz_[gs]et_xmii() and the 
ksz_[gs]et_gbit().

...

If all that should be changed in the commit message is "to access the 
P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the 
"ksz_set_xmii()" function instead, then just say so.

[...]