Skip to contentSkip to conversation

firmware: Update hotplug script, remove sysfs files

PATCH In review Magnus Damm 3 participants 4 messages Jun 25, 2010 – Jun 28, 2010

Subsystems (3)
Documentation Driver core Firmware loader (request_firmware)
lore
Share & tools
lore

Magnus Damm

author
#0 lore raw

PATCH 1/1 firmware: Update hotplug script, remove sysfs files

From: Magnus Damm <[email protected]>

Update the in-kernel hotplug example script to work properly with recent kernels. Without this fix the script may load the firmware twice - both at "add" and "remove" time.

The second load only triggers in the case when multiple firmware images are used. A good example is the b43 driver which does not work properly without this fix.

While at it, make sure sysfs files are removed.

Signed-off-by Magnus Damm <[email protected]>

---

 Documentation/firmware_class/hotplug-script |   17 +++++++++--------
 drivers/base/firmware_class.c               |    6 +++++-
 2 files changed, 14 insertions(+), 9 deletions(-)
Documentation/firmware_class/hotplug-script +9 -8
@@ -6,11 +6,12 @@
 
 HOTPLUG_FW_DIR=/usr/lib/hotplug/firmware/
 
-echo 1 > /sys/$DEVPATH/loading
-cat $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data
-echo 0 > /sys/$DEVPATH/loading
-
-# To cancel the load in case of error:
-#
-#	echo -1 > /sys/$DEVPATH/loading
-#
+if [ "$SUBSYSTEM" == "firmware" -a "$ACTION" == "add" ]; then
+  if [ -f $HOTPLUG_FW_DIR/$FIRMWARE ]; then
+    echo 1 > /sys/$DEVPATH/loading
+    cat $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data
+    echo 0 > /sys/$DEVPATH/loading
+  else
+    echo -1 > /sys/$DEVPATH/loading
+  fi
+fi
drivers/base/firmware_class.c +5 -1
@@ -508,7 +508,7 @@ static int fw_setup_device(struct firmwa
 	retval = device_create_file(f_dev, &dev_attr_loading);
 	if (retval) {
 		dev_err(device, "%s: device_create_file failed\n", __func__);
-		goto error_unreg;
+		goto error_unreg2;
 	}
 
 	if (uevent)

@@ -516,6 +516,8 @@ static int fw_setup_device(struct firmwa
 	*dev_p = f_dev;
 	goto out;
 
+error_unreg2:
+	sysfs_remove_bin_file(&f_dev->kobj, &fw_priv->attr_data);
 error_unreg:
 	device_unregister(f_dev);
 out:

@@ -578,6 +580,8 @@ _request_firmware(const struct firmware 
 	}
 	fw_priv->fw = NULL;
 	mutex_unlock(&fw_lock);
+	sysfs_remove_bin_file(&f_dev->kobj, &fw_priv->attr_data);
+	device_remove_file(f_dev, &dev_attr_loading);
 	device_unregister(f_dev);
 	goto out;
#1 lore raw
Quoted 7 lines · Magnus Damm

From: Magnus Damm <[email protected]>

Update the in-kernel hotplug example script to work properly with recent kernels. Without this fix the script may load the firmware twice - both at "add" and "remove" time.

Quoted 5 lines

The second load only triggers in the case when multiple firmware images are used. A good example is the b43 driver which does not work properly without this fix.

While at it, make sure sysfs files are removed.

Hm, the documentation update applied, but this second part didn't. Care to redo it against the linux-next tree and resend it as a separate patch? I've split this up and applied the documentation part now, so you don't have to redo that.

thanks,

greg k-h

author
#2 lore raw
Quoted 12 lines · Greg KH
Quoted 11 lines · Magnus Damm

From: Magnus Damm <[email protected]>

Update the in-kernel hotplug example script to work properly with recent kernels. Without this fix the script may load the firmware twice - both at "add" and "remove" time.

The second load only triggers in the case when multiple firmware images are used. A good example is the b43 driver which does not work properly without this fix.

Quoted 6 lines
Quoted 1 line

While at it, make sure sysfs files are removed.

Hm, the documentation update applied, but this second part didn't.  Care to redo it against the linux-next tree and resend it as a separate patch?  I've split this up and applied the documentation part now, so you don't have to redo that.

Thanks a lot for splitting up the patch. I don't mind redoing the second part of the patch, but I'm not sure if it's actually needed to prevent memory leaks. This piece of code serves as an example:

+error_unreg2:
+ sysfs_remove_bin_file(&f_dev->kobj, &fw_priv->attr_data);
error_unreg:

        device_unregister(f_dev);

Perhaps device_unregister() call above already removes the kobj which makes the added sysfs_remove_bin_file() unnecessary?

Cheers,

/ magnus

#3 lore raw
Quoted 29 lines · Magnus Damm
Quoted 12 lines · Greg KH
Quoted 11 lines · Magnus Damm

From: Magnus Damm <[email protected]>

Update the in-kernel hotplug example script to work properly with recent kernels. Without this fix the script may load the firmware twice - both at "add" and "remove" time.

The second load only triggers in the case when multiple firmware images are used. A good example is the b43 driver which does not work properly without this fix.

Quoted 6 lines
Quoted 1 line

While at it, make sure sysfs files are removed.

Hm, the documentation update applied, but this second part didn't.  Care to redo it against the linux-next tree and resend it as a separate patch?  I've split this up and applied the documentation part now, so you don't have to redo that.

Thanks a lot for splitting up the patch. I don't mind redoing the second part of the patch, but I'm not sure if it's actually needed to prevent memory leaks. This piece of code serves as an example:

drivers/base/firmware_class.c +2 -0
+error_unreg2:
+       sysfs_remove_bin_file(&f_dev->kobj, &fw_priv->attr_data);
 error_unreg:
        device_unregister(f_dev);
Quoted 3 lines

Perhaps device_unregister() call above already removes the kobj which makes the added sysfs_remove_bin_file() unnecessary?

It is so happens that device_unregister() does clean up leftover sysfs attributes but we should not rely on that happen in the future. That said I glanced at linux-next and it appears that we do clean up the attributes properly now.

Thanks.

Signature · 1 line
Dmitry