* Re: [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52()
2025-05-16 8:38 [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52() Wentao Liang
@ 2025-05-16 13:50 ` Markus Elfring
2025-05-16 13:56 ` Johannes Berg
2025-05-17 20:35 ` kernel test robot
2025-05-19 7:32 ` Geert Uytterhoeven
2 siblings, 1 reply; 5+ messages in thread
From: Markus Elfring @ 2025-05-16 13:50 UTC (permalink / raw)
To: vulab, linux-wireless, Ajay Singh, Claudiu Beznea, Kalle Valo
Cc: stable, LKML
…
> Add error handling for wilc_sdio_cmd52(). If wilc_sdio_cmd52() fails,
> log an error message via dev_err().
Please avoid duplicate exception handling code.
Can another jump target become nicer for this purpose?
Regards,
Markus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52()
2025-05-16 13:50 ` Markus Elfring
@ 2025-05-16 13:56 ` Johannes Berg
0 siblings, 0 replies; 5+ messages in thread
From: Johannes Berg @ 2025-05-16 13:56 UTC (permalink / raw)
To: Markus Elfring, vulab, linux-wireless, Ajay Singh, Claudiu Beznea,
Kalle Valo
Cc: stable, LKML
On Fri, 2025-05-16 at 15:50 +0200, Markus Elfring wrote:
> …
> > Add error handling for wilc_sdio_cmd52(). If wilc_sdio_cmd52() fails,
> > log an error message via dev_err().
>
> Please avoid duplicate exception handling code.
> Can another jump target become nicer for this purpose?
>
<form letter>
(stolen from Greg)
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52()
2025-05-16 8:38 [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52() Wentao Liang
2025-05-16 13:50 ` Markus Elfring
@ 2025-05-17 20:35 ` kernel test robot
2025-05-19 7:32 ` Geert Uytterhoeven
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2025-05-17 20:35 UTC (permalink / raw)
To: Wentao Liang, ajay.kathat, claudiu.beznea, kvalo
Cc: oe-kbuild-all, linux-wireless, linux-kernel, Wentao Liang, stable
Hi Wentao,
kernel test robot noticed the following build errors:
[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.15-rc6 next-20250516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wentao-Liang/wifi-wilc1000-Add-error-handling-for-wilc_sdio_cmd52/20250516-163959
base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link: https://lore.kernel.org/r/20250516083842.903-1-vulab%40iscas.ac.cn
patch subject: [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52()
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250518/202505180440.dyQDHWJJ-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250518/202505180440.dyQDHWJJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505180440.dyQDHWJJ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/mmc/sdio_func.h:11,
from drivers/net/wireless/microchip/wilc1000/sdio.c:8:
drivers/net/wireless/microchip/wilc1000/sdio.c: In function 'wilc_sdio_read_size':
>> drivers/net/wireless/microchip/wilc1000/sdio.c:792:32: error: 'struct sdio_func' has no member named 'devm'; did you mean 'dev'?
792 | dev_err(&func->devm, "Fail cmd 52, interrupt data register...\n");
| ^~~~
include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
drivers/net/wireless/microchip/wilc1000/sdio.c:792:17: note: in expansion of macro 'dev_err'
792 | dev_err(&func->devm, "Fail cmd 52, interrupt data register...\n");
| ^~~~~~~
drivers/net/wireless/microchip/wilc1000/sdio.c:801:32: error: 'struct sdio_func' has no member named 'devm'; did you mean 'dev'?
801 | dev_err(&func->devm, "Fail cmd 52, interrupt data register...\n");
| ^~~~
include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
drivers/net/wireless/microchip/wilc1000/sdio.c:801:17: note: in expansion of macro 'dev_err'
801 | dev_err(&func->devm, "Fail cmd 52, interrupt data register...\n");
| ^~~~~~~
vim +792 drivers/net/wireless/microchip/wilc1000/sdio.c
774
775 static int wilc_sdio_read_size(struct wilc *wilc, u32 *size)
776 {
777 u32 tmp;
778 struct sdio_cmd52 cmd;
779 struct sdio_func *func = dev_to_sdio_func(wilc->dev);
780 int ret;
781
782 /**
783 * Read DMA count in words
784 **/
785 cmd.read_write = 0;
786 cmd.function = 0;
787 cmd.raw = 0;
788 cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG;
789 cmd.data = 0;
790 ret = wilc_sdio_cmd52(wilc, &cmd);
791 if (ret) {
> 792 dev_err(&func->devm, "Fail cmd 52, interrupt data register...\n");
793 return ret;
794 }
795 tmp = cmd.data;
796
797 cmd.address = WILC_SDIO_INTERRUPT_DATA_SZ_REG + 1;
798 cmd.data = 0;
799 ret = wilc_sdio_cmd52(wilc, &cmd);
800 if (ret) {
801 dev_err(&func->devm, "Fail cmd 52, interrupt data register...\n");
802 return ret;
803 }
804 tmp |= (cmd.data << 8);
805
806 *size = tmp;
807 return 0;
808 }
809
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52()
2025-05-16 8:38 [PATCH] wifi: wilc1000: Add error handling for wilc_sdio_cmd52() Wentao Liang
2025-05-16 13:50 ` Markus Elfring
2025-05-17 20:35 ` kernel test robot
@ 2025-05-19 7:32 ` Geert Uytterhoeven
2 siblings, 0 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2025-05-19 7:32 UTC (permalink / raw)
To: Wentao Liang
Cc: ajay.kathat, claudiu.beznea, kvalo, linux-wireless, linux-kernel,
stable
Hi Wentao,
Thanks for your patch!
On Fri, 16 May 2025 at 10:39, Wentao Liang <vulab@iscas.ac.cn> wrote:
> The wilc_sdio_read_size() calls wilc_sdio_cmd52() but does not check the
> return value. This could lead to execution with potentially invalid data
> if wilc_sdio_cmd52() fails. A proper implementation can be found in
> wilc_sdio_read_reg().
>
> Add error handling for wilc_sdio_cmd52(). If wilc_sdio_cmd52() fails,
> log an error message via dev_err().
There is no need to print an error message, as wilc_sdio_cmd52()
already does that. Same with the existing wilc_sdio_read_reg(), and
with all of its callers, which leads to multiple error messages for
a single failure.
> Fixes: 0e1af73ddeb9 ("staging/wilc1000: use proper naming for global symbols")
This is not the commit you are looking for...
> Cc: stable@vger.kernel.org # v4.15
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
> @@ -771,6 +771,8 @@ static int wilc_sdio_read_size(struct wilc *wilc, u32 *size)
> {
> u32 tmp;
> struct sdio_cmd52 cmd;
> + struct sdio_func *func = dev_to_sdio_func(wilc->dev);
> + 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->devm, "Fail cmd 52, interrupt data register...\n");
Looks like the AI wasn't trained properly. Please try to (at least)
test-compile your patches.
> + 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->devm, "Fail cmd 52, interrupt data register...\n");
Likewise.
> + return ret;
> + }
> tmp |= (cmd.data << 8);
>
> *size = tmp;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 5+ messages in thread