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]

Date: Mon, 11 Dec 2023 09:24:35 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
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

On Mon, Dec 11, 2023 at 02:02:30AM -0300, Luiz Angelo Daros de Luca wrote:
> > > +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.

Yes, it is exhausting with this back and forth. Please just address what
you are willing to address and I will review again. Thanks.

> 
> Regards,
> 
> Luiz