Re: [PATCH 10/11] ALSA: firewire: Replace tasklet with work

7 Sep 2020

On Sun, 06 Sep 2020 10:26:28 +0200,
Takashi Sakamoto wrote:
...
Hi,
On Thu, Sep 03, 2020 at 12:41:30PM +0200, Takashi Iwai wrote:
...
The tasklet is an old API that should be deprecated, usually can be
converted to another decent API.  In FireWire driver, a tasklet is
still used for offloading the AMDTP PCM stream handling.  It can be
achieved gracefully with a work queued, too.
This patch replaces the tasklet usage in firewire-lib driver with a
simple work.  The conversion is fairly straightforward but for the
in_interrupt() checks that are replaced with the check using the
current_work().
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/firewire/amdtp-stream-trace.h |  2 +-
 sound/firewire/amdtp-stream.c       | 25 +++++++++++++------------
 sound/firewire/amdtp-stream.h       |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)
After testing this patch, I agree with the usage of
'(current_work() == &s->period_work)' as an alternative of 'in_interrupt()'.
However, the usage is not appropriate for tracepoints event in the case.
...

diff --git a/sound/firewire/amdtp-stream-trace.h b/sound/firewire/amdtp-stream-trace.h
index 26e7cb555d3c..5386d548cada 100644
--- a/sound/firewire/amdtp-stream-trace.h
+++ b/sound/firewire/amdtp-stream-trace.h
@@ -49,7 +49,7 @@ TRACE_EVENT(amdtp_packet,
   	__entry->data_blocks = data_blocks;
   	__entry->data_block_counter = data_block_counter,
   	__entry->packet_index = s->packet_index;

__entry->irq = !!in_interrupt();




__entry->irq = (current_work() == &s->period_work);

__entry->index = index;
),
TP_printk(

The tracepoints event is probed in two contexts:

softirq for isochronous context to process hardware events of 1394 OHCI.
user task of ALSA PCM applications.

However, it's not probed in the workqueue task since the case is already
avoided carefully in below patch:
...
@@ -1184,7 +1185,7 @@ unsigned long amdtp_domain_stream_pcm_pointer(struct amdtp_domain *d,
if (irq_target && amdtp_stream_running(irq_target)) {
   	// This function is called in software IRQ context of

// period_tasklet or process context.




// period_work or process context.

//
// When the software IRQ context was scheduled by software IRQ
// context of IT contexts, queued packets were already handled.

@@ -1195,9 +1196,9 @@ unsigned long amdtp_domain_stream_pcm_pointer(struct amdtp_domain *d,
   	// immediately to keep better granularity of PCM pointer.
   	//
   	// Later, the process context will sometimes schedules software

// IRQ context of the period_tasklet. Then, no need to flush the




// IRQ context of the period_work. Then, no need to flush the

// queue by the same reason as described in the above


if (!in_interrupt()) {




if (current_work() != &s->period_work) {
// Queued packet should be processed without any kernel
// preemption to keep latency against bus cycle.
preempt_disable();



as long as testing, I can see no logs for the trancepoints event with the 'irq' field is 1.
I would like you to leave 'amdtp-stream-trace.h' as is by dropping the above change since
the irq field should record whether the context is softirq or user task.
OK, makes sense.  Will fix in v2.
Thanks for reviewing!
Takashi

    