linux-kernel - Re: [PATCH v5] net: mdiobus: Add a function to deassert reset

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: <KL1PR01MB54487B1E3759CF451F06D5D8E6759@KL1PR01MB5448.apcprd01.prod.exchangelabs.com>
Date:   Fri, 12 May 2023 17:50:03 +0800
From:   Yan Wang <rk.code@...look.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] net: mdiobus: Add a function to deassert reset



On 5/12/2023 5:41 PM, Russell King (Oracle) wrote:
> On Fri, May 12, 2023 at 05:28:47PM +0800, Yan Wang wrote:
>>
>> On 5/12/2023 5:02 PM, Russell King (Oracle) wrote:
>>> On Fri, May 12, 2023 at 03:08:53PM +0800, Yan Wang wrote:
>>>> +	gpiod_set_value_cansleep(reset, gpiod_is_active_low(reset));
>>>> +	fsleep(reset_assert_delay);
>>>> +	gpiod_set_value_cansleep(reset, !gpiod_is_active_low(reset));
>>> Andrew, one of the phylib maintainers and thus is responsible for code
>>> in the area you are touching. Andrew has complained about the above
>>> which asserts and then deasserts reset on two occasions now, explained
>>> why it is wrong, but still the code persists in doing this.
>>>
>>> I am going to add my voice as another phylib maintainer to this and say
>>> NO to this code, for the exact same reasons that Andrew has given.
>>>
>>> You now have two people responsible for the code in question telling
>>> you that this is the wrong approach.
>>>
>>> Until this is addressed in some way, it is pointless you posting
>>> another version of this patch.
>>>
>>> Thanks.
>>>
>> I'm very sorry, I didn't have their previous intention.
>> The meaning of the two assertions is reset and reset release.
>> If you believe this is the wrong method, please ignore it.
> As Andrew has told you twice:
>
> We do not want to be resetting the PHY while we are probing the bus,
> and he has given one reason for it.
>
> The reason Andrew gave is that hardware resetting a PHY that was not
> already in reset means that any link is immediately terminated, and
> the PHY has to renegotiate with its link partner when your code
> subsequently releases the reset signal. This is *not* the behaviour
> that phylib maintainers want to see.
>
> The second problem that Andrew didn't mention is that always hardware
> resetting the PHY will clear out any firmware setup that has happened
> before the kernel has been booted. Again, that's a no-no.
>
> The final issue I have is that your patch is described as "add a
> function do *DEASSERT* reset" not "add a function to *ALWAYS* *RESET*"
> which is what you are actually doing here. So the commit message and
> the code disagree with what's going on - the summary line is at best
> misleading.
>
> If your hardware case is that the PHY is already in reset, then of
> course you don't see any of the above as a problem, but that is not
> universally true - and that is exactly why Andrew is bringing this
> up. There are platforms out there where the reset is described in
> the firmware hardware description, *but* when the kernel boots, the
> reset signal is already deasserted. Raising it during kernel boot as
> you are doing will terminate the PHY's link with the remote end,
> and then deasserting it will cause it to renegotiate.
>
> Thanks.
>

Thank you very much for your explanation. I apologize to Andrew
and I will handle it on my bootloader

Best Regards