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.

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>
---
Since no response from RAPIDIO SUBSYSTEM maintainers for more than three months (
https://lkml.kernel.org/r/ad924306-d15c-9f1e-13dc-eaf674878022@I-love.SAKURA.ne.jp
https://lkml.kernel.org/r/8347375e-29fd-cd45-2b07-5504c9ddbd0c@I-love.SAKURA.ne.jp
), I route this patch to Andrew Morton.

Changes in v3:
  Update description.

Changes in v2:
  Use flush_work() instead of introducing a dedicated WQ.

 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

On Sat, 24 Sep 2022 14:11:25 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

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

Why not use cancel_work[_sync](), as the flush_scheduled_work() comment
recommends?

On 2022/09/26 2:27, Andrew Morton wrote:
> On Sat, 24 Sep 2022 14:11:25 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
>> 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.
>>
>> 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().
>>
>> --- 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);
> 
> Why not use cancel_work[_sync](), as the flush_scheduled_work() comment
> recommends?
> 

Alan Stern suggested to use cancel_work_sync() in
commit eef6a7d5c2f38ada ("workqueue: warn about flush_scheduled_work()")
and Tejun Heo suggested to use flush_work() in
https://lkml.kernel.org/r/YjivtdkpY+reW0Gt@slm.duckdns.org .

Is there some reason to prefer one over the other?
I think that user-visible results between flush_work() and cancel_work_sync()
are the same because both wait until work completes.

On Mon, Sep 26, 2022 at 07:28:37PM +0900, Tetsuo Handa wrote:
> On 2022/09/26 2:27, Andrew Morton wrote:
> > On Sat, 24 Sep 2022 14:11:25 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > 
> >> 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.
> >>
> >> 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().
> >>
> >> --- 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);
> > 
> > Why not use cancel_work[_sync](), as the flush_scheduled_work() comment
> > recommends?
> > 
> 
> Alan Stern suggested to use cancel_work_sync() in
> commit eef6a7d5c2f38ada ("workqueue: warn about flush_scheduled_work()")
> and Tejun Heo suggested to use flush_work() in
> https://lkml.kernel.org/r/YjivtdkpY+reW0Gt@slm.duckdns.org .
> 
> Is there some reason to prefer one over the other?
> I think that user-visible results between flush_work() and cancel_work_sync()
> are the same because both wait until work completes.

No, you haven't got it quite right.  flush_work() waits until the work 
completes, but cancel_work_sync() first tries to cancel the work item.  
It then waits until the work item is either cancelled or completed.

If the cancellation is successful (i.e., it happens before the work item 
starts to run) then the call will return at that time and the work item 
will never run -- hence it will never complete.

Alan Stern

On 2022/09/27 0:06, Alan Stern wrote:
>> Alan Stern suggested to use cancel_work_sync() in
>> commit eef6a7d5c2f38ada ("workqueue: warn about flush_scheduled_work()")
>> and Tejun Heo suggested to use flush_work() in
>> https://lkml.kernel.org/r/YjivtdkpY+reW0Gt@slm.duckdns.org .
>>
>> Is there some reason to prefer one over the other?
>> I think that user-visible results between flush_work() and cancel_work_sync()
>> are the same because both wait until work completes.
> 
> No, you haven't got it quite right.  flush_work() waits until the work 
> completes, but cancel_work_sync() first tries to cancel the work item.  
> It then waits until the work item is either cancelled or completed.

I know there is a difference if the cancellation was successful.
But unless cancel_work_sync() is called immediately after schedule_work(),
that work likely (e.g. 99%+) already started running or already completed.

> 
> If the cancellation is successful (i.e., it happens before the work item 
> starts to run) then the call will return at that time and the work item 
> will never run -- hence it will never complete.

A difficult to judge thing is whether the owner/maintainer of that code wants
that work completed or cancelled.
Unlike e.g. https://lkml.kernel.org/r/Yy3byxFrfAfQL9xK@intel.com ,
tsi721_remove() does not say whether pending works should run.

On 2022/09/27 7:13, Tetsuo Handa wrote:
> On 2022/09/27 0:06, Alan Stern wrote:
>>> Alan Stern suggested to use cancel_work_sync() in
>>> commit eef6a7d5c2f38ada ("workqueue: warn about flush_scheduled_work()")
>>> and Tejun Heo suggested to use flush_work() in
>>> https://lkml.kernel.org/r/YjivtdkpY+reW0Gt@slm.duckdns.org .
>>>
>>> Is there some reason to prefer one over the other?
>>> I think that user-visible results between flush_work() and cancel_work_sync()
>>> are the same because both wait until work completes.
>>
>> No, you haven't got it quite right.  flush_work() waits until the work 
>> completes, but cancel_work_sync() first tries to cancel the work item.  
>> It then waits until the work item is either cancelled or completed.
> 
> I know there is a difference if the cancellation was successful.
> But unless cancel_work_sync() is called immediately after schedule_work(),
> that work likely (e.g. 99%+) already started running or already completed.
> 
>>
>> If the cancellation is successful (i.e., it happens before the work item 
>> starts to run) then the call will return at that time and the work item 
>> will never run -- hence it will never complete.
> 
> A difficult to judge thing is whether the owner/maintainer of that code wants
> that work completed or cancelled.
> Unlike e.g. https://lkml.kernel.org/r/Yy3byxFrfAfQL9xK@intel.com ,
> tsi721_remove() does not say whether pending works should run.
> 

It seems that Tejun is too busy to respond.

Although it is a bit worrisome that tsi721_db_dpc() unconditionally re-enables
IDB interrupts when tsi721_remove() wants to disable all device interrupts (I
don't know behavior/specification of tsi721 hardware), I think it is OK to go
with flush_work() (as with other patches which removed flush_scheduled_work()
usage) because flush_work() matches the behavior of flush_scheduled_work().

It is maintainer's responsibility to fix if re-enabling IDB interrupts is not safe,
using a trick explained in e.g. commit a91b750fd6629354 ("net: rds: don't hold sock
lock when cancelling work from rds_tcp_reset_callbacks()").