* [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().
@ 2022-09-24 5:11 Tetsuo Handa
2022-09-25 17:27 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2022-09-24 5:11 UTC (permalink / raw)
To: Andrew Morton, LKML
Cc: Matt Porter, Alexandre Bounine, Christophe JAILLET, Arnd Bergmann
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().
2022-09-24 5:11 [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work() Tetsuo Handa
@ 2022-09-25 17:27 ` Andrew Morton
2022-09-26 10:28 ` Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2022-09-25 17:27 UTC (permalink / raw)
To: Tetsuo Handa
Cc: LKML, Matt Porter, Alexandre Bounine, Christophe JAILLET, Arnd Bergmann
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().
2022-09-25 17:27 ` Andrew Morton
@ 2022-09-26 10:28 ` Tetsuo Handa
2022-09-26 15:06 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2022-09-26 10:28 UTC (permalink / raw)
To: Alan Stern, Tejun Heo
Cc: LKML, Matt Porter, Alexandre Bounine, Christophe JAILLET,
Arnd Bergmann, Andrew Morton
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().
2022-09-26 10:28 ` Tetsuo Handa
@ 2022-09-26 15:06 ` Alan Stern
2022-09-26 22:13 ` Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2022-09-26 15:06 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Tejun Heo, LKML, Matt Porter, Alexandre Bounine,
Christophe JAILLET, Arnd Bergmann, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().
2022-09-26 15:06 ` Alan Stern
@ 2022-09-26 22:13 ` Tetsuo Handa
2022-10-10 10:16 ` Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2022-09-26 22:13 UTC (permalink / raw)
To: Alan Stern
Cc: Tejun Heo, LKML, Matt Porter, Alexandre Bounine,
Christophe JAILLET, Arnd Bergmann, Andrew Morton
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work().
2022-09-26 22:13 ` Tetsuo Handa
@ 2022-10-10 10:16 ` Tetsuo Handa
0 siblings, 0 replies; 6+ messages in thread
From: Tetsuo Handa @ 2022-10-10 10:16 UTC (permalink / raw)
To: Andrew Morton
Cc: Tejun Heo, LKML, Matt Porter, Alexandre Bounine,
Christophe JAILLET, Arnd Bergmann, Alan Stern
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()").
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-10 10:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-24 5:11 [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work() Tetsuo Handa
2022-09-25 17:27 ` Andrew Morton
2022-09-26 10:28 ` Tetsuo Handa
2022-09-26 15:06 ` Alan Stern
2022-09-26 22:13 ` Tetsuo Handa
2022-10-10 10:16 ` Tetsuo Handa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.