[PATCH] wlcore: sdio: warn only once for wl12xx_sdio_raw_{read,write}() failures

[PATCH] wlcore: sdio: warn only once for wl12xx_sdio_raw_{read,write}() failures

[Javier Martinez Canillas]

Report these failures only once, instead of keep logging the warnings for
the same condition every time that a SDIO read or write is attempted. This
behaviour is spammy and unnecessarily pollutes the kernel log buffer.

For example, on an AM625 BeaglePlay board where accessing a SDIO WiFi chip
fails with an -110 error:

  $ dmesg | grep "sdio write\|read failed (-110)" | wc -l
  39

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/net/wireless/ti/wlcore/sdio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
index eb5482ed76ae..47ecf33a0fbe 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -75,8 +75,8 @@ static int __must_check wl12xx_sdio_raw_read(struct device *child, int addr,
 
 	sdio_release_host(func);
 
-	if (WARN_ON(ret))
-		dev_err(child->parent, "sdio read failed (%d)\n", ret);
+	if (WARN_ON_ONCE(ret))
+		dev_err_once(child->parent, "sdio read failed (%d)\n", ret);
 
 	if (unlikely(dump)) {
 		printk(KERN_DEBUG "wlcore_sdio: READ from 0x%04x\n", addr);
@@ -120,8 +120,8 @@ static int __must_check wl12xx_sdio_raw_write(struct device *child, int addr,
 
 	sdio_release_host(func);
 
-	if (WARN_ON(ret))
-		dev_err(child->parent, "sdio write failed (%d)\n", ret);
+	if (WARN_ON_ONCE(ret))
+		dev_err_once(child->parent, "sdio write failed (%d)\n", ret);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH] wlcore: sdio: warn only once for wl12xx_sdio_raw_{read,write}() failures

[Breno Leitao]

On Tue, Feb 27, 2024 at 01:20:46AM +0100, Javier Martinez Canillas wrote:
> Report these failures only once, instead of keep logging the warnings for
> the same condition every time that a SDIO read or write is attempted. This
> behaviour is spammy and unnecessarily pollutes the kernel log buffer.
> 
> For example, on an AM625 BeaglePlay board where accessing a SDIO WiFi chip
> fails with an -110 error:
> 
>   $ dmesg | grep "sdio write\|read failed (-110)" | wc -l
>   39
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH] wlcore: sdio: warn only once for wl12xx_sdio_raw_{read,write}() failures

[Kalle Valo]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Report these failures only once, instead of keep logging the warnings for
> the same condition every time that a SDIO read or write is attempted. This
> behaviour is spammy and unnecessarily pollutes the kernel log buffer.

Removing error messages is not usually a good idea, it would be much
better to fix the root cause.

> For example, on an AM625 BeaglePlay board where accessing a SDIO WiFi chip
> fails with an -110 error:
>
>   $ dmesg | grep "sdio write\|read failed (-110)" | wc -l
>   39

-110 is -ETIMEDOUT. Why is it timing out?

> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  drivers/net/wireless/ti/wlcore/sdio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
> index eb5482ed76ae..47ecf33a0fbe 100644
> --- a/drivers/net/wireless/ti/wlcore/sdio.c
> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
> @@ -75,8 +75,8 @@ static int __must_check wl12xx_sdio_raw_read(struct device *child, int addr,
>  
>  	sdio_release_host(func);
>  
> -	if (WARN_ON(ret))
> -		dev_err(child->parent, "sdio read failed (%d)\n", ret);
> +	if (WARN_ON_ONCE(ret))
> +		dev_err_once(child->parent, "sdio read failed (%d)\n", ret);

WARN_ON() feels excessive here, maybe remove that entirely? But
dev_err_ratelimited() feels more approriate than printing the error just
once.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wlcore: sdio: warn only once for wl12xx_sdio_raw_{read,write}() failures

[Javier Martinez Canillas]

Kalle Valo <kvalo@kernel.org> writes:

Hello Kalle,

Thanks for your feedback.

> Javier Martinez Canillas <javierm@redhat.com> writes:
>
>> Report these failures only once, instead of keep logging the warnings for
>> the same condition every time that a SDIO read or write is attempted. This
>> behaviour is spammy and unnecessarily pollutes the kernel log buffer.
>
> Removing error messages is not usually a good idea, it would be much

This patch is not removing error messages though, just limiting to print
only since IMO there is no need to constantly keep printing the same error
message over and over.

> better to fix the root cause.
>

Agreed and I'm trying to figure out the cause. But to do that, I need a
usable serial console and it's barely usable with all the warns and stack
traces printed while I'm trying to type commands.

>> For example, on an AM625 BeaglePlay board where accessing a SDIO WiFi chip
>> fails with an -110 error:
>>
>>   $ dmesg | grep "sdio write\|read failed (-110)" | wc -l
>>   39
>
> -110 is -ETIMEDOUT. Why is it timing out?
>

If I knew it then I wouldn't have to type this patch :) In theory it
should work according to Nishanth (Cc'ed) since I've both the firmware
and the required patches for the bootloader to set some clocks early.

But it's not working for me... I don't know what's missing for me.

>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

[...]

>> -	if (WARN_ON(ret))
>> -		dev_err(child->parent, "sdio read failed (%d)\n", ret);
>> +	if (WARN_ON_ONCE(ret))
>> +		dev_err_once(child->parent, "sdio read failed (%d)\n", ret);
>
> WARN_ON() feels excessive here, maybe remove that entirely? But

Agreed and I'm on board to drop it.

> dev_err_ratelimited() feels more approriate than printing the error just
> once.
>

Works for me. Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat