Re: [PATCH] HID: input: return ENODATA if reading battery attrs fails

[David Herrmann]

Hi Anton

On Tue, May 14, 2013 at 1:20 AM, Anton Vorontsov <anton@xxxxxxxxxx> wrote:
> On Mon, May 13, 2013 at 05:01:30PM +0200, David Herrmann wrote:
> [..]
>> I really dislike the way power_supply core calls into the drivers during the
>> "add" uevent. If a driver holds an I/O mutex (or anything else), it might
>> even deadlock in a very non-obvious way. Is there a reason why we need to
>> pass _all_ battery properties along "add" and "remove" uevents? Isn't it
>> enough to pass them with "change" uevents? This would guarantee that the
>> power_supply callbacks are only called from user-context and "change" events.
>
> I don't think that there is a particular reason for that, but if you want
> to change that, then I'd suggest to still keep uevent reporting of all the
> properties on "add" and "remove" events, but don't actually call the
> drivers' callback, just assume ENODATA.

In case of ENODATA the property is entirely skipped and not included
in the uevent. Therefore, "assuming ENODATA" would be skipping all
properties.

> This way we well preserve the behaviour of the older kernels, so that
> userland will not break if, for example, it allocates needed memory on
> "add" event, and then assumes that "change" will follow the pattern.

I tried fixing this several ways, but there is one deadlock that we
cannot overcome. If we assume a driver holds a lock during
power_supply_unregister() that it also acquires in the get_property
callback, then we will always deadlock. Just think of one CPU
currently calling the power_supply_changed_work() callback while
another CPU currently holds the driver's lock and calls
power_supply_unregister(). power_supply_changed_work() will wait for
the lock, while power_supply_unregister() will wait synchronously for
the work to finish => deadlock

As a side-note, in *_uevent() callbacks we have no chance to see what
kind of event this is. We would have to fix lib/kobject_uevent.c to
provide this information.

So I am back to fixing drivers to allow I/O / safe callbacks while
calling power_supply_register()/unregister(). This might be easy for
HID-USB drivers to implement (there is hid_device_io_start/stop()),
but for Bluetooth HID devices, this will not work as there are
bluetooth locks that are still held. And fixing HIDP isn't enough, we
would actually have to go all the way down to fix Bluetooth HCI core
to provide more fine-grained locking (at least one per hci_conn). And
even then I am not sure how to allow I/O while loading/unloading
drivers on it.

So if anybody wants to step up and fix this mess, feel free to go. I
will give up here. I might try to extend Bluetooth HIDP to allow I/O
during setup, but I currently cannot figure out _any_ way how we can
allow this during destruction/unregistration.
Sorry.

Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/