On Mon, 11 February 2013 Andrew de los Reyes wrote:
> > > > @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
> > > > {
> > > > struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> > > > struct hid_driver *hdrv;
> > > > + int ret = 0;
> > > >
> > > > if (down_interruptible(&hdev->driver_lock))
> > > > return -EINTR;
> > > > + if (down_interruptible(&hdev->driver_input_lock)) {
> > > > + ret = -EINTR;
> > > > + goto unlock_driver_lock;
> > > > + }
> > > > + hdev->io_started = false;
> > >
> > > If we lock driver_input_lock during remove, we might lose important
> > > packages here because the input handler drops them.
> > >
> > > Drivers that require this should deal with it properly, but you might
> > > get annoying timeouts during remove if you do I/O and lose an
> > > important packet here.
> > >
> > > But I think the proper way to fix this is to move I/O handling into
> > > workqueues instead of interrupt-context so we can actually sleep in
> > > handle_input. So I think this is fine.
> >
> > Though the driver would suffer the same timeout if it does not shortcut
> > them when the device is being physically unplugged.
> > So in both cases the driver does need to take explicit action in ->remove()
> > probably even before reenabling I/O in order to "cancel" pending activity
> > prior to doing the removal reconfiguration on logical unplug.
>
> The kernel already blocks input during remove(); this patch doesn't
> change that behavior. We could have another API that allows drivers to
> opt-out of this behavior, but maybe that should be another patch.
I was mostly replying to David on the timeout part.
But sure it is for another patch.
There are two case here anyhow:
- physical unplug
Here it will never make sense to restart I/O as there device is gone
- logical unplug
In this case, maybe driver could tell which variant it prefers, stopping
I/O itself during remove() or having I/O stopped by HID (with option to
temporarily restart it as needed)
In the case driver specifies "don't stop I/O on logical unplug" driver
needs to be able to check current state of I/O. Either using a
hid_device_io_can_start() or having hid_device_io_start() having a return
value of 0 for successful start (even if I/O had never been stopped) and
return -ENODEV if the device is gone.
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
