Received-SPF: pass (google.com: domain of linux-wireless+bounces-2787-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <415a0e6e-5824-44a2-af2a-a75115d5a62e@bootlin.com>
Date: Tue, 30 Jan 2024 10:06:44 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] [v4] wifi: wilc1000: validate chip id during bus probe
To: David Mosberger-Tang <davidm@egauge.net>, linux-wireless@vger.kernel.org
Cc: Ajay.Kathat@microchip.com, kvalo@kernel.org
References: <20240127004331.1334804-1-davidm@egauge.net>
Content-Language: en-US
From: =?UTF-8?Q?Alexis_Lothor=C3=A9?= <alexis.lothore@bootlin.com>
In-Reply-To: <20240127004331.1334804-1-davidm@egauge.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 1/27/24 01:43, David Mosberger-Tang wrote:
> Previously, the driver created a net device (typically wlan0) as soon
> as the module was loaded.  This commit changes the driver to follow
> normal Linux convention of creating the net device only when bus
> probing detects a supported chip.

As already mentioned multiple times, I am skeptical about the validity of
keeping netdev registration before chip presence check, but I am not the
maintainer, so I let Ajay and Kalle decide for this. Aside from that, and from
the kasan warning which is not especially related to the series (and not
observed in nominal case), I still have a few minor comments below

> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
> changelog:
> V1: original version
> V2: Add missing forward declarations
> V3: Add missing forward declarations, actually :-(
> V4: - rearranged function order to improve readability
>     - now relative to wireless-next repository
>     - avoid change error return value and have lower-level functions
>       directly return -ENODEV instead
>     - removed any mention of WILC3000
>     - export and use existing is_wilc1000() for chipid validation
>     - replaced strbool() function with open code
> 
>  drivers/net/wireless/microchip/wilc1000/spi.c | 74 ++++++++++++++-----
>  .../net/wireless/microchip/wilc1000/wlan.c    |  3 +-
>  .../net/wireless/microchip/wilc1000/wlan.h    |  1 +
>  3 files changed, 59 insertions(+), 19 deletions(-)

[...]

> @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
>  	}
>  	if (ret) {
>  		dev_err(&spi->dev, "Failed with CRC7 on and off.\n");
> -		return ret;
> +		return -ENODEV;

You are still rewriting error codes here. At a lower level, sure, but still...
When I suggested setting -ENODEV at lower level, I was thinking about places
where no explicit error code was already in use, but
spi_internal_read/spi_internal_write already generate proper error codes. Or am
I missing a constraint, like the probe chain really needing -ENODEV ?

>  	}
>  
>  	/* set up the desired CRC configuration: */
> @@ -1165,7 +1193,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
>  		dev_err(&spi->dev,
>  			"[wilc spi %d]: Failed internal write reg\n",
>  			__LINE__);
> -		return ret;
> +		return -ENODEV;
>  	}
>  	/* update our state to match new protocol settings: */
>  	spi_priv->crc7_enabled = enable_crc7;
> @@ -1176,17 +1204,27 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
>  
>  	spi_priv->probing_crc = false;
>  
> +	return 0;
> +}
> +
> +static int wilc_validate_chipid(struct wilc *wilc)
> +{
> +	struct spi_device *spi = to_spi_device(wilc->dev);
> +	u32 chipid;
> +	int ret;
> +
>  	/*
>  	 * make sure can read chip id without protocol error
>  	 */
>  	ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid);
>  	if (ret) {
>  		dev_err(&spi->dev, "Fail cmd read chip id...\n");
> -		return ret;
> +		return -ENODEV;
> +	}
> +	if (!is_wilc1000(chipid)) {
> +		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
> +		return -ENODEV;
>  	}
> -
> -	spi_priv->isinit = true;
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 6b2f2269ddf8..3130a3ea8d71 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -12,10 +12,11 @@
>  
>  #define WAKE_UP_TRIAL_RETRY		10000
>  
> -static inline bool is_wilc1000(u32 id)
> +bool is_wilc1000(u32 id)
>  {
>  	return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID;
>  }
> +EXPORT_SYMBOL_GPL(is_wilc1000);

nit: Since the function is not static anymore, it would have been nice to move
it with the other exported functions to maintain the existing functions ordering

>  static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
>  {
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
> index f02775f7e41f..ebdfb0afaf71 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.h
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
> @@ -409,6 +409,7 @@ struct wilc_cfg_rsp {
>  
>  struct wilc_vif;
>  
> +bool is_wilc1000(u32 id);
>  int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
>  				u32 buffer_size);
>  int wilc_wlan_start(struct wilc *wilc);

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com