* [RFT][PATCH 1/5] firewire: core: allocate workqueue to handle isochronous contexts in card
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
@ 2024-09-01 11:06 ` Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 2/5] firewire: core: add local API for work items scheduled to workqueue specific to isochronous contexts Takashi Sakamoto
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-01 11:06 UTC (permalink / raw)
To: linux1394-devel
Cc: linux-kernel, linux-sound, apais, edmund.raile, linux-media,
netdev
This commit adds a workqueue dedicated for isochronous context processing.
The workqueue is allocated per instance of fw_card structure to satisfy the
following characteristics descending from 1394 OHCI specification:
In 1394 OHCI specification, memory pages are reserved to each isochronous
context dedicated to DMA transmission. It allows to operate these
per-context pages concurrently, thus WQ_UNBOUND is specified. Additionally,
it is sleepable to operate the content of pages, thus WQ_BH is not used.
The isochronous context delivers the packets with time stamp, thus
WQ_HIGHPRI is specified for semi real-time data such as IEC 61883-1/6
protocol implemented by ALSA firewire stack. The isochronous context is not
used by the implementation of SCSI over IEEE1394 protocol (sbp2), thus
WQ_MEM_RECLAIM is not specified.
It is useful for users to adjust cpu affinity of the workqueue depending
on their work loads, thus WQ_SYS is specified to expose the attributes to
user space.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-card.c | 31 ++++++++++++++++++++++++++++---
drivers/firewire/core.h | 4 ++--
drivers/firewire/ohci.c | 2 +-
include/linux/firewire.h | 2 ++
4 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index e80b762888fa..e0b8423fd4d0 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -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,12 @@ 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)
+ if (ret < 0) {
+ destroy_workqueue(isoc_wq);
return ret;
+ }
+ card->isoc_wq = isoc_wq;
list_add_tail(&card->link, &card_list);
return 0;
@@ -708,6 +728,8 @@ void fw_core_remove_card(struct fw_card *card)
{
struct fw_card_driver dummy_driver = dummy_driver_template;
+ 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);
scoped_guard(spinlock_irqsave, &card->lock)
fw_destroy_nodes(card);
@@ -727,6 +750,8 @@ void fw_core_remove_card(struct fw_card *card)
fw_card_put(card);
wait_for_completion(&card->done);
+ destroy_workqueue(card->isoc_wq);
+
WARN_ON(!list_empty(&card->transaction_list));
}
EXPORT_SYMBOL(fw_core_remove_card);
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 57d101c01e36..96ae366889e0 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -115,8 +115,8 @@ struct fw_card_driver {
void fw_card_initialize(struct fw_card *card,
const struct fw_card_driver *driver, struct device *device);
-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);
void fw_core_remove_card(struct fw_card *card);
int fw_compute_block_crc(__be32 *block);
void fw_schedule_bm_work(struct fw_card *card, unsigned long delay);
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index a3a37955b174..ad3bdc48f0f5 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -3825,7 +3825,7 @@ static int pci_probe(struct pci_dev *dev,
goto fail_msi;
}
- err = fw_card_add(&ohci->card, max_receive, link_speed, guid);
+ err = fw_card_add(&ohci->card, max_receive, link_speed, guid, ohci->n_it + ohci->n_ir);
if (err)
goto fail_irq;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 1cca14cf5652..10e135d60824 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -134,6 +134,8 @@ struct fw_card {
__be32 topology_map[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4];
__be32 maint_utility_register;
+
+ struct workqueue_struct *isoc_wq;
};
static inline struct fw_card *fw_card_get(struct fw_card *card)
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [RFT][PATCH 2/5] firewire: core: add local API for work items scheduled to workqueue specific to isochronous contexts
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 1/5] firewire: core: allocate workqueue to handle isochronous contexts in card Takashi Sakamoto
@ 2024-09-01 11:06 ` Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 3/5] firewire: ohci: process IT/IR events in sleepable work process to obsolete tasklet softIRQ Takashi Sakamoto
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-01 11:06 UTC (permalink / raw)
To: linux1394-devel
Cc: linux-kernel, linux-sound, apais, edmund.raile, linux-media,
netdev
In the previous commit, the workqueue is added per the instance of fw_card
structure for isochronous contexts. The workqueue is designed to be used by
the implementation of fw_card_driver structure underlying the fw_card.
This commit adds some local APIs to be used by the implementation.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-iso.c | 22 ++++++++++++++++++++--
drivers/firewire/core.h | 10 ++++++++++
include/linux/firewire.h | 1 +
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index 101433b8bb51..124579a9c657 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -211,21 +211,39 @@ EXPORT_SYMBOL(fw_iso_context_queue_flush);
int fw_iso_context_flush_completions(struct fw_iso_context *ctx)
{
+ int err;
+
trace_isoc_outbound_flush_completions(ctx);
trace_isoc_inbound_single_flush_completions(ctx);
trace_isoc_inbound_multiple_flush_completions(ctx);
- return ctx->card->driver->flush_iso_completions(ctx);
+ might_sleep();
+
+ disable_work_sync(&ctx->work);
+
+ err = ctx->card->driver->flush_iso_completions(ctx);
+
+ enable_work(&ctx->work);
+
+ return err;
}
EXPORT_SYMBOL(fw_iso_context_flush_completions);
int fw_iso_context_stop(struct fw_iso_context *ctx)
{
+ int err;
+
trace_isoc_outbound_stop(ctx);
trace_isoc_inbound_single_stop(ctx);
trace_isoc_inbound_multiple_stop(ctx);
- return ctx->card->driver->stop_iso(ctx);
+ might_sleep();
+
+ err = ctx->card->driver->stop_iso(ctx);
+
+ cancel_work_sync(&ctx->work);
+
+ return err;
}
EXPORT_SYMBOL(fw_iso_context_stop);
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 96ae366889e0..1b78d66a88a0 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -159,6 +159,16 @@ int fw_iso_buffer_alloc(struct fw_iso_buffer *buffer, int page_count);
int fw_iso_buffer_map_dma(struct fw_iso_buffer *buffer, struct fw_card *card,
enum dma_data_direction direction);
+static inline void fw_iso_context_init_work(struct fw_iso_context *ctx, work_func_t func)
+{
+ INIT_WORK(&ctx->work, func);
+}
+
+static inline void fw_iso_context_schedule_work(struct fw_iso_context *ctx)
+{
+ queue_work(ctx->card->isoc_wq, &ctx->work);
+}
+
/* -topology */
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 10e135d60824..72f497b61739 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -511,6 +511,7 @@ union fw_iso_callback {
struct fw_iso_context {
struct fw_card *card;
+ struct work_struct work;
int type;
int channel;
int speed;
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [RFT][PATCH 3/5] firewire: ohci: process IT/IR events in sleepable work process to obsolete tasklet softIRQ
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 1/5] firewire: core: allocate workqueue to handle isochronous contexts in card Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 2/5] firewire: core: add local API for work items scheduled to workqueue specific to isochronous contexts Takashi Sakamoto
@ 2024-09-01 11:06 ` Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 4/5] firewire: core: non-atomic memory allocation for isochronous event to user client Takashi Sakamoto
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-01 11:06 UTC (permalink / raw)
To: linux1394-devel
Cc: linux-kernel, linux-sound, apais, edmund.raile, linux-media,
netdev
This commit queues work item for IT/IR events at hardIRQ handler to operate
the corresponding isochronous context. The work item is queued to the
workqueue on the cpu in which the events are handled.
The callback for either the implementation of unit protocol and user space
clients is executed in sleepable work item. It could results in any errors
of concurrent processing as well as sleep at atomic context. These errors
are fixed by the following commits.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/ohci.c | 55 +++++++++++++++++++++++++++++++++--------
1 file changed, 45 insertions(+), 10 deletions(-)
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index ad3bdc48f0f5..c94b2728cf03 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1182,6 +1182,47 @@ static void context_tasklet(unsigned long data)
}
}
+static void ohci_isoc_context_work(struct work_struct *work)
+{
+ struct fw_iso_context *base = container_of(work, struct fw_iso_context, work);
+ struct iso_context *isoc_ctx = container_of(base, struct iso_context, base);
+ struct context *ctx = &isoc_ctx->context;
+ struct descriptor *d, *last;
+ u32 address;
+ int z;
+ struct descriptor_buffer *desc;
+
+ desc = list_entry(ctx->buffer_list.next, struct descriptor_buffer, list);
+ last = ctx->last;
+ while (last->branch_address != 0) {
+ struct descriptor_buffer *old_desc = desc;
+
+ address = le32_to_cpu(last->branch_address);
+ z = address & 0xf;
+ address &= ~0xf;
+ ctx->current_bus = address;
+
+ // If the branch address points to a buffer outside of the current buffer, advance
+ // to the next buffer.
+ if (address < desc->buffer_bus || address >= desc->buffer_bus + desc->used)
+ desc = list_entry(desc->list.next, struct descriptor_buffer, list);
+ d = desc->buffer + (address - desc->buffer_bus) / sizeof(*d);
+ last = find_branch_descriptor(d, z);
+
+ if (!ctx->callback(ctx, d, last))
+ break;
+
+ if (old_desc != desc) {
+ // If we've advanced to the next buffer, move the previous buffer to the
+ // free list.
+ old_desc->used = 0;
+ guard(spinlock_irqsave)(&ctx->ohci->lock);
+ list_move_tail(&old_desc->list, &ctx->buffer_list);
+ }
+ ctx->last = last;
+ }
+}
+
/*
* Allocate a new buffer and add it to the list of free buffers for this
* context. Must be called with ohci->lock held.
@@ -2237,8 +2278,7 @@ static irqreturn_t irq_handler(int irq, void *data)
while (iso_event) {
i = ffs(iso_event) - 1;
- tasklet_schedule(
- &ohci->ir_context_list[i].context.tasklet);
+ fw_iso_context_schedule_work(&ohci->ir_context_list[i].base);
iso_event &= ~(1 << i);
}
}
@@ -2249,8 +2289,7 @@ static irqreturn_t irq_handler(int irq, void *data)
while (iso_event) {
i = ffs(iso_event) - 1;
- tasklet_schedule(
- &ohci->it_context_list[i].context.tasklet);
+ fw_iso_context_schedule_work(&ohci->it_context_list[i].base);
iso_event &= ~(1 << i);
}
}
@@ -3128,6 +3167,7 @@ static struct fw_iso_context *ohci_allocate_iso_context(struct fw_card *card,
ret = context_init(&ctx->context, ohci, regs, callback);
if (ret < 0)
goto out_with_header;
+ fw_iso_context_init_work(&ctx->base, ohci_isoc_context_work);
if (type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL) {
set_multichannel_mask(ohci, 0);
@@ -3225,7 +3265,6 @@ static int ohci_stop_iso(struct fw_iso_context *base)
}
flush_writes(ohci);
context_stop(&ctx->context);
- tasklet_kill(&ctx->context.tasklet);
return 0;
}
@@ -3582,10 +3621,8 @@ static int ohci_flush_iso_completions(struct fw_iso_context *base)
struct iso_context *ctx = container_of(base, struct iso_context, base);
int ret = 0;
- tasklet_disable_in_atomic(&ctx->context.tasklet);
-
if (!test_and_set_bit_lock(0, &ctx->flushing_completions)) {
- context_tasklet((unsigned long)&ctx->context);
+ ohci_isoc_context_work(&base->work);
switch (base->type) {
case FW_ISO_CONTEXT_TRANSMIT:
@@ -3605,8 +3642,6 @@ static int ohci_flush_iso_completions(struct fw_iso_context *base)
smp_mb__after_atomic();
}
- tasklet_enable(&ctx->context.tasklet);
-
return ret;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [RFT][PATCH 4/5] firewire: core: non-atomic memory allocation for isochronous event to user client
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
` (2 preceding siblings ...)
2024-09-01 11:06 ` [RFT][PATCH 3/5] firewire: ohci: process IT/IR events in sleepable work process to obsolete tasklet softIRQ Takashi Sakamoto
@ 2024-09-01 11:06 ` Takashi Sakamoto
2024-09-01 11:06 ` [RFT][PATCH 5/5] ALSA: firewire: use nonatomic PCM operation Takashi Sakamoto
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-01 11:06 UTC (permalink / raw)
To: linux1394-devel
Cc: linux-kernel, linux-sound, apais, edmund.raile, linux-media,
netdev
In the former commits, the callback of isochronous context runs on work
process, thus no need to use atomic memory allocation.
This commit replaces GFP_ATOMIC with GCP_KERNEL in the callback for user
client.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-cdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 3ea220d96c31..518eaa073b2b 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -982,7 +982,7 @@ static void iso_callback(struct fw_iso_context *context, u32 cycle,
struct client *client = data;
struct iso_interrupt_event *e;
- e = kmalloc(sizeof(*e) + header_length, GFP_ATOMIC);
+ e = kmalloc(sizeof(*e) + header_length, GFP_KERNEL);
if (e == NULL)
return;
@@ -1001,7 +1001,7 @@ static void iso_mc_callback(struct fw_iso_context *context,
struct client *client = data;
struct iso_interrupt_mc_event *e;
- e = kmalloc(sizeof(*e), GFP_ATOMIC);
+ e = kmalloc(sizeof(*e), GFP_KERNEL);
if (e == NULL)
return;
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [RFT][PATCH 5/5] ALSA: firewire: use nonatomic PCM operation
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
` (3 preceding siblings ...)
2024-09-01 11:06 ` [RFT][PATCH 4/5] firewire: core: non-atomic memory allocation for isochronous event to user client Takashi Sakamoto
@ 2024-09-01 11:06 ` Takashi Sakamoto
2024-09-03 18:06 ` [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Allen Pais
2024-09-04 20:45 ` firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 1 Edmund Raile
6 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-01 11:06 UTC (permalink / raw)
To: linux1394-devel
Cc: linux-kernel, linux-sound, apais, edmund.raile, linux-media,
netdev
In the former commits, the callback of isochronous context runs on work
process. In the case, ALSA PCM device has a flag, nonatomic, to acquire
mutex lock instead of spin lock for PCM substream group.
This commit uses the flag. It has an advantage in the case that ALSA PCM
application uses the large size of intermediate buffer, since it takes
too long time even in tasklet softIRQ to process many of isochronous
packets. It could result in the delay of system event due to disabled
IRQ so long.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
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 +
10 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index c827d7d8d800..7e97ad133874 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -1055,8 +1055,15 @@ static void generate_rx_packet_descs(struct amdtp_stream *s, struct pkt_desc *de
static inline void cancel_stream(struct amdtp_stream *s)
{
+ struct work_struct *work = current_work();
+
s->packet_index = -1;
- if (in_softirq())
+
+ // Detect work items for any isochronous context. The work item for pcm_period_work()
+ // should be avoided since the call of snd_pcm_period_elapsed() can reach via
+ // snd_pcm_ops.pointer() under acquiring PCM stream(group) lock and causes dead lock at
+ // snd_pcm_stop_xrun().
+ if (work && work != &s->period_work)
amdtp_stream_pcm_abort(s);
WRITE_ONCE(s->pcm_buffer_pointer, SNDRV_PCM_POS_XRUN);
}
diff --git a/sound/firewire/bebob/bebob_pcm.c b/sound/firewire/bebob/bebob_pcm.c
index ce49eef0fcba..360ebf3c4ca2 100644
--- a/sound/firewire/bebob/bebob_pcm.c
+++ b/sound/firewire/bebob/bebob_pcm.c
@@ -367,6 +367,7 @@ int snd_bebob_create_pcm_devices(struct snd_bebob *bebob)
goto end;
pcm->private_data = bebob;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", bebob->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c
index d64366217d57..2cf2adb48f2a 100644
--- a/sound/firewire/dice/dice-pcm.c
+++ b/sound/firewire/dice/dice-pcm.c
@@ -441,6 +441,7 @@ int snd_dice_create_pcm(struct snd_dice *dice)
if (err < 0)
return err;
pcm->private_data = dice;
+ pcm->nonatomic = true;
strcpy(pcm->name, dice->card->shortname);
if (capture > 0)
diff --git a/sound/firewire/digi00x/digi00x-pcm.c b/sound/firewire/digi00x/digi00x-pcm.c
index 3bd1575c9d9c..85e65cbc00c4 100644
--- a/sound/firewire/digi00x/digi00x-pcm.c
+++ b/sound/firewire/digi00x/digi00x-pcm.c
@@ -350,6 +350,7 @@ int snd_dg00x_create_pcm_devices(struct snd_dg00x *dg00x)
return err;
pcm->private_data = dg00x;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", dg00x->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
diff --git a/sound/firewire/fireface/ff-pcm.c b/sound/firewire/fireface/ff-pcm.c
index ec915671a79b..63457d24a288 100644
--- a/sound/firewire/fireface/ff-pcm.c
+++ b/sound/firewire/fireface/ff-pcm.c
@@ -390,6 +390,7 @@ int snd_ff_create_pcm_devices(struct snd_ff *ff)
return err;
pcm->private_data = ff;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", ff->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &pcm_playback_ops);
diff --git a/sound/firewire/fireworks/fireworks_pcm.c b/sound/firewire/fireworks/fireworks_pcm.c
index c3c21860b245..eaf7778211de 100644
--- a/sound/firewire/fireworks/fireworks_pcm.c
+++ b/sound/firewire/fireworks/fireworks_pcm.c
@@ -397,6 +397,7 @@ int snd_efw_create_pcm_devices(struct snd_efw *efw)
goto end;
pcm->private_data = efw;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name), "%s PCM", efw->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &capture_ops);
diff --git a/sound/firewire/isight.c b/sound/firewire/isight.c
index 806f82c9ceee..b1e059f0d473 100644
--- a/sound/firewire/isight.c
+++ b/sound/firewire/isight.c
@@ -454,6 +454,7 @@ static int isight_create_pcm(struct isight *isight)
if (err < 0)
return err;
pcm->private_data = isight;
+ pcm->nonatomic = true;
strcpy(pcm->name, "iSight");
isight->pcm = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream;
isight->pcm->ops = &ops;
diff --git a/sound/firewire/motu/motu-pcm.c b/sound/firewire/motu/motu-pcm.c
index d410c2efbde5..f3b48495acae 100644
--- a/sound/firewire/motu/motu-pcm.c
+++ b/sound/firewire/motu/motu-pcm.c
@@ -360,6 +360,7 @@ int snd_motu_create_pcm_devices(struct snd_motu *motu)
if (err < 0)
return err;
pcm->private_data = motu;
+ pcm->nonatomic = true;
strcpy(pcm->name, motu->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &capture_ops);
diff --git a/sound/firewire/oxfw/oxfw-pcm.c b/sound/firewire/oxfw/oxfw-pcm.c
index 5f43a0b826d2..8ca9dde54ec6 100644
--- a/sound/firewire/oxfw/oxfw-pcm.c
+++ b/sound/firewire/oxfw/oxfw-pcm.c
@@ -440,6 +440,7 @@ int snd_oxfw_create_pcm(struct snd_oxfw *oxfw)
return err;
pcm->private_data = oxfw;
+ pcm->nonatomic = true;
strcpy(pcm->name, oxfw->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
if (cap > 0)
diff --git a/sound/firewire/tascam/tascam-pcm.c b/sound/firewire/tascam/tascam-pcm.c
index f6da571707ac..a73003ac11e6 100644
--- a/sound/firewire/tascam/tascam-pcm.c
+++ b/sound/firewire/tascam/tascam-pcm.c
@@ -279,6 +279,7 @@ int snd_tscm_create_pcm_devices(struct snd_tscm *tscm)
return err;
pcm->private_data = tscm;
+ pcm->nonatomic = true;
snprintf(pcm->name, sizeof(pcm->name),
"%s PCM", tscm->card->shortname);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops);
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
` (4 preceding siblings ...)
2024-09-01 11:06 ` [RFT][PATCH 5/5] ALSA: firewire: use nonatomic PCM operation Takashi Sakamoto
@ 2024-09-03 18:06 ` Allen Pais
2024-09-04 0:30 ` Takashi Sakamoto
2024-09-04 20:45 ` firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 1 Edmund Raile
6 siblings, 1 reply; 10+ messages in thread
From: Allen Pais @ 2024-09-03 18:06 UTC (permalink / raw)
To: Takashi Sakamoto
Cc: linux1394-devel, Linux Kernel Mailing List, linux-sound,
edmund.raile, linux-media, netdev
>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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events
2024-09-03 18:06 ` [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Allen Pais
@ 2024-09-04 0:30 ` Takashi Sakamoto
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-04 0:30 UTC (permalink / raw)
To: Allen Pais
Cc: Linux Kernel Mailing List, linux-sound, edmund.raile, linux-media,
netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 1
2024-09-01 11:06 [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Takashi Sakamoto
` (5 preceding siblings ...)
2024-09-03 18:06 ` [RFT][PATCH 0/5] firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events Allen Pais
@ 2024-09-04 20:45 ` Edmund Raile
2024-09-05 8:18 ` Takashi Sakamoto
6 siblings, 1 reply; 10+ messages in thread
From: Edmund Raile @ 2024-09-04 20:45 UTC (permalink / raw)
To: o-takashi
Cc: apais, edmund.raile, linux-kernel, linux-media, linux-sound,
linux1394-devel, netdev
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>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 1
2024-09-04 20:45 ` firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 1 Edmund Raile
@ 2024-09-05 8:18 ` Takashi Sakamoto
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Sakamoto @ 2024-09-05 8:18 UTC (permalink / raw)
To: Edmund Raile
Cc: apais, linux-kernel, linux-media, linux-sound, linux1394-devel,
netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread