* [PATCH] ACPI: Avoid flush_scheduled_work() usage
@ 2022-04-19 13:57 Tetsuo Handa
2022-04-19 14:48 ` Rafael J. Wysocki
2022-04-20 2:02 ` kernel test robot
0 siblings, 2 replies; 4+ messages in thread
From: Tetsuo Handa @ 2022-04-19 13:57 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, James Morse, Tony Luck,
Borislav Petkov
Cc: linux-acpi
Flushing system-wide workqueues is dangerous and will be forbidden.
Replace system_wq with local acpi_wq.
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>
---
This patch blindly converts schedule_{,rcu_}work() into queue_{,rcu_}work()
within drivers/acpi/ directory, based on an assumption that none of work items
outside of drivers/acpi/ directory needs to be handled by acpi_wq.
Also, I kept sharing acpi_wq between acpi/ and acpi/apei, based on assumption
that sharing is safe and correct behavior. Did I convert correctly?
drivers/acpi/acpi_video.c | 2 +-
drivers/acpi/apei/apei-internal.h | 1 +
drivers/acpi/apei/ghes.c | 2 +-
drivers/acpi/bus.c | 2 +-
drivers/acpi/internal.h | 1 +
drivers/acpi/osl.c | 6 +++++-
drivers/acpi/scan.c | 2 +-
drivers/acpi/video_detect.c | 2 +-
8 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 990ff5b0aeb8..18a5b8dd766e 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1637,7 +1637,7 @@ static void brightness_switch_event(struct acpi_video_device *video_device,
return;
video_device->switch_brightness_event = event;
- schedule_delayed_work(&video_device->switch_brightness_work, HZ / 10);
+ queue_delayed_work(acpi_wq, &video_device->switch_brightness_work, HZ / 10);
}
static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index 1d6ef9654725..205dcacf8abf 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -136,4 +136,5 @@ int cper_estatus_check_header(const struct acpi_hest_generic_status *estatus);
int cper_estatus_check(const struct acpi_hest_generic_status *estatus);
int apei_osc_setup(void);
+extern struct workqueue_struct *acpi_wq;
#endif
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d91ad378c00d..871b5f6c2b45 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -619,7 +619,7 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
entry->error_severity = sev;
INIT_WORK(&entry->work, ghes_vendor_record_work_func);
- schedule_work(&entry->work);
+ queue_work(acpi_wq, &entry->work);
}
static bool ghes_do_proc(struct ghes *ghes,
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index e807bffc0804..7dae7f884187 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -608,7 +608,7 @@ static void acpi_sb_notify(acpi_handle handle, u32 event, void *data)
if (event == ACPI_SB_NOTIFY_SHUTDOWN_REQUEST) {
if (!work_busy(&acpi_sb_work))
- schedule_work(&acpi_sb_work);
+ queue_work(acpi_wq, &acpi_sb_work);
} else
pr_warn("event %x is not supported by \\_SB device\n", event);
}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 628bf8f18130..5f77c7c573a6 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -77,6 +77,7 @@ void acpi_lpss_init(void);
#else
static inline void acpi_lpss_init(void) {}
#endif
+extern struct workqueue_struct *acpi_wq;
void acpi_apd_init(void);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 7a70c4bfc23c..1725125890cd 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -64,6 +64,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
static acpi_osd_handler acpi_irq_handler;
static void *acpi_irq_context;
+struct workqueue_struct *acpi_wq;
static struct workqueue_struct *kacpid_wq;
static struct workqueue_struct *kacpi_notify_wq;
static struct workqueue_struct *kacpi_hotplug_wq;
@@ -1575,7 +1576,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
*/
synchronize_rcu();
rcu_barrier();
- flush_scheduled_work();
+ flush_workqueue(acpi_wq);
return AE_OK;
}
@@ -1755,9 +1756,11 @@ acpi_status __init acpi_os_initialize(void)
acpi_status __init acpi_os_initialize1(void)
{
+ acpi_wq = alloc_workqueue("acpi", 0, 0);
kacpid_wq = alloc_workqueue("kacpid", 0, 1);
kacpi_notify_wq = alloc_workqueue("kacpi_notify", 0, 1);
kacpi_hotplug_wq = alloc_ordered_workqueue("kacpi_hotplug", 0);
+ BUG_ON(!acpi_wq);
BUG_ON(!kacpid_wq);
BUG_ON(!kacpi_notify_wq);
BUG_ON(!kacpi_hotplug_wq);
@@ -1786,6 +1789,7 @@ acpi_status acpi_os_terminate(void)
destroy_workqueue(kacpid_wq);
destroy_workqueue(kacpi_notify_wq);
destroy_workqueue(kacpi_hotplug_wq);
+ destroy_workqueue(acpi_wq);
return AE_OK;
}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 762b61f67e6c..f16aaec445f2 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2676,7 +2676,7 @@ void acpi_scan_table_notify(void)
return;
INIT_WORK(work, acpi_table_events_fn);
- schedule_work(work);
+ queue_work(acpi_wq, work);
}
int acpi_reconfig_notifier_register(struct notifier_block *nb)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..441664275645 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -529,7 +529,7 @@ static int acpi_video_backlight_notify(struct notifier_block *nb,
/* A raw bl registering may change video -> native */
if (backlight->props.type == BACKLIGHT_RAW &&
val == BACKLIGHT_REGISTERED)
- schedule_work(&backlight_notify_work);
+ queue_work(acpi_wq, &backlight_notify_work);
return NOTIFY_OK;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] ACPI: Avoid flush_scheduled_work() usage
2022-04-19 13:57 [PATCH] ACPI: Avoid flush_scheduled_work() usage Tetsuo Handa
@ 2022-04-19 14:48 ` Rafael J. Wysocki
2022-04-19 15:19 ` Tetsuo Handa
2022-04-20 2:02 ` kernel test robot
1 sibling, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2022-04-19 14:48 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Rafael J. Wysocki, Len Brown, James Morse, Tony Luck,
Borislav Petkov, ACPI Devel Maling List
On Tue, Apr 19, 2022 at 3:57 PM 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 acpi_wq.
>
> 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>
> ---
> This patch blindly converts schedule_{,rcu_}work() into queue_{,rcu_}work()
> within drivers/acpi/ directory, based on an assumption that none of work items
> outside of drivers/acpi/ directory needs to be handled by acpi_wq.
> Also, I kept sharing acpi_wq between acpi/ and acpi/apei, based on assumption
> that sharing is safe and correct behavior. Did I convert correctly?
Please don't do it this way.
There is not much sense in sharing a dedicated workqueue between the
different pieces of code below.
I guess that there is a need to switch over to dedicated workqueues in
all cases when the workqueue needs to be flushed directly. If so,
please let me look at what can be done.
>
> drivers/acpi/acpi_video.c | 2 +-
> drivers/acpi/apei/apei-internal.h | 1 +
> drivers/acpi/apei/ghes.c | 2 +-
> drivers/acpi/bus.c | 2 +-
> drivers/acpi/internal.h | 1 +
> drivers/acpi/osl.c | 6 +++++-
> drivers/acpi/scan.c | 2 +-
> drivers/acpi/video_detect.c | 2 +-
> 8 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 990ff5b0aeb8..18a5b8dd766e 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -1637,7 +1637,7 @@ static void brightness_switch_event(struct acpi_video_device *video_device,
> return;
>
> video_device->switch_brightness_event = event;
> - schedule_delayed_work(&video_device->switch_brightness_work, HZ / 10);
> + queue_delayed_work(acpi_wq, &video_device->switch_brightness_work, HZ / 10);
> }
>
> static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index 1d6ef9654725..205dcacf8abf 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -136,4 +136,5 @@ int cper_estatus_check_header(const struct acpi_hest_generic_status *estatus);
> int cper_estatus_check(const struct acpi_hest_generic_status *estatus);
>
> int apei_osc_setup(void);
> +extern struct workqueue_struct *acpi_wq;
> #endif
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index d91ad378c00d..871b5f6c2b45 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -619,7 +619,7 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
> entry->error_severity = sev;
>
> INIT_WORK(&entry->work, ghes_vendor_record_work_func);
> - schedule_work(&entry->work);
> + queue_work(acpi_wq, &entry->work);
> }
>
> static bool ghes_do_proc(struct ghes *ghes,
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index e807bffc0804..7dae7f884187 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -608,7 +608,7 @@ static void acpi_sb_notify(acpi_handle handle, u32 event, void *data)
>
> if (event == ACPI_SB_NOTIFY_SHUTDOWN_REQUEST) {
> if (!work_busy(&acpi_sb_work))
> - schedule_work(&acpi_sb_work);
> + queue_work(acpi_wq, &acpi_sb_work);
> } else
> pr_warn("event %x is not supported by \\_SB device\n", event);
> }
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 628bf8f18130..5f77c7c573a6 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -77,6 +77,7 @@ void acpi_lpss_init(void);
> #else
> static inline void acpi_lpss_init(void) {}
> #endif
> +extern struct workqueue_struct *acpi_wq;
>
> void acpi_apd_init(void);
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 7a70c4bfc23c..1725125890cd 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -64,6 +64,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>
> static acpi_osd_handler acpi_irq_handler;
> static void *acpi_irq_context;
> +struct workqueue_struct *acpi_wq;
> static struct workqueue_struct *kacpid_wq;
> static struct workqueue_struct *kacpi_notify_wq;
> static struct workqueue_struct *kacpi_hotplug_wq;
> @@ -1575,7 +1576,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
> */
> synchronize_rcu();
> rcu_barrier();
> - flush_scheduled_work();
> + flush_workqueue(acpi_wq);
>
> return AE_OK;
> }
> @@ -1755,9 +1756,11 @@ acpi_status __init acpi_os_initialize(void)
>
> acpi_status __init acpi_os_initialize1(void)
> {
> + acpi_wq = alloc_workqueue("acpi", 0, 0);
> kacpid_wq = alloc_workqueue("kacpid", 0, 1);
> kacpi_notify_wq = alloc_workqueue("kacpi_notify", 0, 1);
> kacpi_hotplug_wq = alloc_ordered_workqueue("kacpi_hotplug", 0);
> + BUG_ON(!acpi_wq);
> BUG_ON(!kacpid_wq);
> BUG_ON(!kacpi_notify_wq);
> BUG_ON(!kacpi_hotplug_wq);
> @@ -1786,6 +1789,7 @@ acpi_status acpi_os_terminate(void)
> destroy_workqueue(kacpid_wq);
> destroy_workqueue(kacpi_notify_wq);
> destroy_workqueue(kacpi_hotplug_wq);
> + destroy_workqueue(acpi_wq);
>
> return AE_OK;
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 762b61f67e6c..f16aaec445f2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2676,7 +2676,7 @@ void acpi_scan_table_notify(void)
> return;
>
> INIT_WORK(work, acpi_table_events_fn);
> - schedule_work(work);
> + queue_work(acpi_wq, work);
> }
>
> int acpi_reconfig_notifier_register(struct notifier_block *nb)
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index becc198e4c22..441664275645 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -529,7 +529,7 @@ static int acpi_video_backlight_notify(struct notifier_block *nb,
> /* A raw bl registering may change video -> native */
> if (backlight->props.type == BACKLIGHT_RAW &&
> val == BACKLIGHT_REGISTERED)
> - schedule_work(&backlight_notify_work);
> + queue_work(acpi_wq, &backlight_notify_work);
>
> return NOTIFY_OK;
> }
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ACPI: Avoid flush_scheduled_work() usage
2022-04-19 14:48 ` Rafael J. Wysocki
@ 2022-04-19 15:19 ` Tetsuo Handa
0 siblings, 0 replies; 4+ messages in thread
From: Tetsuo Handa @ 2022-04-19 15:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, James Morse, Tony Luck, Borislav Petkov,
ACPI Devel Maling List
On 2022/04/19 23:48, Rafael J. Wysocki wrote:
> On Tue, Apr 19, 2022 at 3:57 PM 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 acpi_wq.
>>
>> 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>
>> ---
>> This patch blindly converts schedule_{,rcu_}work() into queue_{,rcu_}work()
>> within drivers/acpi/ directory, based on an assumption that none of work items
>> outside of drivers/acpi/ directory needs to be handled by acpi_wq.
>> Also, I kept sharing acpi_wq between acpi/ and acpi/apei, based on assumption
>> that sharing is safe and correct behavior. Did I convert correctly?
>
> Please don't do it this way.
>
> There is not much sense in sharing a dedicated workqueue between the
> different pieces of code below.
>
> I guess that there is a need to switch over to dedicated workqueues in
> all cases when the workqueue needs to be flushed directly. If so,
> please let me look at what can be done.
>
Since system_wq is shared throughout the kernel, calling flush_scheduled_work()
might cause deadlock. In order to avoid possibility of deadlock, there is a need to
use a dedicated workqueue in all cases when some work is subjected to flush_workqueue().
This is one of patches for avoiding flush_scheduled_work() usage, but I'm not familiar
with ACPI code. Thus, please introduce a dedicated workqueue in a way you think makes
sense.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ACPI: Avoid flush_scheduled_work() usage
2022-04-19 13:57 [PATCH] ACPI: Avoid flush_scheduled_work() usage Tetsuo Handa
2022-04-19 14:48 ` Rafael J. Wysocki
@ 2022-04-20 2:02 ` kernel test robot
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-04-20 2:02 UTC (permalink / raw)
To: Tetsuo Handa, Rafael J. Wysocki, Len Brown, James Morse,
Tony Luck, Borislav Petkov
Cc: llvm, kbuild-all, linux-acpi
Hi Tetsuo,
I love your patch! Yet something to improve:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on v5.18-rc3 next-20220419]
[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]
url: https://github.com/intel-lab-lkp/linux/commits/Tetsuo-Handa/ACPI-Avoid-flush_scheduled_work-usage/20220419-215854
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220420/202204200917.Gi0wjcgM-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c1c49a356162b22554088d269f7689bdb044a9f1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/cebc6c01f6fac86f7deaf74ca81311d9c3d179f4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Tetsuo-Handa/ACPI-Avoid-flush_scheduled_work-usage/20220419-215854
git checkout cebc6c01f6fac86f7deaf74ca81311d9c3d179f4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/acpi/acpi_video.c:1640:21: error: use of undeclared identifier 'acpi_wq'
queue_delayed_work(acpi_wq, &video_device->switch_brightness_work, HZ / 10);
^
drivers/acpi/acpi_video.c:2258:6: warning: no previous prototype for function 'acpi_video_unregister_backlight' [-Wmissing-prototypes]
void acpi_video_unregister_backlight(void)
^
drivers/acpi/acpi_video.c:2258:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void acpi_video_unregister_backlight(void)
^
static
1 warning and 1 error generated.
--
>> drivers/acpi/video_detect.c:532:14: error: use of undeclared identifier 'acpi_wq'
queue_work(acpi_wq, &backlight_notify_work);
^
drivers/acpi/video_detect.c:605:13: warning: no previous prototype for function 'acpi_video_detect_exit' [-Wmissing-prototypes]
void __exit acpi_video_detect_exit(void)
^
drivers/acpi/video_detect.c:605:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void __exit acpi_video_detect_exit(void)
^
static
1 warning and 1 error generated.
vim +/acpi_wq +1640 drivers/acpi/acpi_video.c
1632
1633 static void brightness_switch_event(struct acpi_video_device *video_device,
1634 u32 event)
1635 {
1636 if (!brightness_switch_enabled)
1637 return;
1638
1639 video_device->switch_brightness_event = event;
> 1640 queue_delayed_work(acpi_wq, &video_device->switch_brightness_work, HZ / 10);
1641 }
1642
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-20 2:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-19 13:57 [PATCH] ACPI: Avoid flush_scheduled_work() usage Tetsuo Handa
2022-04-19 14:48 ` Rafael J. Wysocki
2022-04-19 15:19 ` Tetsuo Handa
2022-04-20 2:02 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox