Re: [PATCH] firewire: Convert from tasklet to BH workqueue

[Takashi S. <o-t...@sa...>]

Hi,

Thanks for the patch. The replacement of tasklet with workqueue is one
of my TODO list, and the change would be helpful.

On Wed, Apr 03, 2024 at 02:45:58PM +0000, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
> 
> This patch converts drivers/firewire/* from tasklet to BH workqueue.
> 
> Based on the work done by Tejun Heo <tj...@ke...>
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git disable_work-v1
> 
> Changes are tested by: @recallmenot
> (https://github.com/allenpais/for-6.9-bh-conversions/issues/1)
> 
> Signed-off-by: Allen Pais <all...@gm...>
> ---
>  drivers/firewire/ohci.c | 54 ++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)

However, the changes look to be too early, since some kernel APIs
are referred from the change but locate just in Heo's tree. Thus,
any application of the patch brings build failure, like:

```
drivers/firewire/ohci.c: In function ‘at_context_flush’:
drivers/firewire/ohci.c:1463:9: error: implicit declaration of function ‘disable_work_sync’; did you mean ‘disable_irq_nosync’? [-Werror=implicit-function-declaration]
 1463 |         disable_work_sync(&ctx->work);
      |         ^~~~~~~~~~~~~~~~~
      |         disable_irq_nosync
drivers/firewire/ohci.c:1468:9: error: implicit declaration of function ‘enable_and_queue_work’ [-Werror=implicit-function-declaration]
 1468 |         enable_and_queue_work(system_bh_wq, &ctx->work);
      |         ^~~~~~~~~~~~~~~~~~~~~
```

In my humble opinion, the change proposal should be posted after merging
Heo's work, to prevent developers and users from being puzzled.
Furthermore, any kind of report for the performance test is preferable.

Especially, in FireWire subsystem, 1394 OHCI IT/IR contexts can be
processed by both tasklet and process (e.g. ioctl), thus the exclusive
control of workqueue for the contexts is important between them. I wish
it is done successfully by the new pair of enabling/disabling workqueue
API, and need more information about it.


Thanks

Takashi Sakamoto