[PATCH] platform/surface: avoid flush_scheduled_work() usage

[PATCH] platform/surface: avoid flush_scheduled_work() usage

[Tetsuo Handa]

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(-)

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");
-- 
2.18.4

Re: [PATCH] platform/surface: avoid flush_scheduled_work() usage

[Maximilian Luz]

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");

Re: [PATCH] platform/surface: avoid flush_scheduled_work() usage

[Tetsuo Handa]

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(-)

Re: [PATCH] platform/surface: avoid flush_scheduled_work() usage

[Maximilian Luz]

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(-)

Re: [PATCH] platform/surface: avoid flush_scheduled_work() usage

[Hans de Goede]

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");
>