On Tue, Feb 12, 2019 at 08:47:30PM +0000, Jonathan Cameron wrote:
> On Mon, 11 Feb 2019 17:30:18 -0500
> Sven Van Asbroeck <thesven73@xxxxxxxxx> wrote:
>
> > On Mon, Feb 11, 2019 at 4:27 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > >
> > > Agreed. Or potentially just use regmap_bulk_read and rely on
> > > the regmap internal locking to do it for you.
> >
> > Neat solution. But it may only work correctly iff regmap_bulk_read()
> > reads the low
> > address first. I'm not sure if this function has that guarantee. If
> > somebody changes
> > the read order, the driver will break. But I think I'm being overly
> > paranoid here :)
>
> Good question on whether it is guaranteed to read in increasing register
> order (I didn't actually check the addresses are in increasing order
> but assume they are or you would have pointed that out ;)
>
> That strikes me as behaviour that should probably be documented as long
> as it is true currently.
>
> >
> > > So yes, it's more than possible that userspace won't get the same number
> > > of events as samples taken over the limit, but I don't know why we care.
> > > We can about missing a threshold being passed entirely, not about knowing
> > > how many samples we were above it for.
> >
> > I suspect that we run a small risk of losing an event, like so:
> >
> > PS (12.5 ms)
> > --> interrupt -> iio event
> More interesting if this one never happened, so we got a one off proximity
> event missed.
>
> > ALS (100 ms)
> > --> interrupt -> iio event
> > PS (12.5 ms)
> > --> interrupt ========= no iio event generated
> > ALS (100 ms)
> > --> interrupt -> iio event
> >
> > To see why, imagine that the scheduler decides to move away from the
> > threaded interrupt
> > handler right before ap3216c_clear_int(). Say 20ms, which I know is a
> > loooong time,
> > but bear with me, the point is that it _could_ happen as we're not a RTOS.
> >
> > static irqreturn_t ap3216c_event_handler(int irq, void *p)
> > {
> > /* imagine ALS interrupt came in, INT_STATUS is 0b01 */
> > regmap_read(data->regmap, AP3216C_INT_STATUS, &status);
> > if (status & mask1) iio_push_event(PROX);
> > if (status & mask2) iio_push_event(LIGHT);
> >
> > /* imagine schedule happens here */
> > msleep(20);
> > /* while we were not running, PS interrupt came in
> > INT_STATUS is now 0b11
> > yet no new interrupt is generated, as we are ONESHOT
> > */
> > ap3216c_clear_int(data);
> > /* clears both bits, interrupt line goes low.
> > knowledge that the PS interrupt came in is now lost */
> > }
> >
> > Not sure if that's acceptable driver behaviour. In real life it
> > probably wouldn't matter much,
> > except for occasional added latency maybe ?
> Good point, I'd missed that a single clear was clearing both bits
> rather than just the one we thought had fired.
>
> If we clear just the right one, (which I think we can do from
> the datasheet
> "1: Software clear after writing 1 into address 0x01 each bit#"
> However the code isn't writing a 3 in that clear, so I'm not
> sure if the datasheet is correct or not...
>
> and it is a level interrupt (which I think it is?) then we would
> be safe against this miss.
>
> If either we can only globally clear or it's not a level interrupt
> there isn't much we can do to avoid a miss, it's just a bad hardware
> design.
This totally makes sense, obviously something I had missed.
I think you are right, if each INT bit is cleared individually,
then that second event won't be lost.
I'll take a closer look at the ideas put forth by you and Sven here on
using mutex on some of the other cases (such as the write_event_config
vs event_handler race condition) and put those into v2.
Thanks again for all the constructive feedback.
-Bobby
