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: <ZF4J1VqEqbnE6JG9@shell.armlinux.org.uk>
Date:   Fri, 12 May 2023 10:41:41 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Yan Wang <rk.code@...look.com>
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 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.

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