| Message ID | 20240901110642.154523-1-o-takashi@sakamocchi.jp |
|---|---|
| Headers | show |
| Series | firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events | expand |
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, 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(-)