netdev - Re: [PATCH] net: dsa: lan9303: fix reset on probe

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: <Yga4wD+pkn6B45Iz@lunn.ch>
Date:   Fri, 11 Feb 2022 20:28:00 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Måns Rullgård <mans@...sr.com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Vladimir Oltean <olteanv@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Juergen Borleis <kernel@...gutronix.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: dsa: lan9303: fix reset on probe

On Wed, Feb 09, 2022 at 04:34:15PM +0000, Måns Rullgård wrote:
> Andrew Lunn <andrew@...n.ch> writes:
> 
> > On Wed, Feb 09, 2022 at 02:54:54PM +0000, Mans Rullgard wrote:
> >> The reset input to the LAN9303 chip is active low, and devicetree
> >> gpio handles reflect this.  Therefore, the gpio should be requested
> >> with an initial state of high in order for the reset signal to be
> >> asserted.  Other uses of the gpio already use the correct polarity.
> >> 
> >> Signed-off-by: Mans Rullgard <mans@...sr.com>
> >> ---
> >>  drivers/net/dsa/lan9303-core.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> >> index aa1142d6a9f5..2de67708bbd2 100644
> >> --- a/drivers/net/dsa/lan9303-core.c
> >> +++ b/drivers/net/dsa/lan9303-core.c
> >> @@ -1301,7 +1301,7 @@ static int lan9303_probe_reset_gpio(struct lan9303 *chip,
> >>  				     struct device_node *np)
> >>  {
> >>  	chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "reset",
> >> -						   GPIOD_OUT_LOW);
> >> +						   GPIOD_OUT_HIGH);
> >>  	if (IS_ERR(chip->reset_gpio))
> >>  		return PTR_ERR(chip->reset_gpio);
> >
> > lan9303_handle_reset() does a sleep and then releases the reset. I
> > don't see anywhere in the driver which asserts the reset first. So is
> > it actually asserted as part of this getting the GPIO? And if so, does
> > not this change actually break the reset?
> 
> The GPIOD_OUT_xxx flags to gpiod_get() request that the pin be
> configured as output and set to high/low initially.  The GPIOD_OUT_LOW
> currently used by the lan9303 driver together with GPIO_ACTIVE_LOW in
> the devicetrees results in the actual voltage being set high.  The
> driver then sleeps for a bit before setting the gpio value to zero,
> again translated to a high output voltage.  That is, the value set after
> the sleep is the same as it was initially.  This is obviously not the
> intent.

Yes, i agree. I'm just wondering how this worked for whoever
implemented this code. I guess it never actually did a reset, or the
bootloader left the reset already in the asserted state, so that the
gpiod_get() actual deasserted the reset?

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew