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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Feb 22, 2023 at 11:58:23PM +0100, Marek Vasut wrote:
> On 2/22/23 23:31, Vladimir Oltean wrote:
> > 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.
> 
> Sorry, not like this and not with this feedback tone.

I gave you reasonable technical feedback. I spent a lot of time working
that out.

However, your responses were consistently difficult, but I kept my
cool. I completely agree with Vladimir's sentiments.

Had you come straight out and asked for help writing the commit
message, then I might have been more receptive, but to "play dumb" as
you seemed to do, only to then eventually ask for me to write the
commit message for you... sorry, but no, you don't get that free
cookie anymore and that ship sailed when you decided to be difficult.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!