linux1394-devel Mailing List for IEEE 1394 for Linux (Page 2)
Brought to you by:
aeb,
bencollins
You can subscribe to this list here.
2000 |
Jan
|
Feb
|
Mar
(39) |
Apr
(154) |
May
(172) |
Jun
(237) |
Jul
(127) |
Aug
(135) |
Sep
(193) |
Oct
(175) |
Nov
(173) |
Dec
(148) |
---|---|---|---|---|---|---|---|---|---|---|---|---|
2001 |
Jan
(161) |
Feb
(225) |
Mar
(193) |
Apr
(158) |
May
(179) |
Jun
(292) |
Jul
(146) |
Aug
(134) |
Sep
(185) |
Oct
(190) |
Nov
(149) |
Dec
(161) |
2002 |
Jan
(186) |
Feb
(236) |
Mar
(254) |
Apr
(207) |
May
(189) |
Jun
(182) |
Jul
(202) |
Aug
(155) |
Sep
(149) |
Oct
(449) |
Nov
(191) |
Dec
(108) |
2003 |
Jan
(174) |
Feb
(242) |
Mar
(243) |
Apr
(255) |
May
(202) |
Jun
(290) |
Jul
(237) |
Aug
(178) |
Sep
(101) |
Oct
(153) |
Nov
(144) |
Dec
(95) |
2004 |
Jan
(162) |
Feb
(278) |
Mar
(282) |
Apr
(152) |
May
(127) |
Jun
(138) |
Jul
(94) |
Aug
(63) |
Sep
(64) |
Oct
(150) |
Nov
(102) |
Dec
(197) |
2005 |
Jan
(102) |
Feb
(172) |
Mar
(89) |
Apr
(158) |
May
(139) |
Jun
(160) |
Jul
(288) |
Aug
(89) |
Sep
(201) |
Oct
(92) |
Nov
(190) |
Dec
(139) |
2006 |
Jan
(121) |
Feb
(204) |
Mar
(133) |
Apr
(134) |
May
(91) |
Jun
(226) |
Jul
(122) |
Aug
(101) |
Sep
(144) |
Oct
(141) |
Nov
|
Dec
|
2023 |
Jan
(19) |
Feb
(1) |
Mar
(5) |
Apr
(5) |
May
(33) |
Jun
(17) |
Jul
|
Aug
|
Sep
(3) |
Oct
(1) |
Nov
(5) |
Dec
(40) |
2024 |
Jan
(26) |
Feb
(14) |
Mar
(26) |
Apr
(46) |
May
(17) |
Jun
(47) |
Jul
(23) |
Aug
(72) |
Sep
(33) |
Oct
|
Nov
|
Dec
|
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:16:31
|
(resend to linux1394-devel since the post is waiting for moderator approval. Please refer to the original post for any reply: https://lore.kernel.org/lkml/202...@sa.../) 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/202...@li.../ [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(-) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:16:29
|
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-t...@sa...> --- 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 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:16:28
|
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-t...@sa...> --- 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 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:07:10
|
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-t...@sa...> --- 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 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:07:03
|
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-t...@sa...> --- 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 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:07:02
|
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-t...@sa...> --- 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 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:06:59
|
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/202...@li.../ [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(-) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-09-01 11:06:59
|
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-t...@sa...> --- 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 |
From: Adam G. <ad...@po...> - 2024-08-31 10:18:17
|
On Sun, Aug 25, 2024 at 09:31:40PM +0100, Andrew Ferguson wrote: > I have encountered a bug in that some Macs are not detected in Linux, when > the Mac is being used in "Target Disk Mode". [...] > Mac models that do not work: > iBook G4 > Powerbook G4 Hi Andrew, I did a quick test with kernel 6.8-rc1 and a titanium Powerbook G4 (powerbook3,4 A1001). I held down T and turned on the Powerbook. After about 25 seconds, the FireWire logo appeared on the screen, and the drive appeared on Linux as sdb. I was able to read from the drive with dd. Can you provide steps to reproduce the problem? -- Adam |
From: Takashi S. <o-t...@sa...> - 2024-08-26 08:39:22
|
Hi, On Sun, Aug 25, 2024 at 09:31:40PM +0100, Andrew Ferguson wrote: > Hello, > > I firstly wanted to thank the maintainers for working on the ieee1394 > system for Linux. It's great to see continuing support for firewire in > Linux. > > I have encountered a bug in that some Macs are not detected in Linux, when > the Mac is being used in "Target Disk Mode". For those unfamiliar, Target > Disk Mode boots an older (firewire-compatible) Mac in a mode where > connecting it to another computer via firewire causes the Mac to appear as > an external firewire hard drive. See the Wikipedia article > <https://en.wikipedia.org/wiki/Target_Disk_Mode> for more information. To > be clear, I am not trying to run the Linux1394 system on an older Mac; I am > trying to connect a Mac in Target Disk Mode to a regular PC running Linux > over firewire. The Mac should appear as an external firewire hard drive. On > some Mac models, it works just fine. However on other models, it doesn't > work. If I connect one of these non-working models to a Mac running OS X, > it does work, so it does appear to be an issue with Linux1394. > > Mac models that work: > Macbook Pro late 2008 > iMac G4 > > Mac models that do not work: > iBook G4 > Powerbook G4 > > I have access to more Mac models to test if necessary - the benefits of > hoarding old electronics! > > If the maintainers are interested in investigating this issue, what sort of > debugging information can I provide? There's only a few lines in dmesg that > to my eyes look useful ("sbp_scsi_abort" is one) but I can provide full > dmesg output if helpful. I have a number of different Firewire PCI / PCIe > cards I can use, including one that should be able to run the "nosy" > ieee1394 packet sniffer (although I have no clue how to set that up!). > > Many thanks, and kind regards, > Andrew Thanks for the report. I'm sorry but I have less interests in the issue, since I have no actual hardware to investigate the issue (I'm a developer of ALSA, thus the center of my interest is multimedia protocol such like IEC 61883.) Therefore it is up to you. If you are willing to solve the issue, I would help you. Else, nothing changed. In Linux kernel v6.10, I added some tracepoints events[1]. These events are available to track asynchronous communication between Linux system and peer node. If the issue is caused by any kind of communication error, you could diagnose it. ``` $ ls -1 /sys/kernel/debug/tracing/events/firewire/ async_phy_inbound/ async_phy_outbound_initiate/ async_request_outbound_complete/ async_response_inbound/ async_response_outbound_initiate/ async_phy_outbound_complete/ async_request_inbound/ async_request_outbound_initiate/ async_response_outbound_complete/ ... ``` Additionally, I think the protocol in the case is SBP. The protocol specification is still available[2] and it would help your diagnose, too. [1] https://www.kernel.org/doc/html/latest/trace/events.html [2] http://ieee1394.docs.kernel.org/en/latest/specifications.html#the-serial-bus-protocol Regards Takashi Sakamoto |
From: Andrew F. <and...@gm...> - 2024-08-25 20:31:59
|
Hello, I firstly wanted to thank the maintainers for working on the ieee1394 system for Linux. It's great to see continuing support for firewire in Linux. I have encountered a bug in that some Macs are not detected in Linux, when the Mac is being used in "Target Disk Mode". For those unfamiliar, Target Disk Mode boots an older (firewire-compatible) Mac in a mode where connecting it to another computer via firewire causes the Mac to appear as an external firewire hard drive. See the Wikipedia article <https://en.wikipedia.org/wiki/Target_Disk_Mode> for more information. To be clear, I am not trying to run the Linux1394 system on an older Mac; I am trying to connect a Mac in Target Disk Mode to a regular PC running Linux over firewire. The Mac should appear as an external firewire hard drive. On some Mac models, it works just fine. However on other models, it doesn't work. If I connect one of these non-working models to a Mac running OS X, it does work, so it does appear to be an issue with Linux1394. Mac models that work: Macbook Pro late 2008 iMac G4 Mac models that do not work: iBook G4 Powerbook G4 I have access to more Mac models to test if necessary - the benefits of hoarding old electronics! If the maintainers are interested in investigating this issue, what sort of debugging information can I provide? There's only a few lines in dmesg that to my eyes look useful ("sbp_scsi_abort" is one) but I can provide full dmesg output if helpful. I have a number of different Firewire PCI / PCIe cards I can use, including one that should be able to run the "nosy" ieee1394 packet sniffer (although I have no clue how to set that up!). Many thanks, and kind regards, Andrew |
From: Takashi S. <o-t...@sa...> - 2024-08-21 14:29:37
|
Hi, On Thu, Aug 15, 2024 at 10:58:04PM +0800, Zijun Hu wrote: > From: Zijun Hu <qui...@qu...> > > To prepare for constifying the following old driver core API: > > struct device *device_find_child(struct device *dev, void *data, > int (*match)(struct device *dev, void *data)); > to new: > struct device *device_find_child(struct device *dev, const void *data, > int (*match)(struct device *dev, const void *data)); > > The new API does not allow its match function (*match)() to modify > caller's match data @*data, but lookup_existing_device() as the old > API's match function indeed modifies relevant match data, so it is not > suitable for the new API any more, fixed by implementing a equivalent > fw_device_find_child() instead of the old API usage. > > Signed-off-by: Zijun Hu <qui...@qu...> > --- > drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) Please drop this patch from your series since I applied another patch[1] to for-next branch. [1] https://lore.kernel.org/r/202...@sa... Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-08-21 14:26:03
|
On Tue, Aug 20, 2024 at 10:21:32PM +0900, Takashi Sakamoto wrote: > When detecting updates of bus topology, the data of fw_device is newly > allocated and caches the content of configuration ROM from the > corresponding node. Then, the tree of device is sought to find the > previous data of fw_device corresponding to the node. If found, the > previous data is updated and reused and the data of fw_device newly > allocated is going to be released. > > The above procedure is done in the call of device_find_child(), however it > is a bit abusing against the intention of the helper function, since it is > preferable to find only without updating. > > This commit splits the update outside of the call. > > Cc: Zijun Hu <zij...@ic...> > Signed-off-by: Takashi Sakamoto <o-t...@sa...> > --- > drivers/firewire/core-device.c | 116 +++++++++++++++++---------------- > 1 file changed, 59 insertions(+), 57 deletions(-) Applied to for-next branch. Regards Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-08-20 13:21:44
|
When detecting updates of bus topology, the data of fw_device is newly allocated and caches the content of configuration ROM from the corresponding node. Then, the tree of device is sought to find the previous data of fw_device corresponding to the node. If found, the previous data is updated and reused and the data of fw_device newly allocated is going to be released. The above procedure is done in the call of device_find_child(), however it is a bit abusing against the intention of the helper function, since it is preferable to find only without updating. This commit splits the update outside of the call. Cc: Zijun Hu <zij...@ic...> Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 116 +++++++++++++++++---------------- 1 file changed, 59 insertions(+), 57 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index f71e118ef60a..a99fe35f1f0d 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -928,56 +928,6 @@ static void fw_device_update(struct work_struct *work) device_for_each_child(&device->device, NULL, update_unit); } -/* - * If a device was pending for deletion because its node went away but its - * bus info block and root directory header matches that of a newly discovered - * device, revive the existing fw_device. - * The newly allocated fw_device becomes obsolete instead. - */ -static int lookup_existing_device(struct device *dev, void *data) -{ - struct fw_device *old = fw_device(dev); - struct fw_device *new = data; - struct fw_card *card = new->card; - int match = 0; - - if (!is_fw_device(dev)) - return 0; - - guard(rwsem_read)(&fw_device_rwsem); // serialize config_rom access - guard(spinlock_irq)(&card->lock); // serialize node access - - if (memcmp(old->config_rom, new->config_rom, 6 * 4) == 0 && - atomic_cmpxchg(&old->state, - FW_DEVICE_GONE, - FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { - struct fw_node *current_node = new->node; - struct fw_node *obsolete_node = old->node; - - new->node = obsolete_node; - new->node->data = new; - old->node = current_node; - old->node->data = old; - - old->max_speed = new->max_speed; - old->node_id = current_node->node_id; - smp_wmb(); /* update node_id before generation */ - old->generation = card->generation; - old->config_rom_retries = 0; - fw_notice(card, "rediscovered device %s\n", dev_name(dev)); - - old->workfn = fw_device_update; - fw_schedule_device_work(old, 0); - - if (current_node == card->root_node) - fw_schedule_bm_work(card, 0); - - match = 1; - } - - return match; -} - enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, }; static void set_broadcast_channel(struct fw_device *device, int generation) @@ -1038,12 +988,24 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen) return 0; } +static int compare_configuration_rom(struct device *dev, void *data) +{ + const struct fw_device *old = fw_device(dev); + const u32 *config_rom = data; + + if (!is_fw_device(dev)) + return 0; + + // Compare the bus information block and root_length/root_crc. + return !memcmp(old->config_rom, config_rom, 6 * 4); +} + static void fw_device_init(struct work_struct *work) { struct fw_device *device = container_of(work, struct fw_device, work.work); struct fw_card *card = device->card; - struct device *revived_dev; + struct device *found; u32 minor; int ret; @@ -1071,13 +1033,53 @@ static void fw_device_init(struct work_struct *work) return; } - revived_dev = device_find_child(card->device, - device, lookup_existing_device); - if (revived_dev) { - put_device(revived_dev); - fw_device_release(&device->device); + // If a device was pending for deletion because its node went away but its bus info block + // and root directory header matches that of a newly discovered device, revive the + // existing fw_device. The newly allocated fw_device becomes obsolete instead. + // + // serialize config_rom access. + scoped_guard(rwsem_read, &fw_device_rwsem) { + found = device_find_child(card->device, (void *)device->config_rom, + compare_configuration_rom); + } + if (found) { + struct fw_device *reused = fw_device(found); + + if (atomic_cmpxchg(&reused->state, + FW_DEVICE_GONE, + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) { + // serialize node access + scoped_guard(spinlock_irq, &card->lock) { + struct fw_node *current_node = device->node; + struct fw_node *obsolete_node = reused->node; + + device->node = obsolete_node; + device->node->data = device; + reused->node = current_node; + reused->node->data = reused; + + reused->max_speed = device->max_speed; + reused->node_id = current_node->node_id; + smp_wmb(); /* update node_id before generation */ + reused->generation = card->generation; + reused->config_rom_retries = 0; + fw_notice(card, "rediscovered device %s\n", + dev_name(found)); + + reused->workfn = fw_device_update; + fw_schedule_device_work(reused, 0); + + if (current_node == card->root_node) + fw_schedule_bm_work(card, 0); + } - return; + put_device(found); + fw_device_release(&device->device); + + return; + } + + put_device(found); } device_initialize(&device->device); -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-18 01:48:47
|
On Sat, Aug 17, 2024 at 06:11:28PM +0900, Takashi Sakamoto wrote: > A commit 404957c1e207 ("firewire: ohci: use guard macro to serialize > accesses to phy registers") refactored initiated_reset() helper function, > while the error path was changed wrongly. > > This commit fixes the bug. > > Reported-by: Dan Carpenter <dan...@li...> > Fixes: 404957c1e207 ("firewire: ohci: use guard macro to serialize accesses to phy registers") > Signed-off-by: Takashi Sakamoto <o-t...@sa...> > --- > drivers/firewire/ohci.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) Applied to for-next branch. Regards Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-08-17 09:11:41
|
A commit 404957c1e207 ("firewire: ohci: use guard macro to serialize accesses to phy registers") refactored initiated_reset() helper function, while the error path was changed wrongly. This commit fixes the bug. Reported-by: Dan Carpenter <dan...@li...> Fixes: 404957c1e207 ("firewire: ohci: use guard macro to serialize accesses to phy registers") Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 53132eae37fe..a3a37955b174 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1919,7 +1919,7 @@ static int get_self_id_pos(struct fw_ohci *ohci, u32 self_id, return i; } -static bool initiated_reset(struct fw_ohci *ohci) +static int detect_initiated_reset(struct fw_ohci *ohci, bool *is_initiated_reset) { int reg; @@ -1946,7 +1946,9 @@ static bool initiated_reset(struct fw_ohci *ohci) return reg; // bit 3 indicates "initiated reset" - return !!((reg & 0x08) == 0x08); + *is_initiated_reset = !!((reg & 0x08) == 0x08); + + return 0; } /* @@ -1956,7 +1958,8 @@ static bool initiated_reset(struct fw_ohci *ohci) */ static int find_and_insert_self_id(struct fw_ohci *ohci, int self_id_count) { - int reg, i, pos; + int reg, i, pos, err; + bool is_initiated_reset; u32 self_id = 0; // link active 1, speed 3, bridge 0, contender 1, more packets 0. @@ -1985,7 +1988,6 @@ static int find_and_insert_self_id(struct fw_ohci *ohci, int self_id_count) for (i = 0; i < 3; i++) { enum phy_packet_self_id_port_status status; - int err; err = get_status_for_port(ohci, i, &status); if (err < 0) @@ -1994,7 +1996,10 @@ static int find_and_insert_self_id(struct fw_ohci *ohci, int self_id_count) self_id_sequence_set_port_status(&self_id, 1, i, status); } - phy_packet_self_id_zero_set_initiated_reset(&self_id, initiated_reset(ohci)); + err = detect_initiated_reset(ohci, &is_initiated_reset); + if (err < 0) + return err; + phy_packet_self_id_zero_set_initiated_reset(&self_id, is_initiated_reset); pos = get_self_id_pos(ohci, self_id, self_id_count); if (pos >= 0) { -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-15 14:41:07
|
Hi, On Thu, Aug 15, 2024 at 10:08:27AM +0300, Dan Carpenter wrote: > Hello Takashi Sakamoto, > > Commit 404957c1e207 ("firewire: ohci: use guard macro to serialize > accesses to phy registers") from Aug 5, 2024 (linux-next), leads to > the following Smatch static checker warning: > > drivers/firewire/ohci.c:1927 initiated_reset() > warn: potential negative cast to bool 'reg' > > drivers/firewire/ohci.c > 1923 static bool initiated_reset(struct fw_ohci *ohci) > 1924 { > 1925 int reg; > 1926 > --> 1927 guard(mutex)(&ohci->phy_reg_mutex); > > I'm not sure why Smatch is blaming this line... > > 1928 > 1929 // Select page 7 > 1930 reg = write_phy_reg(ohci, 7, 0xe0); > 1931 if (reg < 0) > 1932 return reg; > > This is a bool function. I think these should be return false, instead of > return negative/true. > > 1933 > 1934 reg = read_phy_reg(ohci, 8); > 1935 if (reg < 0) > 1936 return reg; > ^^^^^^^^^^ > > 1937 > 1938 // set PMODE bit > 1939 reg |= 0x40; > 1940 reg = write_phy_reg(ohci, 8, reg); > 1941 if (reg < 0) > 1942 return reg; > ^^^^^^^^^^ > > 1943 > 1944 // read register 12 > 1945 reg = read_phy_reg(ohci, 12); > 1946 if (reg < 0) > 1947 return reg; > ^^^^^^^^^^ > > 1948 > 1949 // bit 3 indicates "initiated reset" > 1950 return !!((reg & 0x08) == 0x08); > 1951 } Indeed. It is my mistake, and I'll fix it soon. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-08-14 23:46:51
|
On Wed, Aug 14, 2024 at 10:12:19PM +0900, Takashi Sakamoto wrote: > Hi, > > This series of changes include minor code refactoring in core function > and 1394 OHCI driver. > > > Takashi Sakamoto (3): > firewire: ohci: use helper macro for compiler aligned attribute > firewire: ohci: remove unused wrapper macro for dev_info() > firewire: core/ohci: minor refactoring for computation of > configuration ROM size > > drivers/firewire/core-device.c | 3 ++- > drivers/firewire/ohci.c | 5 ++--- > 2 files changed, 4 insertions(+), 4 deletions(-) Applied to for-next branch. Regards Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-08-14 13:12:43
|
The size of space for configuration ROM is defined by IEEE 1212. The start and end offsets are available as some macros in UAPI header. This commit uses these macros to compute the size. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 3 ++- drivers/firewire/ohci.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index 32ac0f115793..f71e118ef60a 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -564,7 +564,8 @@ static int read_rom(struct fw_device *device, return rcode; } -#define MAX_CONFIG_ROM_SIZE 256 +// By quadlet unit. +#define MAX_CONFIG_ROM_SIZE ((CSR_CONFIG_ROM_END - CSR_CONFIG_ROM) / sizeof(u32)) /* * Read the bus info block, perform a speed probe, and read all of the rest of diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 979f1e1f2d16..53132eae37fe 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -174,7 +174,7 @@ struct iso_context { u8 tags; }; -#define CONFIG_ROM_SIZE 1024 +#define CONFIG_ROM_SIZE (CSR_CONFIG_ROM_END - CSR_CONFIG_ROM) struct fw_ohci { struct fw_card card; -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-14 13:12:41
|
The ohci_info() macro is a thin wrapper of dev_info(), while it is never used. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 198c96d75155..979f1e1f2d16 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -50,7 +50,6 @@ static u32 cond_le32_to_cpu(__le32 value, bool has_be_header_quirk); #define CREATE_TRACE_POINTS #include <trace/events/firewire_ohci.h> -#define ohci_info(ohci, f, args...) dev_info(ohci->card.device, f, ##args) #define ohci_notice(ohci, f, args...) dev_notice(ohci->card.device, f, ##args) #define ohci_err(ohci, f, args...) dev_err(ohci->card.device, f, ##args) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-14 13:12:40
|
The __aligned() macro has been available since v4.19 kernel by a commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive"). This commit replaces with the macro. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/ohci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index e1d24e0ec991..198c96d75155 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -77,7 +77,7 @@ struct descriptor { __le32 branch_address; __le16 res_count; __le16 transfer_status; -} __attribute__((aligned(16))); +} __aligned(16); #define CONTROL_SET(regs) (regs) #define CONTROL_CLEAR(regs) ((regs) + 4) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-14 13:12:34
|
Hi, This series of changes include minor code refactoring in core function and 1394 OHCI driver. Takashi Sakamoto (3): firewire: ohci: use helper macro for compiler aligned attribute firewire: ohci: remove unused wrapper macro for dev_info() firewire: core/ohci: minor refactoring for computation of configuration ROM size drivers/firewire/core-device.c | 3 ++- drivers/firewire/ohci.c | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-14 00:12:21
|
On Tue, Aug 13, 2024 at 08:52:05AM +0900, Takashi Sakamoto wrote: > Hi, > > In core function, the instances of several types of client resources > are maintained by IDR, and the index value of each resource is passed to > user space application as handle. As of kernel v6.0, IDR has been > superseded by XArray and deprecated. > > This series of changes is to obsolete the usage of IDR with XArray. > > Takashi Sakamoto (5): > firewire: core: minor code refactoring to release client resource > firewire: core: add helper functions to convert to parent resource > structure > firewire: core: add helper function to detect data of iso resource > structure > firewire: core: code refactoring to use idr_for_each_entry() macro > instead of idr_for_each() function > firewire: core: use xarray instead of idr to maintain client resource > > drivers/firewire/core-cdev.c | 170 +++++++++++++++++++---------------- > drivers/firewire/core.h | 1 - > 2 files changed, 94 insertions(+), 77 deletions(-) Applied to for-next branch. Regards Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-08-12 23:52:31
|
In core function, the instances of some client resource structures are maintained by IDR. As of kernel v6.0, IDR has been superseded by XArray and deprecated. This commit replaces the usage of IDR with XArray to maintain the resource instances. The instance of XArray is allocated per client with XA_FLAGS_ALLOC1 so that the index of allocated entry is greater than zero and returns to user space client as handle of the resource. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-cdev.c | 66 +++++++++++++++++++----------------- drivers/firewire/core.h | 1 - 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 83d25327c1d3..3ea220d96c31 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -14,7 +14,6 @@ #include <linux/errno.h> #include <linux/firewire.h> #include <linux/firewire-cdev.h> -#include <linux/idr.h> #include <linux/irqflags.h> #include <linux/jiffies.h> #include <linux/kernel.h> @@ -54,7 +53,7 @@ struct client { spinlock_t lock; bool in_shutdown; - struct idr resource_idr; + struct xarray resource_xa; struct list_head event_list; wait_queue_head_t wait; wait_queue_head_t tx_flush_wait; @@ -297,7 +296,7 @@ static int fw_device_op_open(struct inode *inode, struct file *file) client->device = device; spin_lock_init(&client->lock); - idr_init(&client->resource_idr); + xa_init_flags(&client->resource_xa, XA_FLAGS_ALLOC1 | XA_FLAGS_LOCK_BH); INIT_LIST_HEAD(&client->event_list); init_waitqueue_head(&client->wait); init_waitqueue_head(&client->tx_flush_wait); @@ -407,7 +406,7 @@ static void queue_bus_reset_event(struct client *client) { struct bus_reset_event *e; struct client_resource *resource; - int id; + unsigned long index; e = kzalloc(sizeof(*e), GFP_KERNEL); if (e == NULL) @@ -420,7 +419,7 @@ static void queue_bus_reset_event(struct client *client) guard(spinlock_irq)(&client->lock); - idr_for_each_entry(&client->resource_idr, resource, id) { + xa_for_each(&client->resource_xa, index, resource) { if (is_iso_resource(resource)) schedule_iso_resource(to_iso_resource(resource), 0); } @@ -501,31 +500,33 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg) return ret ? -EFAULT : 0; } -static int add_client_resource(struct client *client, - struct client_resource *resource, gfp_t gfp_mask) +static int add_client_resource(struct client *client, struct client_resource *resource, + gfp_t gfp_mask) { - bool preload = gfpflags_allow_blocking(gfp_mask); int ret; - if (preload) - idr_preload(gfp_mask); - scoped_guard(spinlock_irqsave, &client->lock) { - if (client->in_shutdown) + u32 index; + + if (client->in_shutdown) { ret = -ECANCELED; - else - ret = idr_alloc(&client->resource_idr, resource, 0, 0, GFP_NOWAIT); + } else { + if (gfpflags_allow_blocking(gfp_mask)) { + ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b, + GFP_NOWAIT); + } else { + ret = xa_alloc_bh(&client->resource_xa, &index, resource, + xa_limit_32b, GFP_NOWAIT); + } + } if (ret >= 0) { - resource->handle = ret; + resource->handle = index; client_get(client); if (is_iso_resource(resource)) schedule_iso_resource(to_iso_resource(resource), 0); } } - if (preload) - idr_preload_end(); - return ret < 0 ? ret : 0; } @@ -533,17 +534,18 @@ static int release_client_resource(struct client *client, u32 handle, client_resource_release_fn_t release, struct client_resource **return_resource) { + unsigned long index = handle; struct client_resource *resource; scoped_guard(spinlock_irq, &client->lock) { if (client->in_shutdown) return -EINVAL; - resource = idr_find(&client->resource_idr, handle); + resource = xa_load(&client->resource_xa, index); if (!resource || resource->release != release) return -EINVAL; - idr_remove(&client->resource_idr, handle); + xa_erase(&client->resource_xa, handle); } if (return_resource) @@ -566,9 +568,10 @@ static void complete_transaction(struct fw_card *card, int rcode, u32 request_ts { struct outbound_transaction_event *e = data; struct client *client = e->client; + unsigned long index = e->r.resource.handle; scoped_guard(spinlock_irqsave, &client->lock) { - idr_remove(&client->resource_idr, e->r.resource.handle); + xa_erase(&client->resource_xa, index); if (client->in_shutdown) wake_up(&client->tx_flush_wait); } @@ -619,7 +622,7 @@ static void complete_transaction(struct fw_card *card, int rcode, u32 request_ts break; } - /* Drop the idr's reference */ + // Drop the xarray's reference. client_put(client); } @@ -1317,6 +1320,7 @@ static void iso_resource_work(struct work_struct *work) struct iso_resource *r = container_of(work, struct iso_resource, work.work); struct client *client = r->client; + unsigned long index = r->resource.handle; int generation, channel, bandwidth, todo; bool skip, free, success; @@ -1351,7 +1355,7 @@ static void iso_resource_work(struct work_struct *work) todo == ISO_RES_ALLOC_ONCE); /* * Is this generation outdated already? As long as this resource sticks - * in the idr, it will be scheduled again for a newer generation or at + * in the xarray, it will be scheduled again for a newer generation or at * shutdown. */ if (channel == -EAGAIN && @@ -1366,10 +1370,10 @@ static void iso_resource_work(struct work_struct *work) if (r->todo == ISO_RES_ALLOC) r->todo = ISO_RES_REALLOC; // Allocation or reallocation failure? Pull this resource out of the - // idr and prepare for deletion, unless the client is shutting down. + // xarray and prepare for deletion, unless the client is shutting down. if (r->todo == ISO_RES_REALLOC && !success && !client->in_shutdown && - idr_remove(&client->resource_idr, r->resource.handle)) { + xa_erase(&client->resource_xa, index)) { client_put(client); free = true; } @@ -1839,11 +1843,11 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma) static bool has_outbound_transactions(struct client *client) { struct client_resource *resource; - int id; + unsigned long index; guard(spinlock_irq)(&client->lock); - idr_for_each_entry(&client->resource_idr, resource, id) { + xa_for_each(&client->resource_xa, index, resource) { if (is_outbound_transaction_resource(resource)) return true; } @@ -1856,7 +1860,7 @@ static int fw_device_op_release(struct inode *inode, struct file *file) struct client *client = file->private_data; struct event *event, *next_event; struct client_resource *resource; - int id; + unsigned long index; scoped_guard(spinlock_irq, &client->device->card->lock) list_del(&client->phy_receiver_link); @@ -1870,17 +1874,17 @@ static int fw_device_op_release(struct inode *inode, struct file *file) if (client->buffer.pages) fw_iso_buffer_destroy(&client->buffer, client->device->card); - /* Freeze client->resource_idr and client->event_list */ + // Freeze client->resource_xa and client->event_list. scoped_guard(spinlock_irq, &client->lock) client->in_shutdown = true; wait_event(client->tx_flush_wait, !has_outbound_transactions(client)); - idr_for_each_entry(&client->resource_idr, resource, id) { + xa_for_each(&client->resource_xa, index, resource) { resource->release(client, resource); client_put(client); } - idr_destroy(&client->resource_idr); + xa_destroy(&client->resource_xa); list_for_each_entry_safe(event, next_event, &client->event_list, link) kfree(event); diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 8cace026090c..57d101c01e36 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -7,7 +7,6 @@ #include <linux/dma-mapping.h> #include <linux/fs.h> #include <linux/list.h> -#include <linux/idr.h> #include <linux/xarray.h> #include <linux/mm_types.h> #include <linux/rwsem.h> -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-08-12 23:52:30
|
This commit is a preparation to use xa_for_each() macro. Current implementation uses idr_for_each() function and has a disadvantage to replace with the macro. The IDR framework has idr_for_each_entry() macro for the similar purpose. This commit replace the function with the macro with minor code refactoring. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-cdev.c | 64 +++++++++++++++++------------------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 6fe2a2ea9869..83d25327c1d3 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -166,6 +166,14 @@ static int is_iso_resource(const struct client_resource *resource) return resource->release == release_iso_resource; } +static void release_transaction(struct client *client, + struct client_resource *resource); + +static int is_outbound_transaction_resource(const struct client_resource *resource) +{ + return resource->release == release_transaction; +} + static void schedule_iso_resource(struct iso_resource *r, unsigned long delay) { client_get(r->client); @@ -173,12 +181,6 @@ static void schedule_iso_resource(struct iso_resource *r, unsigned long delay) client_put(r->client); } -static void schedule_if_iso_resource(struct client_resource *resource) -{ - if (is_iso_resource(resource)) - schedule_iso_resource(to_iso_resource(resource), 0); -} - /* * dequeue_event() just kfree()'s the event, so the event has to be * the first field in a struct XYZ_event. @@ -401,16 +403,11 @@ static void for_each_client(struct fw_device *device, callback(c); } -static int schedule_reallocations(int id, void *p, void *data) -{ - schedule_if_iso_resource(p); - - return 0; -} - static void queue_bus_reset_event(struct client *client) { struct bus_reset_event *e; + struct client_resource *resource; + int id; e = kzalloc(sizeof(*e), GFP_KERNEL); if (e == NULL) @@ -423,7 +420,10 @@ static void queue_bus_reset_event(struct client *client) guard(spinlock_irq)(&client->lock); - idr_for_each(&client->resource_idr, schedule_reallocations, client); + idr_for_each_entry(&client->resource_idr, resource, id) { + if (is_iso_resource(resource)) + schedule_iso_resource(to_iso_resource(resource), 0); + } } void fw_device_cdev_update(struct fw_device *device) @@ -518,7 +518,8 @@ static int add_client_resource(struct client *client, if (ret >= 0) { resource->handle = ret; client_get(client); - schedule_if_iso_resource(resource); + if (is_iso_resource(resource)) + schedule_iso_resource(to_iso_resource(resource), 0); } } @@ -1835,35 +1836,27 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma) return ret; } -static int is_outbound_transaction_resource(int id, void *p, void *data) +static bool has_outbound_transactions(struct client *client) { - struct client_resource *resource = p; - - return resource->release == release_transaction; -} + struct client_resource *resource; + int id; -static int has_outbound_transactions(struct client *client) -{ guard(spinlock_irq)(&client->lock); - return idr_for_each(&client->resource_idr, is_outbound_transaction_resource, NULL); -} - -static int shutdown_resource(int id, void *p, void *data) -{ - struct client_resource *resource = p; - struct client *client = data; - - resource->release(client, resource); - client_put(client); + idr_for_each_entry(&client->resource_idr, resource, id) { + if (is_outbound_transaction_resource(resource)) + return true; + } - return 0; + return false; } static int fw_device_op_release(struct inode *inode, struct file *file) { struct client *client = file->private_data; struct event *event, *next_event; + struct client_resource *resource; + int id; scoped_guard(spinlock_irq, &client->device->card->lock) list_del(&client->phy_receiver_link); @@ -1883,7 +1876,10 @@ static int fw_device_op_release(struct inode *inode, struct file *file) wait_event(client->tx_flush_wait, !has_outbound_transactions(client)); - idr_for_each(&client->resource_idr, shutdown_resource, client); + idr_for_each_entry(&client->resource_idr, resource, id) { + resource->release(client, resource); + client_put(client); + } idr_destroy(&client->resource_idr); list_for_each_entry_safe(event, next_event, &client->event_list, link) -- 2.43.0 |