From: "Bruno Prémont" <bonbons@linux-vserver.org>
To: Andrew de los Reyes <andrew-vger@gizmolabs.org>
Cc: David Herrmann <dh.herrmann@googlemail.com>,
Linux Input <linux-input@vger.kernel.org>,
Nestor Lopez Casado <nlopezcasad@logitech.com>,
Benjamin Tissoires <benjamin.tissoires@gmail.com>,
Jiri Kosina <jkosina@suse.cz>
Subject: Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
Date: Mon, 11 Feb 2013 22:05:26 +0100 [thread overview]
Message-ID: <20130211220526.6909cc0f@neptune.home> (raw)
In-Reply-To: <CAG_cf+cNqrveHTR+JqYueTafH7Jtv+VDebU4QNicfWY3dhY+7w@mail.gmail.com>
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
next prev parent reply other threads:[~2013-02-11 21:05 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-10 17:47 [PATCH] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
2013-02-11 15:42 ` David Herrmann
2013-02-11 17:55 ` Bruno Prémont
2013-02-11 19:13 ` Andrew de los Reyes
2013-02-11 21:05 ` Bruno Prémont [this message]
2013-02-14 5:08 ` Andrew de los Reyes
2013-02-14 7:06 ` Bruno Prémont
2013-02-17 1:43 ` Andrew de los Reyes
-- strict thread matches above, loose matches on Subject: below --
2013-02-14 5:07 Andrew de los Reyes
2013-02-18 13:08 ` Jiri Kosina
2013-02-25 13:03 ` Jiri Kosina
2013-02-25 13:09 ` David Herrmann
2013-02-25 13:15 ` Jiri Kosina
2013-02-25 17:16 ` Andrew de los Reyes
2012-11-25 18:48 Andrew de los Reyes
2012-11-26 17:56 ` Bruno Prémont
2012-11-26 18:34 ` Benjamin Tissoires
2012-11-26 18:54 ` David Herrmann
2012-11-26 20:50 ` Andrew de los Reyes
2012-12-04 8:04 ` Jiri Kosina
2012-12-03 13:17 ` Jiri Kosina
2012-12-03 17:53 ` Nestor Lopez Casado
2013-02-05 15:07 ` Jiri Kosina
2013-02-05 16:19 ` Nestor Lopez Casado
[not found] ` <CAG_cf+ev2y1MHNJFYZycbjQfNgk4hYYRaEoFsKkZyhzAzMgc1A@mail.gmail.com>
2013-02-05 17:22 ` David Herrmann
2013-02-05 17:50 ` Nestor Lopez Casado
2013-02-06 15:10 ` Jiri Kosina
2013-02-06 15:21 ` David Herrmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130211220526.6909cc0f@neptune.home \
--to=bonbons@linux-vserver.org \
--cc=andrew-vger@gizmolabs.org \
--cc=benjamin.tissoires@gmail.com \
--cc=dh.herrmann@googlemail.com \
--cc=jkosina@suse.cz \
--cc=linux-input@vger.kernel.org \
--cc=nlopezcasad@logitech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).