[PATCH] firmware: Update hotplug script, remove sysfs files

[PATCH] firmware: Update hotplug script, remove sysfs files

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

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 <damm@opensource.se>
---

 Documentation/firmware_class/hotplug-script |   17 +++++++++--------
 drivers/base/firmware_class.c               |    6 +++++-
 2 files changed, 14 insertions(+), 9 deletions(-)

--- 0001/Documentation/firmware_class/hotplug-script
+++ work/Documentation/firmware_class/hotplug-script	2010-06-25 14:46:17.000000000 +0900
@@ -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
--- 0001/drivers/base/firmware_class.c
+++ work/drivers/base/firmware_class.c	2010-06-25 14:48:53.000000000 +0900
@@ -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;
 

Re: [PATCH] firmware: Update hotplug script, remove sysfs files

[Greg KH]

On Fri, Jun 25, 2010 at 05:55:11PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> 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.

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

Re: [PATCH] firmware: Update hotplug script, remove sysfs files

[Magnus Damm]

On Sat, Jun 26, 2010 at 3:22 AM, Greg KH <greg@kroah.com> wrote:
> On Fri, Jun 25, 2010 at 05:55:11PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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.
>
> 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

Re: [PATCH] firmware: Update hotplug script, remove sysfs files

[Dmitry Torokhov]

On Mon, Jun 28, 2010 at 03:17:22PM +0900, Magnus Damm wrote:
> On Sat, Jun 26, 2010 at 3:22 AM, Greg KH <greg@kroah.com> wrote:
> > On Fri, Jun 25, 2010 at 05:55:11PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >>
> >> 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.
> >
> > 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?
> 

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.

-- 
Dmitry