netdev - Re: [PATCH net-next 4/7] net: dsa: realtek: create realtek-common

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: <CAJq09z4YtJPnpLb3OqYaGdiPU3zPO636tu=jG08a=ROD0A=dRQ@mail.gmail.com>
Date: Mon, 11 Dec 2023 02:02:30 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "andrew@...n.ch" <andrew@...n.ch>, 
	"f.fainelli@...il.com" <f.fainelli@...il.com>, "olteanv@...il.com" <olteanv@...il.com>, 
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>, 
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>, 
	"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 4/7] net: dsa: realtek: create realtek-common

> > +struct realtek_priv *
> > +realtek_common_probe_pre(struct device *dev, struct regmap_config rc,
> > +                      struct regmap_config rc_nolock)
> > +{
>
> <snip>
>
> > +
> > +     /* TODO: if power is software controlled, set up any regulators here */
> > +
> > +     priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(priv->reset)) {
> > +             dev_err(dev, "failed to get RESET GPIO\n");
> > +             return ERR_CAST(priv->reset);
> > +     }
> > +     if (priv->reset) {
> > +             gpiod_set_value(priv->reset, 1);
> > +             dev_dbg(dev, "asserted RESET\n");
> > +             msleep(REALTEK_HW_STOP_DELAY);
> > +             gpiod_set_value(priv->reset, 0);
> > +             msleep(REALTEK_HW_START_DELAY);
> > +             dev_dbg(dev, "deasserted RESET\n");
> > +     }
>
> Another thing I would like to suggest is that you do not move the
> hardware reset and the /* TODO: regulators */ into the common code. I
> actually wanted to add regulator support for rtl8365mb after you are
> finished with your series, and I noticed that it will not fit well here,
> because the supplies are different for the two switch variants.
>
> If we were to do the hardware reset here in common_probe_pre(), where
> should I put my variant-specific regulator_bulk_enable()? I can't put it
> before _pre() because I do not have the private data allocated yet. If I
> put it afterwards, then the above hardware reset toggle will have had no
> effect.

We would need to move the HW reset out of common_probe_pre(). Putting
it in _post() or between _pre() and _post() would not solve your case
as that happens in interface context. The probe is currently
interface-specific, not variant-specific. Maybe the easiest solution
would be to move the reset into the detect(), just before getting the
chip id, creating a new realtek_common_hwreset(). That way, you could
set up the regulators a little bit before the reset in the variant
context.

We could also change the interface-specific to a variant-specific
probe like this:

rtl8365mb_probe_smi(){
       priv = realtek_common_probe() /* previously the _pre func */
       realtek_smi_probe(priv) /* everything but the common calls */
       rtl8365mb_setup_regulators(priv)
       realtek_common_hwreset(priv) /* the reset code from common_probe_pre */
       rtl8365mb_detect(priv)
       realtek_common_register(priv) /* previously the
common_probe_post without the detect */
}

rtl8365mb_probe_mdio(){
    <repeat rtl8365rb_probe_smi but replace realtek_smi_probe with
realtek_mdio_probe>
}

rtl8366rb_probe_smi() { ... }
rtl8366rb_probe_mdio() { ... }

But it would be mostly 4 times the same code above, with lots of extra checks.

For the sake of keeping this patch as small as possible, I would
prefer to maintain the reset in its current location unless it is a
merging requirement. You can easily move it out when necessary. I
don't believe preparing for a potential future change fits in this
series, as we may misjudge what will be needed to set up the
regulators.

Regards,

Luiz