Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	"Christian Eggers" <ceggers@arri.de>, <linux-iio@vger.kernel.org>
Subject: Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context
Date: Thu, 13 Aug 2020 15:55:19 +0100	[thread overview]
Message-ID: <20200813155519.00000684@Huawei.com> (raw)
In-Reply-To: <877du2louq.fsf@nanos.tec.linutronix.de>

On Thu, 13 Aug 2020 14:19:57 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:
> > On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote:  
> >> If you are running with forced IRQ threads the only thing that will then
> >> happen in the actual hard IRQ context is the launching of the IRQ threads.
> >> Th e IRQ handler of the device driver will run in a threaded IRQ.  
> >
> > So if it is really just the wakeup of the IRQ-thread then it should be
> > okay.
> > One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This
> > would invoke trig->ops->try_reenable callback if available.
> > I grepped and found
> > - bma180_trig_try_reen() 
> >   It appears to perform i2c_smbus_read_byte_data() and smbus sounds
> >   sleeping. I don't know if it attempts to acquire any spinlock_t but it
> >   will be wrong on RT.  
> 
> It's wrong even on !RT. i2c reads cannot be invoked from hard interrupt
> context.
> 

We would hit this (and resulting warnings) all the time if it actually
happened, so my suspicion is that it doesn't.

I think the path doesn't actually exist although it looks at first glance like it does.

The interrupt can only be enabled if there is someone using the trigger.
Thus usecount will be non zero and for at least one element 
trig->subirq[i].enabled == true

So we will decrement trig->usecount in the call to iio_trigger_notify_done
but never reach 0 thus the call to trig->ops->try_reenable never happens
in the hard interrupt context.

It does happen later when which ever driver we triggered finishes the
threaded part of it's handler and calls iio_trigger_notify_done, but that
is fine.

Assuming people agree with my analysis it would be good to make it explicit
that we cannot hit the problem path.  

Perhaps call a new iio_trigger_notify_no_needed() that simply does
the decrement without test, or does it with test and spits out a
warning if we hit 0.

Jonathan