netdev - Re: [PATCH net-next v4 3/3] net: dsa: realtek: support reset controller

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: <s4r5fdkrci2thuv4o6pstdxwkh7vfsd3w63rlcke3qqz6pdqne@t5jvkqwpgy7r>
Date: Tue, 20 Feb 2024 13:30:33 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: Linus Walleij <linus.walleij@...aro.org>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v4 3/3] net: dsa: realtek: support reset
 controller

On Tue, Feb 20, 2024 at 09:22:44AM -0300, Luiz Angelo Daros de Luca wrote:
> Hi Alvin,
> 
> > On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote:
> > > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = reset_control_assert(priv->reset_ctl);
> > > +     if (!ret)
> > > +             return;
> >
> > If priv->reset_ctl is NULL - i.e. if no DT property is specified - then
> > this will always return early and the GPIO will not be asserted.
> 
> I made a mistake. I should be
> 
> if (ret) {
>           dev_warn...
> }
> 
> not returning on error (as you suggested below).
> 
> I was sure I was doing just that... I was surprised to see it as it
> is.  I'll recheck my branch with all the integrated changes. It passed
> my tests as when reset is missed, it normally does not matter. Thanks
> for the catch.
> 
> >
> > > +
> > > +     dev_warn(priv->dev,
> > > +              "Failed to assert the switch reset control: %pe\n",
> > > +              ERR_PTR(ret));
> >
> > You only log an error if the reset controller assert fails, but not if
> > the GPIO assert fails. Why the unequal treatment?
> 
> Because it does not return a value. There is no way to tell if it failed.

Ah ok, nevermind that part then.

BTW, please use gpiod_set_value_cansleep(). With that I think this is good.