On 02/09/16 16:08, Lars-Peter Clausen wrote:
> Hi,
>
> A common pattern that we see with drivers that implement events is the
> following. I did not check all driver, but all those that I checked followed
> this pattern.
>
> irqreturn_t event_callback(int irq, void *devid)
> {
> struct iio_dev *indio_dev = devid;
> ...
> iio_push_event(indio_dev, ...);
>
> return IRQ_HANDLED;
> }
>
> int driver_probe(struct device *dev)
> {
> struct iio_dev *indio_dev;
>
> indio_dev = iio_device_alloc(...);
>
> request_irq(event_irq, event_callback, ..., indio_dev);
>
> return iio_device_register(indio_dev);
> }
Most of the time this is actually fine as we know the hardware
is in a state where it won't generate interrupts until they
are explicitly enabled from userspace (which requires
iio_device_register to have occurred).
However you are quite correct in thinking this isn't always the
case and we have a race to clean up here.
>
> Now iio_push_event() accessed indio_dev->event_interface. The
> event_interface is only allocated and assigned in iio_device_register()
> though. This means there is a window of opportunity where the interrupt is
> live and can trigger, but event_interface is still NULL. So we'll hit a NULL
> pointer dereference if the IRQ fires before iio_device_register() completes.
>
> I'm a bit conflicted on what is the best way to resolve this. On one hand
> the correct approach appears to be to simply delay the requesting of the IRQ
> until iio_device_register() has completed.
I'm not keen on the churn that would cause.
>On the other hand it is possible
> to argue that users should be able to expect that it is safe to call APIs
> that take a struct iio_dev if iio_device_alloc() succeeded.
Agreed. This is explicitly allowed in the equivalent in input.
See input_event in input.c description text.
>
> The later approach also has the advantage that we only need to update
> iio_push_event() rather than all drivers that use it. But raises the obvious
> question what is the right behavior of iio_push_event() in case the event
> interface has not been registered yet? Return an error? What should the
> caller do if it encounters an error? Or maybe just silently become a no-op?
Input just does it by no-op.
So what cases would that not make sense?
* We want an event to say a channel is already above a threshold level.
* err.
For the first one it's hardware dependent whether an interrupt even occurs
in that case so I'd have no problem with us making it disappear in all cases.
Jonathan
>
> - Lars
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
