netdev - Re: [PATCH] wifi: wilc1000: Add proper error handling for remaining CMD52

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: <a6529da9-6333-4516-923d-01f12c439b33@bootlin.com>
Date: Tue, 22 Oct 2024 11:11:42 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Marek Vasut <marex@...x.de>, linux-wireless@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
 Adham Abozaeid <adham.abozaeid@...rochip.com>,
 Ajay Singh <ajay.kathat@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>, Conor Dooley
 <conor+dt@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Kalle Valo <kvalo@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH] wifi: wilc1000: Add proper error handling for remaining
 CMD52


Hi Marek,

On 10/18/24 21:41, Marek Vasut wrote:
> A few of the CMD52 calls did not have any error handling, add it.
> This prevents odd errors like "Unexpected interrupt (1) int=nnn"
> when the CMD52 fails just above in the IRQ handler and the CMD52
> error code is ignored by the driver. Fill the error handling in.
> Sort the variables in those affected functions while at it. Note
> that the error code itself is already printed in wilc_sdio_cmd52().
> 
> Signed-off-by: Marek Vasut <marex@...x.de>
> ---
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Adham Abozaeid <adham.abozaeid@...rochip.com>
> Cc: Ajay Singh <ajay.kathat@...rochip.com>
> Cc: Alexis Lothoré <alexis.lothore@...tlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@...on.dev>
> Cc: Conor Dooley <conor+dt@...nel.org>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Kalle Valo <kvalo@...nel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: Rob Herring <robh@...nel.org>
> Cc: devicetree@...r.kernel.org
> Cc: linux-wireless@...r.kernel.org
> Cc: netdev@...r.kernel.org
> ---
>  .../net/wireless/microchip/wilc1000/sdio.c    | 27 ++++++++++++++-----
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
> index 5262c8846c13d..170470d1c2092 100644
> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
> @@ -769,8 +769,10 @@ static int wilc_sdio_init(struct wilc *wilc, bool resume)
>  
>  static int wilc_sdio_read_size(struct wilc *wilc, u32 *size)
>  {
> -	u32 tmp;
> +	struct sdio_func *func = dev_to_sdio_func(wilc->dev);
>  	struct sdio_cmd52 cmd;
> +	u32 tmp;
> +	int ret;
>  
>  	/**
>  	 *      Read DMA count in words
> @@ -780,12 +782,20 @@ static int wilc_sdio_read_size(struct wilc *wilc, u32 *size)
>  	cmd.raw = 0;
>  	cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG;
>  	cmd.data = 0;
> -	wilc_sdio_cmd52(wilc, &cmd);
> +	ret = wilc_sdio_cmd52(wilc, &cmd);
> +	if (ret) {
> +		dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[0] register...\n");

I don't get the log message, why "set" DATA_SZ[0] ? This helper is rather trying
to read it. Same for the other logs added below

> +		return ret;
> +	}
>  	tmp = cmd.data;
>  
>  	cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG + 1;
>  	cmd.data = 0;
> -	wilc_sdio_cmd52(wilc, &cmd);
> +	ret = wilc_sdio_cmd52(wilc, &cmd);
> +	if (ret) {
> +		dev_err(&func->dev, "Fail cmd 52, set DATA_SZ[1] register...\n");
> +		return ret;
> +	}
>  	tmp |= (cmd.data << 8);
>  
>  	*size = tmp;
> @@ -796,9 +806,10 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status)
>  {
>  	struct sdio_func *func = dev_to_sdio_func(wilc->dev);
>  	struct wilc_sdio *sdio_priv = wilc->bus_data;
> -	u32 tmp;
> -	u8 irq_flags;
>  	struct sdio_cmd52 cmd;
> +	u8 irq_flags;
> +	u32 tmp;
> +	int ret;
>  
>  	wilc_sdio_read_size(wilc, &tmp);
>  
> @@ -817,7 +828,11 @@ static int wilc_sdio_read_int(struct wilc *wilc, u32 *int_status)
>  	cmd.raw = 0;
>  	cmd.read_write = 0;
>  	cmd.data = 0;
> -	wilc_sdio_cmd52(wilc, &cmd);
> +	ret = wilc_sdio_cmd52(wilc, &cmd);
> +	if (ret) {
> +		dev_err(&func->dev, "Fail cmd 52, set IRQ_FLAG register...\n");
> +		return ret;
> +	}
>  	irq_flags = cmd.data;
>  
>  	if (sdio_priv->irq_gpio)


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