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@xxxxxxx>
> ---
> 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
