[RFT,0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events

  Hi,

This series of change is inspired by BH workqueue available in recent
kernel.

In Linux FireWire subsystem, tasklet softIRQ context has been utilized to
operate 1394 OHCI Isochronous Transmit (IT) and Isochronous Receive (IR)
contexts. The tasklet context is not preferable, as you know.

I have already received a proposal[1][2] to replace the usage of tasklet
with BH workqueue. However, the proposal includes bare replacement for 1394
OHCI IT, IR, Asynchronous Transmit (AT), and Asynchronous Receive (AR)
contexts with neither any care of actual usage for each context nor
practical test reports. In theory, this kind of change should be done by
step by step with enough amount of evaluation over software design to avoid
any disorder.

In this series of changes, the usage of tasklet for 1394 OHCI IT/IR
contexts is just replaced, as a first step. In 1394 OHCI IR/IT events,
software is expected to process the content of page dedicated to DMA
transmission for each isochronous context. It means that the content can be
processed concurrently per isochronous context. Additionally, the content
of page is immutable as long as the software schedules the transmission
again for the page. It means that the task to process the content can sleep
or be preempted. Due to the characteristics, BH workqueue is _not_ used.

At present, 1394 OHCI driver is responsible for the maintenance of tasklet
context, while in this series of change the core function is responsible
for the maintenance of workqueue and work items. This change is an attempt
to let each implementation focus on own task.

The change affects the following implementations of unit protocols which
operate isochronous contexts:

- firewire-net for IP over 1394 (RFC 2734/3146)
- firedtv
- drivers in ALSA firewire stack for IEC 61883-1/6
- user space applications

As long as reading their codes, the first two drivers look to require no
change. While the drivers in ALSA firewire stack require change to switch
the type of context in which callback is executed. The series of change
includes a patch for them to adapt to work process context.

Finally, these changes are tested by devices supported by ALSA firewire
stack with/without no-period-wakeup runtime of PCM substream. I also tested
examples in libhinoko[3] as samples of user space applications. Currently I
face no issue.

On the other hand, I have neither tested for firewire-net nor firedtv,
since I have never used these functions. If someone has any experience to
use them, I would request to test the change.

[1] https://lore.kernel.org/lkml/20240403144558.13398-1-apais@linux.microsoft.com/
[2] https://github.com/allenpais/for-6.9-bh-conversions/issues/1
[3] https://git.kernel.org/pub/scm/libs/ieee1394/libhinoko.git/


Regards

Takashi Sakamoto (5):
  firewire: core: allocate workqueue to handle isochronous contexts in
    card
  firewire: core: add local API for work items scheduled to workqueue
    specific to isochronous contexts
  firewire: ohci: process IT/IR events in sleepable work process to
    obsolete tasklet softIRQ
  firewire: core: non-atomic memory allocation for isochronous event to
    user client
  ALSA: firewire: use nonatomic PCM operation

 drivers/firewire/core-card.c             | 31 +++++++++++--
 drivers/firewire/core-cdev.c             |  4 +-
 drivers/firewire/core-iso.c              | 22 ++++++++-
 drivers/firewire/core.h                  | 14 +++++-
 drivers/firewire/ohci.c                  | 57 +++++++++++++++++++-----
 include/linux/firewire.h                 |  3 ++
 sound/firewire/amdtp-stream.c            |  9 +++-
 sound/firewire/bebob/bebob_pcm.c         |  1 +
 sound/firewire/dice/dice-pcm.c           |  1 +
 sound/firewire/digi00x/digi00x-pcm.c     |  1 +
 sound/firewire/fireface/ff-pcm.c         |  1 +
 sound/firewire/fireworks/fireworks_pcm.c |  1 +
 sound/firewire/isight.c                  |  1 +
 sound/firewire/motu/motu-pcm.c           |  1 +
 sound/firewire/oxfw/oxfw-pcm.c           |  1 +
 sound/firewire/tascam/tascam-pcm.c       |  1 +
 16 files changed, 128 insertions(+), 21 deletions(-)
  

>This series of change is inspired by BH workqueue available in recent
>kernel.

>In Linux FireWire subsystem, tasklet softIRQ context has been utilized to
>operate 1394 OHCI Isochronous Transmit (IT) and Isochronous Receive (IR)
>contexts. The tasklet context is not preferable, as you know.

>I have already received a proposal[1][2] to replace the usage of tasklet
>with BH workqueue. However, the proposal includes bare replacement for 1394
>OHCI IT, IR, Asynchronous Transmit (AT), and Asynchronous Receive (AR)
>contexts with neither any care of actual usage for each context nor
>practical test reports. In theory, this kind of change should be done by
>step by step with enough amount of evaluation over software design to avoid
>any disorder.

>In this series of changes, the usage of tasklet for 1394 OHCI IT/IR
>contexts is just replaced, as a first step. In 1394 OHCI IR/IT events,
>software is expected to process the content of page dedicated to DMA
>transmission for each isochronous context. It means that the content can be
>processed concurrently per isochronous context. Additionally, the content
>of page is immutable as long as the software schedules the transmission
>again for the page. It means that the task to process the content can sleep
>or be preempted. Due to the characteristics, BH workqueue is _not_ used.

>At present, 1394 OHCI driver is responsible for the maintenance of tasklet
>context, while in this series of change the core function is responsible
>for the maintenance of workqueue and work items. This change is an attempt
>to let each implementation focus on own task.

>The change affects the following implementations of unit protocols which
>operate isochronous contexts:

>- firewire-net for IP over 1394 (RFC 2734/3146)
>- firedtv
>- drivers in ALSA firewire stack for IEC 61883-1/6
>- user space applications

>As long as reading their codes, the first two drivers look to require no
>change. While the drivers in ALSA firewire stack require change to switch
>the type of context in which callback is executed. The series of change
>includes a patch for them to adapt to work process context.

>Finally, these changes are tested by devices supported by ALSA firewire
>stack with/without no-period-wakeup runtime of PCM substream. I also tested
>examples in libhinoko[3] as samples of user space applications. Currently I
>face no issue.

>On the other hand, I have neither tested for firewire-net nor firedtv,
>since I have never used these functions. If someone has any experience to
>use them, I would request to test the change.

>[1] https://lore.kernel.org/lkml/20240403144558.13398-1-apais@linux.microsoft.com/
>[2] https://github.com/allenpais/for-6.9-bh-conversions/issues/1
>[3] https://git.kernel.org/pub/scm/libs/ieee1394/libhinoko.git/


>Regards

Thank you for doing this work. You will probably need to send out a v2
As most of you patches have single line comment instead of Block style
Commnents (/* ..*/). Please have it fixed.

- Allen

>Takashi Sakamoto (5):
> firewire: core: allocate workqueue to handle isochronous contexts in
>   card
> firewire: core: add local API for work items scheduled to workqueue
>   specific to isochronous contexts
> firewire: ohci: process IT/IR events in sleepable work process to
>   obsolete tasklet softIRQ
> firewire: core: non-atomic memory allocation for isochronous event to
>   user client
> ALSA: firewire: use nonatomic PCM operation
  

HI,

On Tue, Sep 03, 2024 at 11:06:53AM -0700, Allen Pais wrote:
> 
> 
> >This series of change is inspired by BH workqueue available in recent
> >kernel.
> 
> >In Linux FireWire subsystem, tasklet softIRQ context has been utilized to
> >operate 1394 OHCI Isochronous Transmit (IT) and Isochronous Receive (IR)
> >contexts. The tasklet context is not preferable, as you know.
> 
> >I have already received a proposal[1][2] to replace the usage of tasklet
> >with BH workqueue. However, the proposal includes bare replacement for 1394
> >OHCI IT, IR, Asynchronous Transmit (AT), and Asynchronous Receive (AR)
> >contexts with neither any care of actual usage for each context nor
> >practical test reports. In theory, this kind of change should be done by
> >step by step with enough amount of evaluation over software design to avoid
> >any disorder.
> 
> >In this series of changes, the usage of tasklet for 1394 OHCI IT/IR
> >contexts is just replaced, as a first step. In 1394 OHCI IR/IT events,
> >software is expected to process the content of page dedicated to DMA
> >transmission for each isochronous context. It means that the content can be
> >processed concurrently per isochronous context. Additionally, the content
> >of page is immutable as long as the software schedules the transmission
> >again for the page. It means that the task to process the content can sleep
> >or be preempted. Due to the characteristics, BH workqueue is _not_ used.
> 
> >At present, 1394 OHCI driver is responsible for the maintenance of tasklet
> >context, while in this series of change the core function is responsible
> >for the maintenance of workqueue and work items. This change is an attempt
> >to let each implementation focus on own task.
> 
> >The change affects the following implementations of unit protocols which
> >operate isochronous contexts:
> 
> >- firewire-net for IP over 1394 (RFC 2734/3146)
> >- firedtv
> >- drivers in ALSA firewire stack for IEC 61883-1/6
> >- user space applications
> 
> >As long as reading their codes, the first two drivers look to require no
> >change. While the drivers in ALSA firewire stack require change to switch
> >the type of context in which callback is executed. The series of change
> >includes a patch for them to adapt to work process context.
> 
> >Finally, these changes are tested by devices supported by ALSA firewire
> >stack with/without no-period-wakeup runtime of PCM substream. I also tested
> >examples in libhinoko[3] as samples of user space applications. Currently I
> >face no issue.
> 
> >On the other hand, I have neither tested for firewire-net nor firedtv,
> >since I have never used these functions. If someone has any experience to
> >use them, I would request to test the change.
> 
> >[1] https://lore.kernel.org/lkml/20240403144558.13398-1-apais@linux.microsoft.com/
> >[2] https://github.com/allenpais/for-6.9-bh-conversions/issues/1
> >[3] https://git.kernel.org/pub/scm/libs/ieee1394/libhinoko.git/
> 
> 
> >Regards
> 
> Thank you for doing this work. You will probably need to send out a v2
> As most of you patches have single line comment instead of Block style
> Commnents (/* ..*/). Please have it fixed.
 
Thanks for your comment. However, I think we have a need to update kernel
documentation.

As long as I know, Mr. Linus has few oposition to C99-style comment[1].
In recent kernel development process, C11 is widely accepted. Actually, we
can see many lines with C99-style comment in source tree.

For the kernel API documentation, we still need to use Doxygen-like block
comment since documentation generator requires it. Additionally we can also
see C90 comment is mandatory for UAPI since 'scripts/check-uapi.sh'
hard-codes it. For the other part, C99-style comment brings no issue, as
long as I know.

> - Allen
> 
> >Takashi Sakamoto (5):
> > firewire: core: allocate workqueue to handle isochronous contexts in
> >   card
> > firewire: core: add local API for work items scheduled to workqueue
> >   specific to isochronous contexts
> > firewire: ohci: process IT/IR events in sleepable work process to
> >   obsolete tasklet softIRQ
> > firewire: core: non-atomic memory allocation for isochronous event to
> >   user client
> > ALSA: firewire: use nonatomic PCM operation

[1] https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/


Thanks

Takashi Sakamoto
  

Hello Sakamoto-San, I very much appreciate the idea and effort to take on the tasklet conversion in small steps instead of all-at-once!

I also thank you for the CC, I'd like to be the testing canary for the coal mine of firewire ALSA with RME FireFace!
The ALSA mailing list is a bit overwhelming and I'll likely unsubscribe so a direct CC for anything I can test is a good idea.

Trying to apply patch 1 of 5 to mainline, your kernel tree appears to be out of sync with mainline!
It was missing b171e20 from 2009 and a7ecbe9 from 2022!
I hope nothing else important is missing!

Since in fw_card_initialize, ret is tested to be 0 we'd need an else instead, is this correct?

I edited these functions of patch 1, now everything applies just fine:

@@ -571,11 +571,28 @@ void fw_card_initialize(struct fw_card *card,
 }
 EXPORT_SYMBOL(fw_card_initialize);
 
-int fw_card_add(struct fw_card *card,
-		u32 max_receive, u32 link_speed, u64 guid)
+int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
+		unsigned int supported_isoc_contexts)
 {
+	struct workqueue_struct *isoc_wq;
 	int ret;
 
+	// This workqueue should be:
+	//  * != WQ_BH			Sleepable.
+	//  * == WQ_UNBOUND		Any core can process data for isoc context. The
+	//				implementation of unit protocol could consumes the core
+	//				longer somehow.
+	//  * != WQ_MEM_RECLAIM		Not used for any backend of block device.
+	//  * == WQ_HIGHPRI		High priority to process semi-realtime timestamped data.
+	//  * == WQ_SYSFS		Parameters are available via sysfs.
+	//  * max_active == n_it + n_ir	A hardIRQ could notify events for multiple isochronous
+	//				contexts if they are scheduled to the same cycle.
+	isoc_wq = alloc_workqueue("firewire-isoc-card%u",
+				  WQ_UNBOUND | WQ_HIGHPRI | WQ_SYSFS,
+				  supported_isoc_contexts, card->index);
+	if (!isoc_wq)
+		return -ENOMEM;
+
 	card->max_receive = max_receive;
 	card->link_speed = link_speed;
 	card->guid = guid;
@@ -584,9 +601,13 @@ int fw_card_add(struct fw_card *card,
 
 	generate_config_rom(card, tmp_config_rom);
 	ret = card->driver->enable(card, tmp_config_rom, config_rom_length);
 	if (ret == 0)
 		list_add_tail(&card->link, &card_list);
+	else
+		destroy_workqueue(isoc_wq);
+
+	card->isoc_wq = isoc_wq;

 	mutex_unlock(&card_mutex);

 	return ret;
@@ -709,7 +729,9 @@ void fw_core_remove_card(struct fw_card *card)
 {
 	struct fw_card_driver dummy_driver = dummy_driver_template;
 	unsigned long flags;
 
+	might_sleep();
+
 	card->driver->update_phy_reg(card, 4,
 				     PHY_LINK_ACTIVE | PHY_CONTENDER, 0);
 	fw_schedule_bus_reset(card, false, true);
@@ -719,6 +741,7 @@ void fw_core_remove_card(struct fw_card *card)
 	dummy_driver.free_iso_context	= card->driver->free_iso_context;
 	dummy_driver.stop_iso		= card->driver->stop_iso;
 	card->driver = &dummy_driver;
+	drain_workqueue(card->isoc_wq);
 
 	spin_lock_irqsave(&card->lock, flags);
 	fw_destroy_nodes(card);

Building a kernel with the patch produced 6.11.0-rc6-1-mainline-00019-g67784a74e258-dirty.
Testing it with TI XIO2213B and RME Fireface 800 so far > 1 hour reveals no issues at all.
ALSA streaming works fine:
  mpv --audio-device=alsa/sysdefault:CARD=Fireface800 Spor-Ignition.flac

Though I haven't the faintest clue how to measure CPU usage impact of the patch, it looks like it would be neglible.

As of finishing this, I noticed you released [2] https://lore.kernel.org/lkml/20240904125155.461886-1-o-takashi@sakamocchi.jp/T/
I'll get around to testing that one too, but tomorrow at the earliest.

Kind regards,
Edmund Raile.

Signed-off-by: Edmund Raile <edmund.raile@protonmail.com>
Tested-by: Edmund Raile <edmund.raile@protonmail.com>
  

Hi,

Thanks for your test.

On Wed, Sep 04, 2024 at 08:45:51PM +0000, Edmund Raile wrote:
> Hello Sakamoto-San, I very much appreciate the idea and effort to take on the tasklet conversion in small steps instead of all-at-once!
> 
> I also thank you for the CC, I'd like to be the testing canary for the coal mine of firewire ALSA with RME FireFace!
> The ALSA mailing list is a bit overwhelming and I'll likely unsubscribe so a direct CC for anything I can test is a good idea.
> 
> Trying to apply patch 1 of 5 to mainline, your kernel tree appears to be out of sync with mainline!
> It was missing b171e20 from 2009 and a7ecbe9 from 2022!
> I hope nothing else important is missing!
 
Yes. The series of changes is prepared for the next merge window to
v6.12 kernel. It is on the top of for-next branch in linux1394 tree.
You can see some patches on v6.12-rc2 tag.

https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/log/?h=for-next

> Since in fw_card_initialize, ret is tested to be 0 we'd need an else instead, is this correct?
> 
> I edited these functions of patch 1, now everything applies just fine:
> 
> @@ -571,11 +571,28 @@ void fw_card_initialize(struct fw_card *card,
>  }
>  EXPORT_SYMBOL(fw_card_initialize);
>  
> -int fw_card_add(struct fw_card *card,
> -		u32 max_receive, u32 link_speed, u64 guid)
> +int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
> +		unsigned int supported_isoc_contexts)
>  {
> +	struct workqueue_struct *isoc_wq;
>  	int ret;
>  
> +	// This workqueue should be:
> +	//  * != WQ_BH			Sleepable.
> +	//  * == WQ_UNBOUND		Any core can process data for isoc context. The
> +	//				implementation of unit protocol could consumes the core
> +	//				longer somehow.
> +	//  * != WQ_MEM_RECLAIM		Not used for any backend of block device.
> +	//  * == WQ_HIGHPRI		High priority to process semi-realtime timestamped data.
> +	//  * == WQ_SYSFS		Parameters are available via sysfs.
> +	//  * max_active == n_it + n_ir	A hardIRQ could notify events for multiple isochronous
> +	//				contexts if they are scheduled to the same cycle.
> +	isoc_wq = alloc_workqueue("firewire-isoc-card%u",
> +				  WQ_UNBOUND | WQ_HIGHPRI | WQ_SYSFS,
> +				  supported_isoc_contexts, card->index);
> +	if (!isoc_wq)
> +		return -ENOMEM;
> +
>  	card->max_receive = max_receive;
>  	card->link_speed = link_speed;
>  	card->guid = guid;
> @@ -584,9 +601,13 @@ int fw_card_add(struct fw_card *card,
>  
>  	generate_config_rom(card, tmp_config_rom);
>  	ret = card->driver->enable(card, tmp_config_rom, config_rom_length);
>  	if (ret == 0)
>  		list_add_tail(&card->link, &card_list);
> +	else
> +		destroy_workqueue(isoc_wq);
> +
> +	card->isoc_wq = isoc_wq;
> 
>  	mutex_unlock(&card_mutex);
> 
>  	return ret;
> @@ -709,7 +729,9 @@ void fw_core_remove_card(struct fw_card *card)
>  {
>  	struct fw_card_driver dummy_driver = dummy_driver_template;
>  	unsigned long flags;
>  
> +	might_sleep();
> +
>  	card->driver->update_phy_reg(card, 4,
>  				     PHY_LINK_ACTIVE | PHY_CONTENDER, 0);
>  	fw_schedule_bus_reset(card, false, true);
> @@ -719,6 +741,7 @@ void fw_core_remove_card(struct fw_card *card)
>  	dummy_driver.free_iso_context	= card->driver->free_iso_context;
>  	dummy_driver.stop_iso		= card->driver->stop_iso;
>  	card->driver = &dummy_driver;
> +	drain_workqueue(card->isoc_wq);
>  
>  	spin_lock_irqsave(&card->lock, flags);
>  	fw_destroy_nodes(card);
> 
> Building a kernel with the patch produced 6.11.0-rc6-1-mainline-00019-g67784a74e258-dirty.
> Testing it with TI XIO2213B and RME Fireface 800 so far > 1 hour reveals no issues at all.
> ALSA streaming works fine:
>   mpv --audio-device=alsa/sysdefault:CARD=Fireface800 Spor-Ignition.flac
> 
> Though I haven't the faintest clue how to measure CPU usage impact of the patch, it looks like it would be neglible.
> 
> As of finishing this, I noticed you released [2] https://lore.kernel.org/lkml/20240904125155.461886-1-o-takashi@sakamocchi.jp/T/
> I'll get around to testing that one too, but tomorrow at the earliest.
> 
> Kind regards,
> Edmund Raile.
> 
> Signed-off-by: Edmund Raile <edmund.raile@protonmail.com>
> Tested-by: Edmund Raile <edmund.raile@protonmail.com>

If using v6.11 kernel, it is convenient to use my remote repository to
backport changes for v6.12. But let you be careful to the history of
changes anyway.

* https://github.com/takaswie/linux-firewire-dkms/


Thanks

Takashi Sakamoto