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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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