ACPI: Avoid flush_scheduled_work() usage

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

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
>

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.

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