[PATCH v2 (REPOST)] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().

[PATCH v2 (REPOST)] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().

[Tetsuo Handa]

Since "struct tsi721_device" is per a device struct, I assume that
tsi721_remove() needs to wait for only two works associated with that
device. Therefore, wait for only these works using flush_work().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
  Use flush_work() instead of introducing a dedicated WQ.

Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
using a macro") for background.

 drivers/rapidio/devices/tsi721.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rapidio/devices/tsi721.c b/drivers/rapidio/devices/tsi721.c
index b3134744fb55..0a42d6a2af24 100644
--- a/drivers/rapidio/devices/tsi721.c
+++ b/drivers/rapidio/devices/tsi721.c
@@ -2941,7 +2941,8 @@ static void tsi721_remove(struct pci_dev *pdev)
 
 	tsi721_disable_ints(priv);
 	tsi721_free_irq(priv);
-	flush_scheduled_work();
+	flush_work(&priv->idb_work);
+	flush_work(&priv->pw_work);
 	rio_unregister_mport(&priv->mport);
 
 	tsi721_unregister_dma(priv);
-- 
2.18.4

Re: [PATCH v2 (REPOST)] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().

[Andrew Morton]

On Thu, 28 Jul 2022 10:02:13 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> Since "struct tsi721_device" is per a device struct, I assume that
> tsi721_remove() needs to wait for only two works associated with that
> device. Therefore, wait for only these works using flush_work().

The changelog provides no reason for making this change.  Correctness?
Efficiency?

Re: [PATCH v2 (REPOST)] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().

[Tetsuo Handa]

On 2022/08/15 9:14, Andrew Morton wrote:
> On Thu, 28 Jul 2022 10:02:13 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
>> Since "struct tsi721_device" is per a device struct, I assume that
>> tsi721_remove() needs to wait for only two works associated with that
>> device. Therefore, wait for only these works using flush_work().
> 
> The changelog provides no reason for making this change.  Correctness?
> Efficiency?

For reducing locking dependency chains that might lead to deadlock.

The reason was explained in commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
using a macro"). For example, after the fact it turned out that a blind conversion patch
fixed a deadlock problem at https://syzkaller.appspot.com/bug?extid=2d6ac90723742279e101
which was hidden due to use of lockdep_set_novalidate_class() call.

Many of flush_scheduled_work() callers have been removed by v6.0-rc1, but not all
patches were verbose (because the reason was already explained).

  a1124c84d467 power: supply: ab8500: Remove flush_scheduled_work() call.
  162b05524ed3 rtc: Replace flush_scheduled_work() with flush_work().
  31a1e4a5c104 platform/surface: avoid flush_scheduled_work() usage
  90c3ca3f247d scsi: mpt3sas: Remove flush_scheduled_work() call
  62ebaf2f9261 ath6kl: avoid flush_scheduled_work() usage
  76faa32077b0 iio: light: tsl2563: Replace flush_scheduled_work() with cancel_delayed_work_sync().
  4bbdc208a5ff staging: olpc_dcon: Replace flush_scheduled_work() with flush_work().
  c4f135d64382 workqueue: Wrap flush_workqueue() using a macro
  9cf62d91e4b7 RDMA/mlx4: Avoid flush_scheduled_work() usage
  549f39a58acf IB/isert: Avoid flush_scheduled_work() usage
  1b3ce51dde36 Input: psmouse-smbus - avoid flush_scheduled_work() usage
  0a2f4b5785ca crypto: atmel - Avoid flush_scheduled_work() usage
  eeff214dbfcb wfx: avoid flush_workqueue(system_highpri_wq) usage
  0b8d7622ab18 aoe: Avoid flush_scheduled_work() usage
  cc271ab86606 wwan_hwsim: Avoid flush_scheduled_work() usage
  ff815a89398d RDMA/core: Avoid flush_workqueue(system_unbound_wq) usage

There are 7 callers remaining as of v6.0-rc1, and I'm pinging them for heads up.

  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
  drivers/gpu/drm/i915/display/intel_display.c
  drivers/gpu/drm/i915/gt/selftest_execlists.c
  drivers/md/dm.c
  drivers/message/fusion/mptscsih.c
  drivers/rapidio/devices/tsi721.c
  drivers/scsi/qla2xxx/qla_target.c

If you want to update patch description, you can insert the following.

----------------------------------------

Like commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a
macro") says, flush_scheduled_work() is dangerous and will be forbidden.
We are on the way for removing all flush_scheduled_work() callers from
the kernel, and this patch is for removing flush_scheduled_work() call
 from tsi721 driver.

Re: [PATCH v2 (REPOST)] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().

[Tetsuo Handa]

Matt and Alexandre, what can I do on this patch?

On 2022/08/15 18:47, Tetsuo Handa wrote:
> On 2022/08/15 9:14, Andrew Morton wrote:
>> On Thu, 28 Jul 2022 10:02:13 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
>>
>>> Since "struct tsi721_device" is per a device struct, I assume that
>>> tsi721_remove() needs to wait for only two works associated with that
>>> device. Therefore, wait for only these works using flush_work().
>>
>> The changelog provides no reason for making this change.  Correctness?
>> Efficiency?
> 
> For reducing locking dependency chains that might lead to deadlock.
> 
> The reason was explained in commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
> using a macro"). For example, after the fact it turned out that a blind conversion patch
> fixed a deadlock problem at https://syzkaller.appspot.com/bug?extid=2d6ac90723742279e101
> which was hidden due to use of lockdep_set_novalidate_class() call.
> 
> Many of flush_scheduled_work() callers have been removed by v6.0-rc1, but not all
> patches were verbose (because the reason was already explained).
> 
>   a1124c84d467 power: supply: ab8500: Remove flush_scheduled_work() call.
>   162b05524ed3 rtc: Replace flush_scheduled_work() with flush_work().
>   31a1e4a5c104 platform/surface: avoid flush_scheduled_work() usage
>   90c3ca3f247d scsi: mpt3sas: Remove flush_scheduled_work() call
>   62ebaf2f9261 ath6kl: avoid flush_scheduled_work() usage
>   76faa32077b0 iio: light: tsl2563: Replace flush_scheduled_work() with cancel_delayed_work_sync().
>   4bbdc208a5ff staging: olpc_dcon: Replace flush_scheduled_work() with flush_work().
>   c4f135d64382 workqueue: Wrap flush_workqueue() using a macro
>   9cf62d91e4b7 RDMA/mlx4: Avoid flush_scheduled_work() usage
>   549f39a58acf IB/isert: Avoid flush_scheduled_work() usage
>   1b3ce51dde36 Input: psmouse-smbus - avoid flush_scheduled_work() usage
>   0a2f4b5785ca crypto: atmel - Avoid flush_scheduled_work() usage
>   eeff214dbfcb wfx: avoid flush_workqueue(system_highpri_wq) usage
>   0b8d7622ab18 aoe: Avoid flush_scheduled_work() usage
>   cc271ab86606 wwan_hwsim: Avoid flush_scheduled_work() usage
>   ff815a89398d RDMA/core: Avoid flush_workqueue(system_unbound_wq) usage
> 
> There are 7 callers remaining as of v6.0-rc1, and I'm pinging them for heads up.
> 
>   drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>   drivers/gpu/drm/i915/display/intel_display.c
>   drivers/gpu/drm/i915/gt/selftest_execlists.c
>   drivers/md/dm.c
>   drivers/message/fusion/mptscsih.c
>   drivers/rapidio/devices/tsi721.c
>   drivers/scsi/qla2xxx/qla_target.c
> 
> If you want to update patch description, you can insert the following.
> 
> ----------------------------------------
> 
> Like commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a
> macro") says, flush_scheduled_work() is dangerous and will be forbidden.
> We are on the way for removing all flush_scheduled_work() callers from
> the kernel, and this patch is for removing flush_scheduled_work() call
>  from tsi721 driver.
>