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

[Vladimir Oltean <olteanv@gmail.com>]

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.