netdev - Re: [PATCH net-next v2 3/7] net: dsa: realtek: common realtek-dsa module

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: <CAJq09z5zupqd-p_rD4-WfpAhYi-Ny6QeR4PD7LbUV=UbnX_gBA@mail.gmail.com>
Date: Thu, 21 Dec 2023 00:25:10 -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 v2 3/7] net: dsa: realtek: common realtek-dsa module

> > > +   /* 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");
> > > +   }
> >
> > I simply cannot understand why you insist on moving this part despite
> > our earlier discussion on it where I pointed out that it makes no
> > sense to move it. Is chip hardware reset not the discipline of the chip
> > variant driver?
> >
> > Why don't you just keep it in its original place? It will make your
> > patch smaller, which is what you seemed to care about the last time I
> > raised this.
> >
> > Sorry, but I cannot give my Reviewed-by on this patch with this part
> > moved around.
>
> Well, to be fair, your original goal was to add support for reset
> controllers. And Vladimir pointed out that you end up with a lot of
> duplicated code when doing that. And he has a point.

Yes, it ended up bringing many more issues, like the driver dependency
direction, duplicated compatibles, and so on. Even issues with the OF
MDIO API and other drivers. However, at some point, we need to stop
adding stuff to the series or I might start to sign-of-by as Sisyphus.

And I still want to take a look at the LED code, which is not working at all.

> So on reflection, I think this part is OK for now. Then you can then get
> unblocked and put your reset controller patch on top when you get around
> to it, without the code duplication. I think code can be adapted to
> support regulators without too much churn. I'm sorry for the overly
> negative feedback.

It's OK. You are just reviewing the code considering what you want to
do on top of it but we need to focus on what makes sense as it is now.

Your reviews are always fruitful. Thank you, Alvin.

> But please have a look at my comments in patch 5, since they apply more
> broadly to your series.

I already did. :-)

> Kind regards,
> Alvin