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

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.

Thanks

Takashi Sakamoto