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: <20230222223141.ozeis33beq5wpkfy@skbuf>
Date:   Thu, 23 Feb 2023 00:31:41 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Marek Vasut <marex@...x.de>
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 Wed, Feb 22, 2023 at 11:05:10PM +0100, Marek Vasut wrote:
> On 2/22/23 22:08, Vladimir Oltean wrote:
> > Please summarize in the commit title what is the user-visible impact of
> > the problem that is being fixed. Short and to the point.
> 
> Can you suggest a Subject which is acceptable ?

Nope. The thing is, I don't know what you're seeing, only you do. I can
only review and comment if it's plausible or not. I'm sure you can come
up with something.

> > > Currently, the driver uses PORT read function on register P_XMII_CTRL_1
> > > to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit.
> > 
> > Provably false. The driver does do that, but not for KSZ87xx.
> 
> The driver uses port read function with register value 0x56 instead of 0x06
> , which means the remapping happens twice, which provably breaks the driver
> since commit Fixes below .

The sentence is false in the context of ksz87xx, which is what is the
implied context of this patch (see commit title written by yourself).
The P_GMII_1GBIT_M field is not accessed, and that is a bug in itself.
Also, the (lack of) access to the P_GMII_1GBIT_M field is not what
causes the breakage that you see, but to other fields from that register.

> > There is no call site other than ksz_set_xmii(). Please delete false
> > information from the commit message.
> 
> $ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/
> drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x06,
> drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1] = 0x0301,
> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
> drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
> drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
> drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
> drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1,
> 
> I count 6.

So your response to 2 reviewers wasting their time to do a detailed
analysis of the code paths that apply to the KSZ87xx model in particular,
to tell you precisely why your commit message is incorrect is "git grep"?

> OK, to make this simple, can you write a commit message which you consider
> acceptable, to close this discussion ?

Nope. The thing is, I'm sure you can, too. Maybe you need to take a
break and think about this some more.