Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

Hi Nestor

On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes
[off-list ref] wrote:

Yes, I'm hoping to upstream soon as part of a patch series to get WTP
support in. The current version of the commit you are asking about is
specifically here:
https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c

The main difference from before is that there is a new return value (1)
which signifies that the event lock has been been up()ed, so the hid-core
doesn't need to call up() itself. This way during probe(), a driver can
allow incoming events continuously from midway through probe() indefinitely.

The patch looks nice. Some comments:

1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or
similar instead of 1. This makes the code much more readable than
returning 0 or 1. Maybe you can find better names than my suggestions?

2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;"
instead of "up(input_lock); return -EINTR;". I think this is more
consistent.

3) in hid_device_probe() maybe check that hid_hw_start() doesn't
return "1" accidentally? The current code deadlocks if hid_hw_start()
would be changed to return 1 (which would be bad as is, but maybe be
rather safe than sorry?).

4) Maybe we can add a similar logic to hid_device_remove? That is,
hid_device_remove() shouldn't lock input_lock if the driver wants to
perform I/O during remove, too. Because this may cause the driver to
drop input events while the lock is held.
At least the hid-wiimote driver could make use of this feature during removal.

5) In current code, if a driver unlocks input_lock but still fails, it
needs to re-lock input_lock and then return the appropriate error
code. Is that the intended behavior?

6) Documentation! At least add some comments to
hid_driver_probe/remove so others can make use of this feature without
looking at other drivers.

If you are interested, I can send just that one commit to the linux-input
list now, instead of waiting on WTP to be ready for posting to linux-input
(which may be a couple weeks).

I would prefer if you send an RFC patch to the list so we can all comment on it.

btw. how about two helpers:

static inline void hid_device_io_start(struct hid_device *d)
{
    up(&d->driver_input_lock);
}

static inline void hid_device_io_stop(struct hid_device *d)
{
    down(&d->driver_input_lock);
}

This would guarantee that drivers don't use down_interruptible()
(which would be wrong in error-paths in ->probe()) and it would be
more readable. The return-value could then be called something like
HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on
hid_device in io_start/io_stop so the return-value isn't needed at
all.

Thanks for the patch!
David