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: <o3c4fxt5xtabsgdg6oz4qyy6rvc7l5qojl65hvd2x2zxyz34bn@7vjv6obs6rgk>
Date: Wed, 20 Dec 2023 15:50:55 +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 v2 3/7] net: dsa: realtek: common realtek-dsa
 module

On Wed, Dec 20, 2023 at 10:40:25AM +0000, Alvin Šipraga wrote:
> On Wed, Dec 20, 2023 at 01:24:26AM -0300, Luiz Angelo Daros de Luca wrote:
> > +	/* 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.

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.

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

Kind regards,
Alvin