linux-kernel - 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]

Date: Tue, 20 Feb 2024 10:26:45 +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 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.

> +
> +	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?

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
>