Re: [PATCH 0/2] firewire: core: optimize for concurrent calls of fw_iso_context_flush_completions()

[Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>]

Hi,

On Mon, Sep 09, 2024 at 11:00:16PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> It seems to be the last week for v6.12 development. I realize it
> unpreferable to propose intrusive changes, however I also realized that
> there is a room to refactor core functions in respect to handler of work
> item for isochronous context for the next merge window...
> 
> This series of changes refactors the core function to call
> fw_iso_context_flush_completions() from the work item. It optimizes some
> event waiting and mediation of concurrent calls as well.
> 
> Takashi Sakamoto (2):
>   firewire: core: move workqueue handler from 1394 OHCI driver to core
>     function
>   firewire: core: use mutex to coordinate concurrent calls to flush
>     completions
> 
>  drivers/firewire/core-iso.c | 31 ++++++++-------
>  drivers/firewire/core.h     |  5 ---
>  drivers/firewire/ohci.c     | 78 +++++++------------------------------
>  include/linux/firewire.h    |  1 +
>  4 files changed, 31 insertions(+), 84 deletions(-)

I realized that the above changes have unpreferable effects to the behaviour
for user space interface. The changes allow to call the handler of
isochronous context again to drain the rest of packet buffer after calling
the handler at first due to processing the interrupt flag of 1394 OHCI IT/IR
descriptor. As a result, it is possible to enqueue two iso_interrupt events
for user space applications in the bottom half of hardIRQ. However, this is
against the description in UAPI header:

```
$ cat include/uapi/linux/firewire-cdev.h
...
 * struct fw_cdev_event_iso_interrupt - Sent when an iso packet was completed
...
 * This event is sent when the controller has completed an &fw_cdev_iso_packet
 * with the %FW_CDEV_ISO_INTERRUPT bit set, when explicitly requested with
 * %FW_CDEV_IOC_FLUSH_ISO, or when there have been so many completed packets
 * without the interrupt bit set that the kernel's internal buffer for @header
 * is about to overflow.  (In the last case, ABI versions < 5 drop header data
 * up to the next interrupt packet.)
```

As a bottom half of hardIRQ, the work item should enqueue a single event
associated to the interrupt event. The rest of packet buffer should be
handled in the bottom half of next hardIRQ unless in the path of
FW_CDEV_ISO_INTERRUPT.

Let me revert these changes later.


Regards

Takashi Sakamoto