Hi On Wed, May 15, 2013 at 4:58 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote: > 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 It was a bit naive to think my brain would just let this go.. So while I tried to sleep, my brain decided to continue working on this and despite it being a fairly hard to solve issue, I could come up with a race-free HIDP fix. With that prepared, we can actually make HID core allow I/O during device registration. During unregistration, the I/O layer will report ENODEV/EIO, anyway, so we don't have this hang in that case. I will try to move power_supply registration to a place where we can safely allow I/O in HID device registration. Then we should at least be good to go for HID devices. Cheers David -- 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
