linux-kernel - Re: [PATCH] net: mdio: force deassert MDIO reset signal

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: <AM6PR08MB4376D91F2459E8610F0CCA59FFC09@AM6PR08MB4376.eurprd08.prod.outlook.com>
Date:   Sun, 15 Jan 2023 23:16:57 +0000
From:   Pierluigi Passaro <pierluigi.p@...iscite.com>
To:     Lars-Peter Clausen <lars@...afoo.de>
CC:     Andrew Lunn <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Eran Matityahu <eran.m@...iscite.com>,
        Nate Drude <Nate.D@...iscite.com>,
        Francesco Ferraro <francesco.f@...iscite.com>,
        Pierluigi Passaro <pierluigi.passaro@...il.com>
Subject: Re: [PATCH] net: mdio: force deassert MDIO reset signal

On Sun, Jan 15, 2023 at 11:56 PM Lars-Peter Clausen <lars@...afoo.de> wrote:
> On 1/15/23 14:33, Pierluigi Passaro wrote:
> > On Sun, Jan 15, 2023 at 10:59 PM Lars-Peter Clausen <lars@...afoo.de> wrote:
> >> On 1/15/23 09:08, Andrew Lunn wrote:
> >>> On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
> >>>> When the reset gpio is defined within the node of the device tree
> >>>> describing the PHY, the reset is initialized and managed only after
> >>>> calling the fwnode_mdiobus_phy_device_register function.
> >>>> However, before calling it, the MDIO communication is checked by the
> >>>> get_phy_device function.
> >>>> When this happen and the reset GPIO was somehow previously set down,
> >>>> the get_phy_device function fails, preventing the PHY detection.
> >>>> These changes force the deassert of the MDIO reset signal before
> >>>> checking the MDIO channel.
> >>>> The PHY may require a minimum deassert time before being responsive:
> >>>> use a reasonable sleep time after forcing the deassert of the MDIO
> >>>> reset signal.
> >>>> Once done, free the gpio descriptor to allow managing it later.
> >>> This has been discussed before. The problem is, it is not just a reset
> >>> GPIO. There could also be a clock which needs turning on, a regulator,
> >>> and/or a linux reset controller. And what order do you turn these on?
> >>>
> >>> The conclusions of the discussion is you assume the device cannot be
> >>> found by enumeration, and you put the ID in the compatible. That is
> >>> enough to get the driver to load, and the driver can then turn
> >>> everything on in the correct order, with the correct delays, etc.
> >> I've been running into this same problem again and again over the past
> >> years.
> >>
> >> Specifying the ID as part of the compatible string works for clause 22
> >> PHYs, but for clause 45 PHYs it does not work. The code always wants to
> >> read the ID from the PHY itself. But I do not understand things well
> >> enough to tell whether that's a fundamental restriction of C45 or just
> >> our implementation and the implementation can be changed to fix it.
> >>
> >> Do you have some thoughts on this?
> >>
> > IMHO, since the framework allows defining the reset GPIO, it does not sound
> > reasonable to manage it only after checking if the PHY can communicate:
> > if the reset is asserted, the PHY cannot communicate at all.
> > This patch just ensures that, if the reset GPIO is defined, it's not asserted
> > while checking the communication.
>
> I fully agree with you and I think this is the right approach, cause it
> is required to make systems work. But I've seen two attempts in the past
> that did the very same thing and they always got rejected. I can't find
> the patches anymore, but I think one was maybe 2 years ago.
>
Rejection is always a chance ;)
As long I can understand the reasons, I can at least try improving this patch.