USB Device Add reading wrong attributes on Linux

[Drew Moseley]

Kay Sievers wrote:
> On Wed, Dec 3, 2008 at 18:21, Drew Moseley <dmoseley at mvista.com> wrote:
>> I've noticed a case with a USB device on a linux host where some of the
>> attributes are not being properly initialized.  Specifically the
>> can_wake_up and is_self_powered fields are incorrect because when HAL
>> reads the bmAttributes sysfs file it gets a read error.  A restart of
>> hal usually brings the correct values in.
>>
>> In the kernel code for the USB sysfs files there is a comment that
>> "Unforunately these attributes cannot be created before the uevent is
>> broadcast".  I do see in the kernel where the sysfs file is being read
>> by HAL but getting an error.  Has anyone else dealt with this?  Should
>> there be a retry of some kind in HAL to handle this?
> 
> Yeah, that's a known issue. Things like this can not work without
> tight integration with the kernel, which must send an update event for
> every change. This is just not implemented, not in the kernel and not
> in HAL. Retry implemented in HAL would only help in some cases.
> 
> To make it kind of work, all these values could be updated when the
> child devices are created/removed. This would fix a bunch of different
> bugs in this area, like when a device is reconfigured, and HAL will
> not notice it, and exports the wrong values from the initial config.
> In most cases when child devices come and go, the parent device has,
> at that time, all values properly populated. So, per example, "usb
> interface" events could trigger an update of the "usb device" values.
> 
> Btw, It was a mistake to mirror any such kernel values in a daemon, we
> didn't know that the time HAL was created. It's really hard to get
> things like this right, and the pseudo "platform abstraction" at that
> fine-grained level just doesn't work in reality. DeviceKit, which will
> replace HAL, will not even try that. It will just use sysfs directly
> on Linux, without any such caching behavior like HAL has.
> 
> Thanks,
> Kay

Thanks for the description.  I think I will just add a retry to handle
the particular issue I am seeing.  The patch is below in case anyone
cares.

Regards,
Drew


diff --git a/hald/linux/device.c b/hald/linux/device.c
index d16ac3a..fb5439f 100644
--- a/hald/linux/device.c
+++ b/hald/linux/device.c
@@ -3584,7 +3584,7 @@ usb_add (const gchar *sysfs_path, const gchar *device_file, HalDevice *parent_de
 	/* only USB interfaces got a : in the bus_id */
 	bus_id = hal_util_get_last_element (sysfs_path);
 	if (strchr (bus_id, ':') == NULL) {
-		gint bmAttributes;
+		gint bmAttributes, retries;
 
 		hal_device_property_set_string (d, "info.subsystem", "usb_device");
 
@@ -3592,6 +3592,19 @@ usb_add (const gchar *sysfs_path, const gchar *device_file, HalDevice *parent_de
 
 		hal_device_property_set_string (d, "usb_device.linux.sysfs_path", sysfs_path);
 
+		/*
+		 * Delay until the attributes field is set
+		 */
+		for (retries = 0; retries < 3; retries++) {
+			int num_ms;
+			if (hal_util_get_int_from_file (sysfs_path, "bmAttributes", &bmAttributes, 16))
+				break;
+			num_ms = 10 * (1<<retries);
+			HAL_INFO (("spinning %d ms waiting for bmAttributes file for sysfs path %s",
+					   num_ms, sysfs_path));
+			usleep (1000 * num_ms);
+		}
+
 		hal_util_set_string_from_file(d, "usb_device.configuration", sysfs_path, "configuration");
 		hal_util_set_int_from_file (d, "usb_device.configuration_value", sysfs_path, "bConfigurationValue", 10);
 		hal_util_set_int_from_file (d, "usb_device.num_configurations", sysfs_path, "bNumConfigurations", 10);