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: <CAJq09z5ak_S3iFnGw+rw0JQwoxf=x69=Ync3Xg5AQ0hx_tsGXg@mail.gmail.com>
Date: Tue, 20 Feb 2024 09:22:44 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
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

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.

>
> I suggest keeping it simple:
>
> void rtl83xx_reset_assert(struct realtek_priv *priv)
> {
>   int ret;
>
>   ret = reset_control_assert(priv->reset_ctl);
>   if (ret)
>     dev_warn(priv->dev, "failed to assert reset control: %d\n", ret);
>
>   ret = gpiod_set_value(priv->reset, false);
>   if (ret)
>     dev_warn(priv->dev, "failed to assert reset GPIO: %d\n", ret);
> }
>
> or even drop the warnings altogether.
>
> > +
> > +     gpiod_set_value(priv->reset, true);
> > +}
> > +
> > +void rtl83xx_reset_deassert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     ret = reset_control_deassert(priv->reset_ctl);
> > +     if (!ret)
> > +             return;
> > +
> > +     dev_warn(priv->dev,
> > +              "Failed to deassert the switch reset control: %pe\n",
> > +              ERR_PTR(ret));
> > +
> > +     gpiod_set_value(priv->reset, false);
> > +}
>
> Same comments apply to this function. Just deassert both.
>
> > +
> >  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> >  MODULE_AUTHOR("Linus Walleij <linus.walleij@...aro.org>");
> >  MODULE_DESCRIPTION("Realtek DSA switches common module");
> > diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> > index 0ddff33df6b0..c8a0ff8fd75e 100644
> > --- a/drivers/net/dsa/realtek/rtl83xx.h
> > +++ b/drivers/net/dsa/realtek/rtl83xx.h
> > @@ -18,5 +18,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv);
> >  void rtl83xx_unregister_switch(struct realtek_priv *priv);
> >  void rtl83xx_shutdown(struct realtek_priv *priv);
> >  void rtl83xx_remove(struct realtek_priv *priv);
> > +void rtl83xx_reset_assert(struct realtek_priv *priv);
> > +void rtl83xx_reset_deassert(struct realtek_priv *priv);
> >
> >  #endif /* _RTL83XX_H */
> >
> > --
> > 2.43.0
> >

Regards,

Luiz

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ