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: <d865c56e-5ad3-71df-d6a9-03f09859347e@gmail.com>
Date:   Wed, 22 Feb 2023 15:05:35 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Marek Vasut <marex@...x.de>, 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>,
        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/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