linux-kernel - Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet

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] [day] [month] [year] [list]

Message-ID: <c486a797-6305-8a62-8604-aef5e835ae72@victronenergy.com>
Date:   Wed, 9 Oct 2019 10:09:40 +0000
From:   Jeroen Hofstee <jhofstee@...tronenergy.com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        Tony Lindgren <tony@...mide.com>
CC:     "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        Koen Kooi <koen@...inion.thruhere.net>,
        Benoît Cousson <bcousson@...libre.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ARM: dts: am335x-sancloud-bbe: Fix PHY mode for ethernet

Hello Grygorri,

On 10/9/19 11:43 AM, Grygorii Strashko wrote:
>
>
>>> Grygorii doesn't suggest to add a fixes tag, just to change the 
>>> referenced
>>> commit to another. Obviously I would like to understand why another 
>>> commit
>>> should be referenced. And then I should read and parse the response, 
>>> so there
>>> is no special reason, just time...
>>
>> OK sure. Well once you guys have the commit figured out, let me
>> know what to apply. And we know Grygorii is mostly right based
>> on his history of comments so best to not ignore that :)
>
> Sry, but I do not think my request is somehow special.
> Yes, your patch is correct by itself, but commit description is not:
> 1) commit cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for 
> RGMII mode")  which you've mentioned is A BUG
> and should not be merged first of all (which you can find out by 
> reading corresponding thread).
>
> just try checkout that commit and apply your patch on top - networking 
> should not work.
>
> But it was merged and not reverted - instead two more patches were 
> applied to fix regression.
>
> 2) Those commits are defined final behavior (which i again explained 
> above) and that new behavior hardly can
> be called "the bug in the at803x driver" as, unfortunately, there were 
> no common conclusion how default values for
> RX/TX delay should be handled vs phy-mode = "rgmii-txid"/"rgmii-rxid".
> Originally many PHY driver kept them default (as per boot 
> strapping/bootloader configuration), but now
> some driver (including at803x) started disabling RX delay if 
> "rgmii-txid" or TX delay if "rgmii-rxid".
>
> Hence, pls update commit message and add proper fixes tag. smth like:
> "Now after commit 6d4cd041f0af net: phy: at803x: disable delay only 
> for RGMII mode the driver will forcibly disable
> RGMII RX delay if phy-mode = "rgmii-txid" is specified in DT which 
> will break networking on ..
>
> Hence change .. to ensure ...
> Fixes: "
>
>

Yes, that part is clear to me, and I am not saying you are incorrect,
only that I would like to understand why above commit pops up
(since it makes no sense to me as well, my own commit). And the reason
is rather silly, I guess... I fixed it on master, thereafter checked the 
5.1 branch,
while keeping the fixed dtb... :(

Let me check that and I will send a v2. Likely tomorrow
(I am not near the device at the moment).

Regards,

Jeroen