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

[Takashi Iwai]

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 at 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