Hello Mr. Ludovic Rousseau, Am Donnerstag, den 31.07.2014, 15:51 +0200 schrieb Ludovic Rousseau: > 2014-07-07 14:22 GMT+02:00 Stefani Seibold <[email protected]>:
> > the current libudev hotplug code have some issues. > > > > The HPRegisterForHotplugEvents() does first a HPRescanUsbBus() and than > > start the HPEstablishUSBNotifications() thread. This could lead in a > > race condition, where a add event occurred between the scan and the > > start of the thread. I moved the scan after the > > udev_monitor_enable_receiving() call, so the race is fixed. > > Do you get this problem in a real use case or is it theoretical? > > If you do get the problem in real life what is your application use case? > It is true that the time window is very small, but i was faced with this kind of problem. Not with pcsc-lite, but with other hotplug code with did it in the same wrong way. But does this make a difference? A bug is a bug and should be fixed. > > Next: The HPRescanUsbBus() is broken too. It will set all > > readerTracker.status to READER_ABSENT. This make it very hard to track > > reader where added twice. This can happen due the fix above and due the > > fact that any udev event in the origin code could lead to a add, because > > the "remove" event whill call HPRescanUsbBus() which calls > > HPAddDevice(). This problem could be fixed using a temporary array of > > status bytes. > > It looks like the problem is that HPRescanUsbBus() is not reentrant by > using a global state. > But HPRescanUsbBus() is only called from HPEstablishUSBNotifications() > when a device is removed. The call to HPRescanUsbBus() is synchronous > and thus HPEstablishUSBNotifications() can't call HPRescanUsbBus() > again before the previous call has returned. > Sorry, but the whole hotplug is broken. You try to add devices when you get an remove and vice versa. > So unless you modify the code to try solve the 1st issue the use of > HPRescanUsbBus() should be correct and safe. > The first issue has noting to do with this problem. > > And at last the HPAddDevice() is broken because a SCARD_E_UNKNOWN_READER > > status could never overwriten. But the "remove" event handling which > > also do device adds it could happen that a HPRescanUsbBus() will try to > > add a device via HPAddDevice() which is still in use by other processes > > or not ready. So a device should not permanently marked as > > SCARD_E_UNKNOWN_READER because when the udev "add" event arrives than > > the device should be normaly accessible. > > I do not follow you here. > What do you call "a device [..] which is still in use by other > processes or not ready." > > It is very simple, the hotplug_libudev code will receive events from and USB device, no matter if is a card reader or any other device. And when a event for a non CCID device you will call HPRescanUsbBus and try to add a CCID device when one is found. But at this time udevd will still working with the device. Udevd has something to do before the hotplug_libudev can access the device, f.e. create the device node, set the file mode bits, maybe execute a helper program and a least it will send a udev netlink messages "add". I was faced with the problem that udev was not ready with the whole process above and the device node was not ready during the call of HPAddDevice(). A clean udev hotplug application handles events seperatly: add events will only add of a device and remove events only remove of a device. The "add" code will also check for duplicate devices adds. This can happen when during the udev_monitor_enable_receiving() and the coldplug scan a device will be plugged. Then the coldplug scanner probably will find the device and a the hotplug add event will be received. > If the driver can't use the device then the device is marked > SCARD_E_UNKNOWN_READER. > If the device need more time so the driver can use it then the driver > should wait for the device to be ready before returning > IFD_NO_SUCH_DEVICE error code. > > A device has no second chance of being detected. It works or it does not. > You are right. But since the hotplug_libudev.c code is not race free... > > The following small patch fix all this issues. > > Thanks for your patch. > First I need to understand what the problems are so I can work on > unitary tests and verify the problems are really solved. > The patch is more a workaround. For a clean solution the code must be revamped. For a clean example of libudev usage have a look at http://www.signal11.us/oss/udev/ If you like i can do this for you. > It looks like you are working with virtual machines and moving USB > devices from one virtual machine to another. Is that the case? > No, i did not work with virtual machines. This happens on a real PowerPC embedded device. The boot up of this device is very optimized and fast, despite the is very slow CPU (400 MHz). I got all the problems and since i fixed it, i was never faced with the problem that a USB CCID Reader was not found during the boot phase. - Stefani _______________________________________________ Pcsclite-muscle mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
