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@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.
OK, makes sense. Will fix in v2.
Thanks for reviewing!
Takashi
