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

[Tetsuo Handa]

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@xxxxxxxxxxxxxxx .
>>>
>>> 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@xxxxxxxxx ,
> 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()").