| Message ID | 7390d51f-60e2-3cee-5277-b819a55ceabe@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] wwan_hwsim: Avoid flush_scheduled_work() usage | expand |
On Fri, 22 Apr 2022 at 05:01, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > Flushing system-wide workqueues is dangerous and will be forbidden. > Replace system_wq with local wwan_wq. > > While we are at it, make err_clean_devs: label of wwan_hwsim_init() > behave like wwan_hwsim_exit(), for it is theoretically possible to call > wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write() > by the moment wwan_hwsim_init_devs() returns. > > Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > Changes in v2: > Keep flush_workqueue(wwan_wq) explicit in order to match the comment. > Make error path of wwan_hwsim_init() identical to wwan_hwsim_exit(). > > drivers/net/wwan/wwan_hwsim.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c > index 5b62cf3b3c42..fad642f9ffd8 100644 > --- a/drivers/net/wwan/wwan_hwsim.c > +++ b/drivers/net/wwan/wwan_hwsim.c > @@ -33,6 +33,7 @@ static struct dentry *wwan_hwsim_debugfs_devcreate; > static DEFINE_SPINLOCK(wwan_hwsim_devs_lock); > static LIST_HEAD(wwan_hwsim_devs); > static unsigned int wwan_hwsim_dev_idx; > +static struct workqueue_struct *wwan_wq; > > struct wwan_hwsim_dev { > struct list_head list; > @@ -371,7 +372,7 @@ static ssize_t wwan_hwsim_debugfs_portdestroy_write(struct file *file, > * waiting this callback to finish in the debugfs_remove() call. So, > * use workqueue. > */ > - schedule_work(&port->del_work); > + queue_work(wwan_wq, &port->del_work); > > return count; > } > @@ -416,7 +417,7 @@ static ssize_t wwan_hwsim_debugfs_devdestroy_write(struct file *file, > * waiting this callback to finish in the debugfs_remove() call. So, > * use workqueue. > */ > - schedule_work(&dev->del_work); > + queue_work(wwan_wq, &dev->del_work); > > return count; > } > @@ -506,9 +507,15 @@ static int __init wwan_hwsim_init(void) > if (wwan_hwsim_devsnum < 0 || wwan_hwsim_devsnum > 128) > return -EINVAL; > > + wwan_wq = alloc_workqueue("wwan_wq", 0, 0); > + if (!wwan_wq) > + return -ENOMEM; > + > wwan_hwsim_class = class_create(THIS_MODULE, "wwan_hwsim"); > - if (IS_ERR(wwan_hwsim_class)) > - return PTR_ERR(wwan_hwsim_class); > + if (IS_ERR(wwan_hwsim_class)) { > + err = PTR_ERR(wwan_hwsim_class); > + goto err_wq_destroy; > + } > > wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL); > wwan_hwsim_debugfs_devcreate = > @@ -523,9 +530,13 @@ static int __init wwan_hwsim_init(void) > return 0; > > err_clean_devs: > + debugfs_remove(wwan_hwsim_debugfs_devcreate); /* Avoid new devs */ > wwan_hwsim_free_devs(); > + flush_workqueue(wwan_wq); /* Wait deletion works completion */ > debugfs_remove(wwan_hwsim_debugfs_topdir); > class_destroy(wwan_hwsim_class); > +err_wq_destroy: > + destroy_workqueue(wwan_wq); > > return err; > } > @@ -534,9 +545,10 @@ static void __exit wwan_hwsim_exit(void) > { > debugfs_remove(wwan_hwsim_debugfs_devcreate); /* Avoid new devs */ > wwan_hwsim_free_devs(); > - flush_scheduled_work(); /* Wait deletion works completion */ > + flush_workqueue(wwan_wq); /* Wait deletion works completion */ > debugfs_remove(wwan_hwsim_debugfs_topdir); > class_destroy(wwan_hwsim_class); > + destroy_workqueue(wwan_wq); > } > > module_init(wwan_hwsim_init); > -- > 2.32.0 >
On Fri, 22 Apr 2022 12:01:24 +0900 Tetsuo Handa wrote: > Flushing system-wide workqueues is dangerous and will be forbidden. > Replace system_wq with local wwan_wq. > > While we are at it, make err_clean_devs: label of wwan_hwsim_init() > behave like wwan_hwsim_exit(), for it is theoretically possible to call > wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write() > by the moment wwan_hwsim_init_devs() returns. > > Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> Commit cc271ab86606 ("wwan_hwsim: Avoid flush_scheduled_work() usage") in net-next now, thanks!
David, can you take this patch? On 2022/04/22 15:38, Loic Poulain wrote: > On Fri, 22 Apr 2022 at 05:01, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> Flushing system-wide workqueues is dangerous and will be forbidden. >> Replace system_wq with local wwan_wq. >> >> While we are at it, make err_clean_devs: label of wwan_hwsim_init() >> behave like wwan_hwsim_exit(), for it is theoretically possible to call >> wwan_hwsim_debugfs_devcreate_write()/wwan_hwsim_debugfs_devdestroy_write() >> by the moment wwan_hwsim_init_devs() returns. >> >> Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > > >> --- >> Changes in v2: >> Keep flush_workqueue(wwan_wq) explicit in order to match the comment. >> Make error path of wwan_hwsim_init() identical to wwan_hwsim_exit(). >> >> drivers/net/wwan/wwan_hwsim.c | 22 +++++++++++++++++----- >> 1 file changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c >> index 5b62cf3b3c42..fad642f9ffd8 100644 >> --- a/drivers/net/wwan/wwan_hwsim.c >> +++ b/drivers/net/wwan/wwan_hwsim.c >> @@ -33,6 +33,7 @@ static struct dentry *wwan_hwsim_debugfs_devcreate; >> static DEFINE_SPINLOCK(wwan_hwsim_devs_lock); >> static LIST_HEAD(wwan_hwsim_devs); >> static unsigned int wwan_hwsim_dev_idx; >> +static struct workqueue_struct *wwan_wq; >> >> struct wwan_hwsim_dev { >> struct list_head list; >> @@ -371,7 +372,7 @@ static ssize_t wwan_hwsim_debugfs_portdestroy_write(struct file *file, >> * waiting this callback to finish in the debugfs_remove() call. So, >> * use workqueue. >> */ >> - schedule_work(&port->del_work); >> + queue_work(wwan_wq, &port->del_work); >> >> return count; >> } >> @@ -416,7 +417,7 @@ static ssize_t wwan_hwsim_debugfs_devdestroy_write(struct file *file, >> * waiting this callback to finish in the debugfs_remove() call. So, >> * use workqueue. >> */ >> - schedule_work(&dev->del_work); >> + queue_work(wwan_wq, &dev->del_work); >> >> return count; >> } >> @@ -506,9 +507,15 @@ static int __init wwan_hwsim_init(void) >> if (wwan_hwsim_devsnum < 0 || wwan_hwsim_devsnum > 128) >> return -EINVAL; >> >> + wwan_wq = alloc_workqueue("wwan_wq", 0, 0); >> + if (!wwan_wq) >> + return -ENOMEM; >> + >> wwan_hwsim_class = class_create(THIS_MODULE, "wwan_hwsim"); >> - if (IS_ERR(wwan_hwsim_class)) >> - return PTR_ERR(wwan_hwsim_class); >> + if (IS_ERR(wwan_hwsim_class)) { >> + err = PTR_ERR(wwan_hwsim_class); >> + goto err_wq_destroy; >> + } >> >> wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL); >> wwan_hwsim_debugfs_devcreate = >> @@ -523,9 +530,13 @@ static int __init wwan_hwsim_init(void) >> return 0; >> >> err_clean_devs: >> + debugfs_remove(wwan_hwsim_debugfs_devcreate); /* Avoid new devs */ >> wwan_hwsim_free_devs(); >> + flush_workqueue(wwan_wq); /* Wait deletion works completion */ >> debugfs_remove(wwan_hwsim_debugfs_topdir); >> class_destroy(wwan_hwsim_class); >> +err_wq_destroy: >> + destroy_workqueue(wwan_wq); >> >> return err; >> } >> @@ -534,9 +545,10 @@ static void __exit wwan_hwsim_exit(void) >> { >> debugfs_remove(wwan_hwsim_debugfs_devcreate); /* Avoid new devs */ >> wwan_hwsim_free_devs(); >> - flush_scheduled_work(); /* Wait deletion works completion */ >> + flush_workqueue(wwan_wq); /* Wait deletion works completion */ >> debugfs_remove(wwan_hwsim_debugfs_topdir); >> class_destroy(wwan_hwsim_class); >> + destroy_workqueue(wwan_wq); >> } >> >> module_init(wwan_hwsim_init); >> -- >> 2.32.0 >>
diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c index 5b62cf3b3c42..fad642f9ffd8 100644 --- a/drivers/net/wwan/wwan_hwsim.c +++ b/drivers/net/wwan/wwan_hwsim.c @@ -33,6 +33,7 @@ static struct dentry *wwan_hwsim_debugfs_devcreate; static DEFINE_SPINLOCK(wwan_hwsim_devs_lock); static LIST_HEAD(wwan_hwsim_devs); static unsigned int wwan_hwsim_dev_idx; +static struct workqueue_struct *wwan_wq; struct wwan_hwsim_dev { struct list_head list; @@ -371,7 +372,7 @@ static ssize_t wwan_hwsim_debugfs_portdestroy_write(struct file *file, * waiting this callback to finish in the debugfs_remove() call. So, * use workqueue. */ - schedule_work(&port->del_work); + queue_work(wwan_wq, &port->del_work); return count; } @@ -416,7 +417,7 @@ static ssize_t wwan_hwsim_debugfs_devdestroy_write(struct file *file, * waiting this callback to finish in the debugfs_remove() call. So, * use workqueue. */ - schedule_work(&dev->del_work); + queue_work(wwan_wq, &dev->del_work); return count; } @@ -506,9 +507,15 @@ static int __init wwan_hwsim_init(void) if (wwan_hwsim_devsnum < 0 || wwan_hwsim_devsnum > 128) return -EINVAL; + wwan_wq = alloc_workqueue("wwan_wq", 0, 0); + if (!wwan_wq) + return -ENOMEM; + wwan_hwsim_class = class_create(THIS_MODULE, "wwan_hwsim"); - if (IS_ERR(wwan_hwsim_class)) - return PTR_ERR(wwan_hwsim_class); + if (IS_ERR(wwan_hwsim_class)) { + err = PTR_ERR(wwan_hwsim_class); + goto err_wq_destroy; + } wwan_hwsim_debugfs_topdir = debugfs_create_dir("wwan_hwsim", NULL); wwan_hwsim_debugfs_devcreate = @@ -523,9 +530,13 @@ static int __init wwan_hwsim_init(void) return 0; err_clean_devs: + debugfs_remove(wwan_hwsim_debugfs_devcreate); /* Avoid new devs */ wwan_hwsim_free_devs(); + flush_workqueue(wwan_wq); /* Wait deletion works completion */ debugfs_remove(wwan_hwsim_debugfs_topdir); class_destroy(wwan_hwsim_class); +err_wq_destroy: + destroy_workqueue(wwan_wq); return err; } @@ -534,9 +545,10 @@ static void __exit wwan_hwsim_exit(void) { debugfs_remove(wwan_hwsim_debugfs_devcreate); /* Avoid new devs */ wwan_hwsim_free_devs(); - flush_scheduled_work(); /* Wait deletion works completion */ + flush_workqueue(wwan_wq); /* Wait deletion works completion */ debugfs_remove(wwan_hwsim_debugfs_topdir); class_destroy(wwan_hwsim_class); + destroy_workqueue(wwan_wq); } module_init(wwan_hwsim_init);