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

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/22/23 14:58, 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.
> 
> If Arun wants to send V2 to fix the actual bug, fine by me.

Seems like we are getting a masterclass in how not communicate, be 
perceived to be rude from contributors, and also lacking the persistence 
to be expected from a contributor, just what we need. Very encouraging.
-- 
Florian