Messages in this thread | | | From | Brian Norris <> | Date | Mon, 23 May 2022 10:55:49 -0700 | Subject | Re: [PATCH v3] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv |
| |
+ Johannes (you should check MAINTAINERS; devcoredump has a specified maintainer)
On Sun, May 22, 2022 at 10:29 PM Duoming Zhou <duoming@zju.edu.cn> wrote: > > There are sleep in atomic context bugs when uploading device dump > data in mwifiex. The root cause is that dev_coredumpv could not > be used in atomic contexts, because it calls dev_set_name which > include operations that may sleep. The call tree shows execution > paths that could lead to bugs: > > (Interrupt context) > fw_dump_timer_fn > mwifiex_upload_device_dump > dev_coredumpv(..., GFP_KERNEL) > dev_coredumpm() > kzalloc(sizeof(*devcd), gfp); //may sleep > dev_set_name > kobject_set_name_vargs > kvasprintf_const(GFP_KERNEL, ...); //may sleep > kstrdup(s, GFP_KERNEL); //may sleep
I was only half paying attention to this patch earlier, but I realize one source of my confusion: you're blaming the fix wrong. This piece of code was only added for mwifiex's USB support; the SDIO/PCIe support is totally fine, as we perform the dump from a worker, not a timer. So, you have the Fixes tag wrong.
> In order to let dev_coredumpv support atomic contexts, this patch > changes the gfp_t parameter of kvasprintf_const and kstrdup in > kobject_set_name_vargs from GFP_KERNEL to GFP_ATOMIC. What's more, > In order to mitigate bug, this patch changes the gfp_t parameter > of dev_coredumpv from GFP_KERNEL to GFP_ATOMIC. > > Fixes: 57670ee882d4 ("mwifiex: device dump support via devcoredump framework")
That's wrong. Should be:
Fixes: f5ecd02a8b20 mwifiex: device dump support for usb interface
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > Changes in v3: > - Let dev_coredumpv support atomic contexts. > > drivers/net/wireless/marvell/mwifiex/main.c | 2 +- > lib/kobject.c | 4 ++--
You almost definitely want to split this in two. One to fix devcoredump to _really_ support the gfp arg (or else to drop it), and one to start using it appropriately in mwifiex.
> 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c > index ace7371c477..258906920a2 100644 > --- a/drivers/net/wireless/marvell/mwifiex/main.c > +++ b/drivers/net/wireless/marvell/mwifiex/main.c > @@ -1116,7 +1116,7 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter) > mwifiex_dbg(adapter, MSG, > "== mwifiex dump information to /sys/class/devcoredump start\n"); > dev_coredumpv(adapter->dev, adapter->devdump_data, adapter->devdump_len, > - GFP_KERNEL); > + GFP_ATOMIC);
As noted above, PCIe and SDIO support is working just fine. Seems a bit much to force it to be GFP_ATOMIC in those cases.
Maybe you also need a gfp arg for mwifiex_upload_device_dump()?
Brian
> mwifiex_dbg(adapter, MSG, > "== mwifiex dump information to /sys/class/devcoredump end\n"); >
| |