netdev - Re: DSA mv88e6xxx

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]

Date:   Thu, 2 Feb 2023 17:57:45 +0000
From:   "Valek, Andrej" <andrej.valek@...mens.com>
To:     "andrew@...n.ch" <andrew@...n.ch>
CC:     "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: DSA mv88e6xxx_probe

Thank you for the explanation, but I have some additional questions...
.

On Thu, 2023-02-02 at 17:05 +0100, Andrew Lunn wrote:
> On Thu, Feb 02, 2023 at 03:18:37PM +0000, Valek, Andrej wrote:
> > Hello everyone!
> > 
> > I have a switch mv88e6085 which is connected via MDIO bus to iMX.8
> > SoC.
> > 
> > Switch is not being detected during booting because the address is
> > different (due to uninitialized PINs from DTB). The problem is,
> > that
> > switch has to be reset during boot phase, but it isn't.
> > 
> > So I would like to ask you maybe a generic question about
> > devm_gpiod_get_optional function inside mv88e6xxx_probe.
> > 
> > Is this "chip->reset = devm_gpiod_get_optional(dev, "reset",
> > GPIOD_OUT_LOW);" line really do the reset? Because from the lines
> > below
> > looks like, but the reset pulse hasn't been made. Measured with
> > scope.
> > 
> > > chip->reset = devm_gpiod_get_optional(dev, "reset",
> > > GPIOD_OUT_LOW);
> > > if (IS_ERR(chip->reset))
> > >         goto out;
> > > 
> > > if (chip->reset)
> > >         usleep_range(1000, 2000);
> > 
> > So it should wait, but for what?
> 
> The current code is designed to take a switch held in reset out of
> reset. It does not perform an actual reset.
> 
How does it then work? I see just a "devm_gpiod_get_optional" which
just assign an pointer to "chip->reset" and then
" if (chip->reset) usleep_range(1000, 2000);" which just waits for
"something" ? Where is the "reset" took out? I don't see any gpio set
to 0.
> If you need a real reset, you probably need to call
> mv88e6xxx_hardware_reset(chip), not usleep().
> 
> However, a reset can be a slow operation, specially if the EEPROM is
> full of stuff. So we want to avoid two resets if possible.
> 
> The MDIO bus itself has DT descriptions for a GPIO reset. See
> Documentation/devicetree/bindings/net/mdio.yaml
This looks promising. So I have to just move the "reset-gpios" DTB
entry from switch to mdio section. But which driver handles it,
drivers/net/phy/mdio_bus.c,  or?
> mdio {
> 	#address-cells = <1>;
> 	#size-cells = 0>;
while here is no compatible part... .
> 
> You might be able to use this to perform the power on reset of the
> switch. That advantage of that is it won't slow down the probe of
> everybody elses switches which have correct pin strapping.
> 
>         Andrew

Regards,
Andrej