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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
