wl18xx: Bad format for rx_frames_per_rates in debugfs?

wl18xx: Bad format for rx_frames_per_rates in debugfs?

[Nicolas Iooss]

Hello,

While adding __printf attributes to several functions in the kernel, I
got a surprising gcc warning in drivers/net/wireless/ti/wl18xx/debugfs.c
about "format '%u' expects argument of type 'unsigned int', but argument
5 has type 'u32 *'".

Indeed it seems that commit c5d94169e818 ("wl18xx: use new fw stats
structures") [1] introduced an array field "u32 rx_frames_per_rates[50]"
in struct wl18xx_acx_rx_rate_stat but is using
WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u"); instead
of something like WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(rx_rate,
rx_frames_per_rates, 50); for displaying this value.  So I believe that
currently the rx_rate entry in debugfs contains a kernel pointer instead
of the actual data.  As I don't have the hardware to test I can't be
sure of it.

Is this a real bug which needs to be fixed or something weird I haven't
understood yet?

Thanks

--
Nicolas

[1]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c5d94169e8189d02dfbd6143411908357865d777

PS: I got this gcc warning by adding __printf(4, 5) to
wl1271_format_buffer() prototype in
drivers/net/wireless/ti/wlcore/debugfs.h:

In file included from
/usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c:23:0:
    /usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c: In function
'rx_rate_rx_frames_per_rates_read':
    /usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c:34:32:
error: format '%u' expects argument of type 'unsigned int', but argument
5 has type 'u32 *' [-Werror=format=]
      DEBUGFS_FWSTATS_FILE(a, b, c, wl18xx_acx_statistics)
                                    ^

/usr/src/linux/drivers/net/wireless/ti/wl18xx/../wlcore/debugfs.h:77:9:
note: in definition of macro 'DEBUGFS_FWSTATS_FILE'
      struct struct_type *stats = wl->stats.fw_stats;   \
             ^
    /usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c:142:1: note:
in expansion of macro 'WL18XX_DEBUGFS_FWSTATS_FILE'
     WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u");
     ^
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wl18xx: Bad format for rx_frames_per_rates in debugfs?

[Eliad Peller]

On Thu, Mar 12, 2015 at 2:39 PM, Nicolas Iooss
<nicolas.iooss_linux@m4x.org> wrote:
> Hello,
>
> While adding __printf attributes to several functions in the kernel, I
> got a surprising gcc warning in drivers/net/wireless/ti/wl18xx/debugfs.c
> about "format '%u' expects argument of type 'unsigned int', but argument
> 5 has type 'u32 *'".
>
> Indeed it seems that commit c5d94169e818 ("wl18xx: use new fw stats
> structures") [1] introduced an array field "u32 rx_frames_per_rates[50]"
> in struct wl18xx_acx_rx_rate_stat but is using
> WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u"); instead
> of something like WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(rx_rate,
> rx_frames_per_rates, 50); for displaying this value.  So I believe that
> currently the rx_rate entry in debugfs contains a kernel pointer instead
> of the actual data.  As I don't have the hardware to test I can't be
> sure of it.
>
> Is this a real bug which needs to be fixed or something weird I haven't
> understood yet?
>
yes, seems like a bug.

Eliad.

[PATCH] wl18xx: show rx_frames_per_rates as an array as it really is

[Nicolas Iooss]

In struct wl18xx_acx_rx_rate_stat, rx_frames_per_rates field is an
array, not a number.  This means WL18XX_DEBUGFS_FWSTATS_FILE can't be
used to display this field in debugfs (it would display a pointer, not
the actual data).  Use WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY instead.

This bug has been found by adding a __printf attribute to
wl1271_format_buffer.  gcc complained about "format '%u' expects
argument of type 'unsigned int', but argument 5 has type 'u32 *'".

Fixes: c5d94169e818 ("wl18xx: use new fw stats structures")
Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---

This patch is only compile-tested as I haven't got the hardware to
test it live.

 drivers/net/wireless/ti/wl18xx/debugfs.c | 2 +-
 drivers/net/wireless/ti/wlcore/debugfs.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ti/wl18xx/debugfs.c b/drivers/net/wireless/ti/wl18xx/debugfs.c
index c93fae95baac..5fbd2230f372 100644
--- a/drivers/net/wireless/ti/wl18xx/debugfs.c
+++ b/drivers/net/wireless/ti/wl18xx/debugfs.c
@@ -139,7 +139,7 @@ WL18XX_DEBUGFS_FWSTATS_FILE(rx_filter, protection_filter, "%u");
 WL18XX_DEBUGFS_FWSTATS_FILE(rx_filter, accum_arp_pend_requests, "%u");
 WL18XX_DEBUGFS_FWSTATS_FILE(rx_filter, max_arp_queue_dep, "%u");
 
-WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u");
+WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(rx_rate, rx_frames_per_rates, 50);
 
 WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(aggr_size, tx_agg_vs_rate,
 				  AGGR_STATS_TX_AGG*AGGR_STATS_TX_RATE);
diff --git a/drivers/net/wireless/ti/wlcore/debugfs.h b/drivers/net/wireless/ti/wlcore/debugfs.h
index 0f2cfb0d2a9e..bf14676e6515 100644
--- a/drivers/net/wireless/ti/wlcore/debugfs.h
+++ b/drivers/net/wireless/ti/wlcore/debugfs.h
@@ -26,8 +26,8 @@
 
 #include "wlcore.h"
 
-int wl1271_format_buffer(char __user *userbuf, size_t count,
-			 loff_t *ppos, char *fmt, ...);
+__printf(4, 5) int wl1271_format_buffer(char __user *userbuf, size_t count,
+					loff_t *ppos, char *fmt, ...);
 
 int wl1271_debugfs_init(struct wl1271 *wl);
 void wl1271_debugfs_exit(struct wl1271 *wl);
-- 
2.3.2

Re: wl18xx: show rx_frames_per_rates as an array as it really is

[Kalle Valo]

> In struct wl18xx_acx_rx_rate_stat, rx_frames_per_rates field is an
> array, not a number.  This means WL18XX_DEBUGFS_FWSTATS_FILE can't be
> used to display this field in debugfs (it would display a pointer, not
> the actual data).  Use WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY instead.
> 
> This bug has been found by adding a __printf attribute to
> wl1271_format_buffer.  gcc complained about "format '%u' expects
> argument of type 'unsigned int', but argument 5 has type 'u32 *'".
> 
> Fixes: c5d94169e818 ("wl18xx: use new fw stats structures")
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html