netdev - Re: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset

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: <13c38696-25ed-418d-b85e-950736384a0b@lunn.ch>
Date: Mon, 29 Apr 2024 01:17:10 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@...roamp.se>
Cc: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, saeedm@...dia.com,
	anthony.l.nguyen@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, corbet@....net,
	linux-doc@...r.kernel.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	devicetree@...r.kernel.org, horatiu.vultur@...rochip.com,
	ruanjinjie@...wei.com, steen.hegelund@...rochip.com,
	vladimir.oltean@....com, UNGLinuxDriver@...rochip.com,
	Thorsten.Kummermehr@...rochip.com, Pier.Beruto@...emi.com,
	Selvamani.Rajagopal@...emi.com, Nicolas.Ferre@...rochip.com,
	benjamin.bigler@...nformulastudent.ch
Subject: Re: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset

On Sun, Apr 28, 2024 at 11:16:32PM +0200, Ramón Nordin Rodriguez wrote:
> >From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?=
>  <ramon.nordin.rodriguez@...roamp.se>
> Date: Sun, 28 Apr 2024 22:25:12 +0200
> Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit

You sent this patch in an odd way. We don't normally see headers like
this. I've been using b4 recently for patch management:

https://b4.docs.kernel.org/en/latest/contributor/prep.html

Using `b4 send` is a good idea. Otherwise git format-patch; git send-email

> index 9abefa8b9d9f..bed9033574b2 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -9,6 +9,7 @@
>  #include <linux/kernel.h>
>  #include <linux/phy.h>
>  #include <linux/oa_tc6.h>
> +#include <linux/gpio/driver.h>

This is not a gpio driver, it is a gpio consumer. So you should be
using linux/gpio/consumer.h. Also, i _think_ the includes are sorted,
so it probably should go earlier.

>  
>  #define DRV_NAME			"lan865x"
>  
> @@ -33,6 +34,7 @@
>  
>  struct lan865x_priv {
>  	struct work_struct multicast_work;
> +	struct gpio_desc *reset_gpio;
>  	struct net_device *netdev;
>  	struct spi_device *spi;
>  	struct oa_tc6 *tc6;
> @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv)
>  	return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
>  }
>  
> +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv)
> +{
> +	priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset_gpio))
> +		return PTR_ERR(priv->reset_gpio);
> +
> +	return 0;
> +}
> +
> +static void lan865x_hw_reset(struct lan865x_priv *priv)
> +{
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	// section 9.6.3 RESET_N Timing specifies a minimum hold of 5us
> +	usleep_range(5, 10);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +}

Do you see a need to do a reset at any time other than probe? If not,
i would probably combine these two functions into one. Also, since you
pass GPIOD_OUT_HIGH, you have already put it into reset. So setting
the gpio to 1 is pointless.

Does the datasheet say anything about how long you should wait after
releasing the reset?

> +
>  static int lan865x_probe(struct spi_device *spi)
>  {
>  	struct net_device *netdev;
> @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi)
>  	priv->netdev = netdev;
>  	priv->spi = spi;
>  	spi_set_drvdata(spi, priv);
> +	if (lan865x_probe_reset_gpio(priv)) {
> +		dev_err(&spi->dev, "failed to probe reset pin");
> +		ret = -ENODEV;

It is normal that a function like lan865x_probe_reset_gpio() would
return an error code. You should then return that error code, rather
than replace it with ENODEV.

     Andrew