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

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

[Marek Vasut]

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@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.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");
+		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)
-- 
2.45.2

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

[Alexis Lothoré]

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@denx.de>
> ---
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
> Cc: Ajay Singh <ajay.kathat@microchip.com>
> Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.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

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

[Marek Vasut]

On 10/22/24 11:11 AM, Alexis Lothoré wrote:

Hi,

>> 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
Fixed in V2 , s@set@get@ , thanks !