| Message ID | 63ec2d45-c67c-1134-f6d3-490c8ba67a01@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/surface: avoid flush_scheduled_work() usage | expand |
On 6/10/22 07:41, Tetsuo Handa wrote: > Use local wq in order to avoid flush_scheduled_work() usage. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() > using a macro") for background. > > This is a blind conversion, and is only compile tested. Looks good to me, thanks! Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> Tested-by: Maximilian Luz <luzmaximilian@gmail.com> > .../platform/surface/surface_acpi_notify.c | 27 ++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c > index 7b758f8cc137..c0e12f0b9b79 100644 > --- a/drivers/platform/surface/surface_acpi_notify.c > +++ b/drivers/platform/surface/surface_acpi_notify.c > @@ -37,6 +37,7 @@ struct san_data { > #define to_san_data(ptr, member) \ > container_of(ptr, struct san_data, member) > > +static struct workqueue_struct *san_wq; > > /* -- dGPU notifier interface. ---------------------------------------------- */ > > @@ -356,7 +357,7 @@ static u32 san_evt_bat_nf(struct ssam_event_notifier *nf, > > memcpy(&work->event, event, sizeof(struct ssam_event) + event->length); > > - schedule_delayed_work(&work->work, delay); > + queue_delayed_work(san_wq, &work->work, delay); > return SSAM_NOTIF_HANDLED; > } > > @@ -861,7 +862,7 @@ static int san_remove(struct platform_device *pdev) > * We have unregistered our event sources. Now we need to ensure that > * all delayed works they may have spawned are run to completion. > */ > - flush_scheduled_work(); > + flush_workqueue(san_wq); > > return 0; > } > @@ -881,7 +882,27 @@ static struct platform_driver surface_acpi_notify = { > .probe_type = PROBE_PREFER_ASYNCHRONOUS, > }, > }; > -module_platform_driver(surface_acpi_notify); > + > +static int __init san_init(void) > +{ > + int ret; > + > + san_wq = alloc_workqueue("san_wq", 0, 0); > + if (!san_wq) > + return -ENOMEM; > + ret = platform_driver_register(&surface_acpi_notify); > + if (ret) > + destroy_workqueue(san_wq); > + return ret; > +} > +module_init(san_init); > + > +static void __exit san_exit(void) > +{ > + platform_driver_unregister(&surface_acpi_notify); > + destroy_workqueue(san_wq); > +} > +module_exit(san_exit); > > MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>"); > MODULE_DESCRIPTION("Surface ACPI Notify driver for Surface System Aggregator Module");
If you are fine with per a module WQ, please apply this patch to your tree. If you prefer per a "struct san_data" WQ like https://lkml.kernel.org/r/f78ddbdc-8989-a1a7-2234-ce9ec3894625@I-love.SAKURA.ne.jp does, you can replace this patch with yours. On 2022/06/10 20:29, Maximilian Luz wrote: > On 6/10/22 07:41, Tetsuo Handa wrote: >> Use local wq in order to avoid flush_scheduled_work() usage. >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> --- >> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() >> using a macro") for background. >> >> This is a blind conversion, and is only compile tested. > > Looks good to me, thanks! > > Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> > Tested-by: Maximilian Luz <luzmaximilian@gmail.com> > >> .../platform/surface/surface_acpi_notify.c | 27 ++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-)
On 6/17/22 12:40, Tetsuo Handa wrote: > If you are fine with per a module WQ, please apply this patch to your tree. > > If you prefer per a "struct san_data" WQ like > https://lkml.kernel.org/r/f78ddbdc-8989-a1a7-2234-ce9ec3894625@I-love.SAKURA.ne.jp > does, you can replace this patch with yours. I think a per-module workqueue is perfectly adequate, unless there are some considerations I'm missing. We generally expect there to only ever be one device instance present that this driver binds to, and even if there were multiple instances, it's fairly low-use and we only flush in the remove function (this is to say that I see virtually no potential for this to live-lock or something similar). I guess one could make the case that having it in the module as opposed to the driver may lead to an unused workqueue if the module is loaded (or built in) but the driver is never used. However, generally the driver should be built as module (unless you're very specifically targeting Surface devices) and the module is only ever loaded if the driver will actually be used. So, in the end, both ways should lead to virtually the same result. And I don't really have any strong preferences either way. > On 2022/06/10 20:29, Maximilian Luz wrote: >> On 6/10/22 07:41, Tetsuo Handa wrote: >>> Use local wq in order to avoid flush_scheduled_work() usage. >>> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> --- >>> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() >>> using a macro") for background. >>> >>> This is a blind conversion, and is only compile tested. >> >> Looks good to me, thanks! >> >> Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> >> Tested-by: Maximilian Luz <luzmaximilian@gmail.com> >> >>> .../platform/surface/surface_acpi_notify.c | 27 ++++++++++++++++--- >>> 1 file changed, 24 insertions(+), 3 deletions(-)
Hi, On 6/10/22 13:29, Maximilian Luz wrote: > On 6/10/22 07:41, Tetsuo Handa wrote: >> Use local wq in order to avoid flush_scheduled_work() usage. >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> --- >> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() >> using a macro") for background. >> >> This is a blind conversion, and is only compile tested. > > Looks good to me, thanks! > > Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> > Tested-by: Maximilian Luz <luzmaximilian@gmail.com> Thanks. Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > >> .../platform/surface/surface_acpi_notify.c | 27 ++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c >> index 7b758f8cc137..c0e12f0b9b79 100644 >> --- a/drivers/platform/surface/surface_acpi_notify.c >> +++ b/drivers/platform/surface/surface_acpi_notify.c >> @@ -37,6 +37,7 @@ struct san_data { >> #define to_san_data(ptr, member) \ >> container_of(ptr, struct san_data, member) >> +static struct workqueue_struct *san_wq; >> /* -- dGPU notifier interface. ---------------------------------------------- */ >> @@ -356,7 +357,7 @@ static u32 san_evt_bat_nf(struct ssam_event_notifier *nf, >> memcpy(&work->event, event, sizeof(struct ssam_event) + event->length); >> - schedule_delayed_work(&work->work, delay); >> + queue_delayed_work(san_wq, &work->work, delay); >> return SSAM_NOTIF_HANDLED; >> } >> @@ -861,7 +862,7 @@ static int san_remove(struct platform_device *pdev) >> * We have unregistered our event sources. Now we need to ensure that >> * all delayed works they may have spawned are run to completion. >> */ >> - flush_scheduled_work(); >> + flush_workqueue(san_wq); >> return 0; >> } >> @@ -881,7 +882,27 @@ static struct platform_driver surface_acpi_notify = { >> .probe_type = PROBE_PREFER_ASYNCHRONOUS, >> }, >> }; >> -module_platform_driver(surface_acpi_notify); >> + >> +static int __init san_init(void) >> +{ >> + int ret; >> + >> + san_wq = alloc_workqueue("san_wq", 0, 0); >> + if (!san_wq) >> + return -ENOMEM; >> + ret = platform_driver_register(&surface_acpi_notify); >> + if (ret) >> + destroy_workqueue(san_wq); >> + return ret; >> +} >> +module_init(san_init); >> + >> +static void __exit san_exit(void) >> +{ >> + platform_driver_unregister(&surface_acpi_notify); >> + destroy_workqueue(san_wq); >> +} >> +module_exit(san_exit); >> MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>"); >> MODULE_DESCRIPTION("Surface ACPI Notify driver for Surface System Aggregator Module"); >
diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c index 7b758f8cc137..c0e12f0b9b79 100644 --- a/drivers/platform/surface/surface_acpi_notify.c +++ b/drivers/platform/surface/surface_acpi_notify.c @@ -37,6 +37,7 @@ struct san_data { #define to_san_data(ptr, member) \ container_of(ptr, struct san_data, member) +static struct workqueue_struct *san_wq; /* -- dGPU notifier interface. ---------------------------------------------- */ @@ -356,7 +357,7 @@ static u32 san_evt_bat_nf(struct ssam_event_notifier *nf, memcpy(&work->event, event, sizeof(struct ssam_event) + event->length); - schedule_delayed_work(&work->work, delay); + queue_delayed_work(san_wq, &work->work, delay); return SSAM_NOTIF_HANDLED; } @@ -861,7 +862,7 @@ static int san_remove(struct platform_device *pdev) * We have unregistered our event sources. Now we need to ensure that * all delayed works they may have spawned are run to completion. */ - flush_scheduled_work(); + flush_workqueue(san_wq); return 0; } @@ -881,7 +882,27 @@ static struct platform_driver surface_acpi_notify = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, }; -module_platform_driver(surface_acpi_notify); + +static int __init san_init(void) +{ + int ret; + + san_wq = alloc_workqueue("san_wq", 0, 0); + if (!san_wq) + return -ENOMEM; + ret = platform_driver_register(&surface_acpi_notify); + if (ret) + destroy_workqueue(san_wq); + return ret; +} +module_init(san_init); + +static void __exit san_exit(void) +{ + platform_driver_unregister(&surface_acpi_notify); + destroy_workqueue(san_wq); +} +module_exit(san_exit); MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>"); MODULE_DESCRIPTION("Surface ACPI Notify driver for Surface System Aggregator Module");
Use local wq in order to avoid flush_scheduled_work() usage. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") for background. This is a blind conversion, and is only compile tested. .../platform/surface/surface_acpi_notify.c | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)