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

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

[Takashi Sakamoto]

Hi,

This series of changes updates my previous RFT[1] to apply for v6.12
kernel. For the detail, please refer to the previous one.

To Iwai-san, this series includes the change for sound subsystem as
well. All of changes are specific to ALSA firewire stack, so I would
like to send it to Linus as the part of firewire subsystem updates if
you do not mind it.

Changes from the RFT:
* WQ_FREEZABLE is newly supported in the workqueue
* Improve code comment in IEC 61883-1/6 packet streaming engine
* Avoid dead lock in the calls of workqueue sync API

[1] https://lore.kernel.org/lkml/20240901110642.154523-1-o-takashi@sakamocchi.jp/


Regards

Takashi Sakamoto (5):
  firewire: core: allocate workqueue to handle isochronous contexts in
    card
  firewire: core: add local API to queue work item to workqueue specific
    to isochronous contexts
  firewire: ohci: operate IT/IR events in sleepable work process instead
    of 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             | 33 ++++++++++++--
 drivers/firewire/core-cdev.c             |  4 +-
 drivers/firewire/core-iso.c              | 30 ++++++++++++-
 drivers/firewire/core.h                  | 14 +++++-
 drivers/firewire/ohci.c                  | 57 +++++++++++++++++++-----
 include/linux/firewire.h                 |  3 ++
 sound/firewire/amdtp-stream.c            | 34 +++++++++++---
 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, 157 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH 1/5] firewire: core: allocate workqueue to handle isochronous contexts in card

[Takashi Sakamoto]

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. Software can schedule hardware interrupt
for several isochronous context to the same cycle, 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 | 33 ++++++++++++++++++++++++++++++---
 drivers/firewire/core.h      |  4 ++--
 drivers/firewire/ohci.c      |  2 +-
 include/linux/firewire.h     |  2 ++
 4 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index e80b762888fa..01354b9de8b2 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -571,11 +571,30 @@ 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_FREEZABLE		Isochronous communication is at regular interval in real
+	//				time, thus should be drained if possible at freeze phase.
+	//  * == 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_FREEZABLE | 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 +603,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 +730,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 +743,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 +752,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 3930fdd56155..50627b8fc38f 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -3827,7 +3827,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

[PATCH 2/5] firewire: core: add local API to queue work item to workqueue specific to isochronous contexts

[Takashi Sakamoto]

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 | 30 ++++++++++++++++++++++++++++--
 drivers/firewire/core.h     | 10 ++++++++++
 include/linux/firewire.h    |  1 +
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index 101433b8bb51..af76fa1823f1 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -211,21 +211,47 @@ 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();
+
+	// Avoid dead lock due to programming mistake.
+	if (WARN_ON(current_work() == &ctx->work))
+		return 0;
+
+	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();
+
+	// Avoid dead lock due to programming mistake.
+	if (WARN_ON(current_work() == &ctx->work))
+		return 0;
+
+	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..2874f316156a 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_queue_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

Re: [PATCH 2/5] firewire: core: add local API to queue work item to workqueue specific to isochronous contexts

[Takashi Iwai]

On Wed, 04 Sep 2024 14:51:51 +0200,
Takashi Sakamoto wrote:
> 
> 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 | 30 ++++++++++++++++++++++++++++--
>  drivers/firewire/core.h     | 10 ++++++++++
>  include/linux/firewire.h    |  1 +
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
> index 101433b8bb51..af76fa1823f1 100644
> --- a/drivers/firewire/core-iso.c
> +++ b/drivers/firewire/core-iso.c
> @@ -211,21 +211,47 @@ 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();
> +
> +	// Avoid dead lock due to programming mistake.
> +	if (WARN_ON(current_work() == &ctx->work))
> +		return 0;

Maybe WARN_ON_ONCE() would be safer if it's supposed to be called
frequently.  Otherwise it can spam too much.
Ditto for fw_iso_context_stop().


thanks,

Takashi

Re: [PATCH 2/5] firewire: core: add local API to queue work item to workqueue specific to isochronous contexts

[Takashi Sakamoto]

Hi,

On Wed, Sep 04, 2024 at 06:07:20PM +0200, Takashi Iwai wrote:
> On Wed, 04 Sep 2024 14:51:51 +0200,
> Takashi Sakamoto wrote:
> > 
> > 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 | 30 ++++++++++++++++++++++++++++--
> >  drivers/firewire/core.h     | 10 ++++++++++
> >  include/linux/firewire.h    |  1 +
> >  3 files changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
> > index 101433b8bb51..af76fa1823f1 100644
> > --- a/drivers/firewire/core-iso.c
> > +++ b/drivers/firewire/core-iso.c
> > @@ -211,21 +211,47 @@ 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();
> > +
> > +	// Avoid dead lock due to programming mistake.
> > +	if (WARN_ON(current_work() == &ctx->work))
> > +		return 0;
> 
> Maybe WARN_ON_ONCE() would be safer if it's supposed to be called
> frequently.  Otherwise it can spam too much.
> Ditto for fw_iso_context_stop().

Thanks for your suggestion. Indeed, the kernel API would be called so
frequently, and the *_ONCE macro would be safer. I'll post another patch
for it, since posting updated series of changes to all of receivers is a
bit cumbersome to me...


Thanks

Takashi Sakamoto

[PATCH 3/5] firewire: ohci: operate IT/IR events in sleepable work process instead of tasklet softIRQ

[Takashi Sakamoto]

This commit queues work item for IT/IR events at hardIRQ handler to operate
the corresponding isochronous context. The work item is queued to any of
worker-pools.

The callback for either the implementation of unit protocol and user space
clients is executed in sleepable work process context. The change 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 50627b8fc38f..d0b1fccc450f 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.
@@ -2242,8 +2283,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_queue_work(&ohci->ir_context_list[i].base);
 			iso_event &= ~(1 << i);
 		}
 	}
@@ -2254,8 +2294,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_queue_work(&ohci->it_context_list[i].base);
 			iso_event &= ~(1 << i);
 		}
 	}
@@ -3130,6 +3169,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);
@@ -3227,7 +3267,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;
 }
@@ -3584,10 +3623,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:
@@ -3607,8 +3644,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

[PATCH 4/5] firewire: core: non-atomic memory allocation for isochronous event to user client

[Takashi Sakamoto]

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

[PATCH 5/5] ALSA: firewire: use nonatomic PCM operation

[Takashi Sakamoto]

In the former commits, the callback of isochronous context runs on usual
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, then result in the delay of system event due to disabled IRQ so
long. It is avertible to switch to nonatomic operation.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c            | 34 +++++++++++++++++++-----
 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, 36 insertions(+), 7 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index c827d7d8d800..c72b2a754775 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -615,6 +615,22 @@ static void update_pcm_pointers(struct amdtp_stream *s,
 		// The program in user process should periodically check the status of intermediate
 		// buffer associated to PCM substream to process PCM frames in the buffer, instead
 		// of receiving notification of period elapsed by poll wait.
+		//
+		// Use another work item for period elapsed event to prevent the following AB/BA
+		// deadlock:
+		//
+		//             thread 1                            thread 2
+		// =================================   =================================
+		//       A.work item (process)                pcm ioctl (process)
+		//                 v                                   v
+		//       process_rx_packets()                  B.PCM stream lock
+		//       process_tx_packets()                          v
+		//                 v                        callbacks in snd_pcm_ops
+		//       update_pcm_pointers()                         v
+		//         snd_pcm_elapsed()           fw_iso_context_flush_completions()
+		//  snd_pcm_stream_lock_irqsave()             disable_work_sync()
+		//                 v                                   v
+		//     wait until release of B                wait until A exits
 		if (!pcm->runtime->no_period_wakeup)
 			queue_work(system_highpri_wq, &s->period_work);
 	}
@@ -1055,8 +1071,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);
 }
@@ -1856,12 +1879,9 @@ unsigned long amdtp_domain_stream_pcm_pointer(struct amdtp_domain *d,
 	struct amdtp_stream *irq_target = d->irq_target;
 
 	if (irq_target && amdtp_stream_running(irq_target)) {
-		// use wq to prevent AB/BA deadlock competition for
-		// substream lock:
-		// fw_iso_context_flush_completions() acquires
-		// lock by ohci_flush_iso_completions(),
-		// amdtp-stream process_rx_packets() attempts to
-		// acquire same lock by snd_pcm_elapsed()
+		// The work item to call snd_pcm_period_elapsed() can reach here by the call of
+		// snd_pcm_ops.pointer(), however less packets would be available then. Therefore
+		// the following call is just for user process contexts.
 		if (current_work() != &s->period_work)
 			fw_iso_context_flush_completions(irq_target->context);
 	}
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

Re: [PATCH 5/5] ALSA: firewire: use nonatomic PCM operation

[Takashi Iwai]

On Wed, 04 Sep 2024 14:51:54 +0200,
Takashi Sakamoto wrote:
> 
> In the former commits, the callback of isochronous context runs on usual
> 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, then result in the delay of system event due to disabled IRQ so
> long. It is avertible to switch to nonatomic operation.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Feel free to take my ack:

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 5/5] ALSA: firewire: use nonatomic PCM operation

[Takashi Sakamoto]

On Wed, Sep 04, 2024 at 06:07:54PM +0200, Takashi Iwai wrote:
> On Wed, 04 Sep 2024 14:51:54 +0200,
> Takashi Sakamoto wrote:
> > 
> > In the former commits, the callback of isochronous context runs on usual
> > 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, then result in the delay of system event due to disabled IRQ so
> > long. It is avertible to switch to nonatomic operation.
> > 
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> Feel free to take my ack:
> 
> Reviewed-by: Takashi Iwai <tiwai@suse.de>

Thanks for your ack ;)


Regards

Takashi Sakamoto

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

[Takashi Sakamoto]

On Wed, Sep 04, 2024 at 09:51:49PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This series of changes updates my previous RFT[1] to apply for v6.12
> kernel. For the detail, please refer to the previous one.
> 
> To Iwai-san, this series includes the change for sound subsystem as
> well. All of changes are specific to ALSA firewire stack, so I would
> like to send it to Linus as the part of firewire subsystem updates if
> you do not mind it.
> 
> Changes from the RFT:
> * WQ_FREEZABLE is newly supported in the workqueue
> * Improve code comment in IEC 61883-1/6 packet streaming engine
> * Avoid dead lock in the calls of workqueue sync API
> 
> [1] https://lore.kernel.org/lkml/20240901110642.154523-1-o-takashi@sakamocchi.jp/
> 
> 
> Regards
> 
> Takashi Sakamoto (5):
>   firewire: core: allocate workqueue to handle isochronous contexts in
>     card
>   firewire: core: add local API to queue work item to workqueue specific
>     to isochronous contexts
>   firewire: ohci: operate IT/IR events in sleepable work process instead
>     of 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             | 33 ++++++++++++--
>  drivers/firewire/core-cdev.c             |  4 +-
>  drivers/firewire/core-iso.c              | 30 ++++++++++++-
>  drivers/firewire/core.h                  | 14 +++++-
>  drivers/firewire/ohci.c                  | 57 +++++++++++++++++++-----
>  include/linux/firewire.h                 |  3 ++
>  sound/firewire/amdtp-stream.c            | 34 +++++++++++---
>  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, 157 insertions(+), 27 deletions(-)

I applied all of them to for-next branch.


Regards

Takashi Sakamoto

firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 2

[Edmund Raile]

Hello Sakamoto-San, I came around to testing your patch [1], after RFT.

I've had to make the following changes to patch 1/5 again for it to apply to
mainline (d1f2d51b711a3b7f1ae1b46701c769c1d580fa7f), due to missing b171e20
from 2009 and a7ecbe9 from 2022.

@@ -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);

Then everything applied fine.

This resulted in 6.11.0-rc6-1-mainline-00326-gd1f2d51b711a-dirty.

Testing it with TI XIO2213B and RME Fireface 800 so far:

Initially I had a buffer freeze after 3 hours of continuous ALSA playback
from mpv:
  mpv --audio-device=alsa/sysdefault:CARD=Fireface800 Spor-Ignition.flac
accompanied by stresstest (mprime).

It didn't freeze/crash the kernel, just the audio buffer kept repeating.
Gone after power-cycling the interface and restarting playback.

Can't say with certainty whether it's related, have been unable to replicate
the issue for the past 3 days (good sign I hope).
That's why I was holding this message back a bit.

Kind regards,
Edmund Raile.

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

Re: firewire: use sleepable workqueue to handle 1394 OHCI IT/IR context events: test 2

[Takashi Sakamoto]

Hi,

On Thu, Sep 12, 2024 at 09:44:52PM +0000, Edmund Raile wrote:
> Hello Sakamoto-San, I came around to testing your patch [1], after RFT.
> 
> I've had to make the following changes to patch 1/5 again for it to apply to
> mainline (d1f2d51b711a3b7f1ae1b46701c769c1d580fa7f), due to missing b171e20
> from 2009 and a7ecbe9 from 2022.
> 
> @@ -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);
> 
> Then everything applied fine.
> 
> This resulted in 6.11.0-rc6-1-mainline-00326-gd1f2d51b711a-dirty.
> 
> Testing it with TI XIO2213B and RME Fireface 800 so far:
> 
> Initially I had a buffer freeze after 3 hours of continuous ALSA playback
> from mpv:
>   mpv --audio-device=alsa/sysdefault:CARD=Fireface800 Spor-Ignition.flac
> accompanied by stresstest (mprime).
> 
> It didn't freeze/crash the kernel, just the audio buffer kept repeating.
> Gone after power-cycling the interface and restarting playback.
> 
> Can't say with certainty whether it's related, have been unable to replicate
> the issue for the past 3 days (good sign I hope).
> That's why I was holding this message back a bit.
> 
> Kind regards,
> Edmund Raile.
> 
> Signed-off-by: Edmund Raile <edmund.raile@protonmail.com>
> Tested-by: Edmund Raile <edmund.raile@protonmail.com>
 
Thank you for your test. I've picked up your Tested-by tag to the
series.


Thanks

Takashi Sakamoto