[patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay Salunke]

This is a patch with modifications in firmware_class.c to have no hotplug 
support. 
The current code invokes hotplug script and cannot be controlled manually. 
The function request_firmware_nohotplug() is a new addition without 
breaking the existing code all the drivers dependent on firmware_class.c
should see no difference.

This fix is required in firmware_class.c to make dell_rbu driver work with 
firmware subsystem without having to do any hotplug stuff as per Greg's 
suggestion.

The nohotplug function makes the copying of the firmware image a manual 
process. Still the steps taken by the hotplug script will now need to be 
done manually. The user will need start the copy by first writign 1 to 
/sys/class/firmware/xxx/loading and then copy data to 
/sys/class/firmware/xxx/data. Finally the copy process ends when the user 
copies 0 or -1 to /sys/class/firmware/xxx/loading. 


By making a contribution to this project, I certify that:
The contribution was created in whole or in part by me and I have the
right to submit it under the open source license indicated in the file.
Resubmitting after cleaning up spaces/tabs etc...

Signed-off-by: Abhay Salunke <Abhay_Salunke@dell.com>

Thanks,
Abhay Salunke
Software Engineer.
DELL Inc

diff -uprN /usr/src/linux-2.6.11.11.ORIG/drivers/base/firmware_class.c /usr/src/linux-2.6.11.11/drivers/base/firmware_class.c
--- /usr/src/linux-2.6.11.11.ORIG/drivers/base/firmware_class.c	2005-06-08 09:49:57.874162352 -0500
+++ /usr/src/linux-2.6.11.11/drivers/base/firmware_class.c	2005-06-08 09:51:43.051173016 -0500
@@ -87,7 +87,7 @@ static struct class firmware_class = {
 	.name		= "firmware",
 	.hotplug	= firmware_class_hotplug,
 	.release	= fw_class_dev_release,
-};
+};  
 
 int
 firmware_class_hotplug(struct class_device *class_dev, char **envp,
@@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
 		printk(KERN_ERR "%s: class_device_create_file failed\n",
 		       __FUNCTION__);
 		goto error_unreg;
+r
 	}
 
 	set_bit(FW_STATUS_READY, &fw_priv->status);
@@ -376,6 +377,65 @@ out:
 	return retval;
 }
 
+static int
+_request_firmware(const struct firmware **firmware_p, const char *name,
+                 struct device *device, int fw_hotplug)
+{
+        struct class_device *class_dev;
+        struct firmware_priv *fw_priv;
+        struct firmware *firmware;
+        int retval;
+
+        if (!firmware_p)
+               return -EINVAL;
+
+        *firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+        if (!firmware) {
+                printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+                       __FUNCTION__);
+                retval = -ENOMEM;
+                goto out;
+        }
+        memset(firmware, 0, sizeof (*firmware));
+
+        retval = fw_setup_class_device(firmware, &class_dev, name, device);
+        if (retval)
+                goto error_kfree_fw;
+
+        fw_priv = class_get_devdata(class_dev);
+
+	if ( fw_hotplug == FW_DO_HOTPLUG) {
+	        if (loading_timeout) {
+        	        fw_priv->timeout.expires = 
+				jiffies + loading_timeout * HZ;
+                	add_timer(&fw_priv->timeout);
+        	}
+	       	kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
+	}
+
+        wait_for_completion(&fw_priv->completion);
+        set_bit(FW_STATUS_DONE, &fw_priv->status);
+
+        del_timer_sync(&fw_priv->timeout);
+
+        down(&fw_lock);
+        if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
+                retval = -ENOENT;
+                release_firmware(fw_priv->fw);
+                *firmware_p = NULL;
+        }
+        fw_priv->fw = NULL;
+        up(&fw_lock);
+        class_device_unregister(class_dev);
+        goto out;
+
+error_kfree_fw:
+        kfree(firmware);
+        *firmware_p = NULL;
+out:
+        return retval;
+}
+
 /**
  * request_firmware: - request firmware to hotplug and wait for it
  * Description:
@@ -392,56 +452,7 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
 		 struct device *device)
 {
-	struct class_device *class_dev;
-	struct firmware_priv *fw_priv;
-	struct firmware *firmware;
-	int retval;
-
-	if (!firmware_p)
-		return -EINVAL;
-
-	*firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
-	if (!firmware) {
-		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
-		       __FUNCTION__);
-		retval = -ENOMEM;
-		goto out;
-	}
-	memset(firmware, 0, sizeof (*firmware));
-
-	retval = fw_setup_class_device(firmware, &class_dev, name, device);
-	if (retval)
-		goto error_kfree_fw;
-
-	fw_priv = class_get_devdata(class_dev);
-
-	if (loading_timeout) {
-		fw_priv->timeout.expires = jiffies + loading_timeout * HZ;
-		add_timer(&fw_priv->timeout);
-	}
-
-	kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
-	wait_for_completion(&fw_priv->completion);
-	set_bit(FW_STATUS_DONE, &fw_priv->status);
-
-	del_timer_sync(&fw_priv->timeout);
-
-	down(&fw_lock);
-	if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
-		retval = -ENOENT;
-		release_firmware(fw_priv->fw);
-		*firmware_p = NULL;
-	}
-	fw_priv->fw = NULL;
-	up(&fw_lock);
-	class_device_unregister(class_dev);
-	goto out;
-
-error_kfree_fw:
-	kfree(firmware);
-	*firmware_p = NULL;
-out:
-	return retval;
+	return _request_firmware(firmware_p, name, device, FW_DO_HOTPLUG);
 }
 
 /**
@@ -472,14 +483,15 @@ register_firmware(const char *name, cons
 	 * decide if firmware caching is reasonable just leave it as a
 	 * noop */
 }
-
+ 
 /* Async support */
 struct firmware_work {
 	struct work_struct work;
-	struct module *module;
+ 	struct module *module;
 	const char *name;
 	struct device *device;
 	void *context;
+	int hotplug;
 	void (*cont)(const struct firmware *fw, void *context);
 };
 
@@ -493,7 +505,7 @@ request_firmware_work_func(void *arg)
 		return 0;
 	}
 	daemonize("%s/%s", "firmware", fw_work->name);
-	request_firmware(&fw, fw_work->name, fw_work->device);
+	_request_firmware(&fw, fw_work->name, fw_work->device, fw_work->hotplug);
 	fw_work->cont(fw, fw_work->context);
 	release_firmware(fw);
 	module_put(fw_work->module);
@@ -501,23 +513,9 @@ request_firmware_work_func(void *arg)
 	return 0;
 }
 
-/**
- * request_firmware_nowait:
- *
- * Description:
- *	Asynchronous variant of request_firmware() for contexts where
- *	it is not possible to sleep.
- *
- *	@cont will be called asynchronously when the firmware request is over.
- *
- *	@context will be passed over to @cont.
- *
- *	@fw may be %NULL if firmware request fails.
- *
- **/
-int
-request_firmware_nowait(
-	struct module *module,
+static int
+_request_firmware_nowait(
+	struct module *module, int fw_hotplug,
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -528,7 +526,7 @@ request_firmware_nowait(
 	if (!fw_work)
 		return -ENOMEM;
 	if (!try_module_get(module)) {
-		kfree(fw_work);
+ 		kfree(fw_work);
 		return -EFAULT;
 	}
 
@@ -538,6 +536,7 @@ request_firmware_nowait(
 		.device = device,
 		.context = context,
 		.cont = cont,
+		.hotplug = fw_hotplug,
 	};
 
 	ret = kernel_thread(request_firmware_work_func, fw_work,
@@ -550,6 +549,41 @@ request_firmware_nowait(
 	return 0;
 }
 
+/**
+ * request_firmware_nowait:
+ *
+ * Description:
+ *	Asynchronous variant of request_firmware() for contexts where
+ *	it is not possible to sleep.
+ *
+ *	@cont will be called asynchronously when the firmware request is over.
+ *
+ *	@context will be passed over to @cont.
+ *
+ *	@fw may be %NULL if firmware request fails.
+ *
+ **/
+int
+request_firmware_nowait(
+	struct module *module,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	return _request_firmware_nowait(module, FW_DO_HOTPLUG, name, 
+					device, context, cont);
+}
+
+int
+request_firmware_nohotplug(
+	struct module *module,
+        const char *name, struct device *device,void *context,
+        void (*cont)(const struct firmware *fw, void *context))
+{
+	return _request_firmware_nowait(module, FW_NO_HOTPLUG_NO_TIMEOUT, 
+					name, device, context, cont);
+}
+
+
 static int __init
 firmware_class_init(void)
 {
@@ -568,6 +602,7 @@ firmware_class_init(void)
 	return error;
 
 }
+r
 static void __exit
 firmware_class_exit(void)
 {
@@ -581,3 +616,5 @@ EXPORT_SYMBOL(release_firmware);
 EXPORT_SYMBOL(request_firmware);
 EXPORT_SYMBOL(request_firmware_nowait);
 EXPORT_SYMBOL(register_firmware);
+EXPORT_SYMBOL(request_firmware_nohotplug);
+
diff -uprN /usr/src/linux-2.6.11.11.ORIG/include/linux/firmware.h /usr/src/linux-2.6.11.11/include/linux/firmware.h
--- /usr/src/linux-2.6.11.11.ORIG/include/linux/firmware.h	2005-06-08 09:49:57.711187128 -0500
+++ /usr/src/linux-2.6.11.11/include/linux/firmware.h	2005-06-08 09:50:41.714497608 -0500
@@ -3,6 +3,8 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #define FIRMWARE_NAME_MAX 30 
+#define FW_DO_HOTPLUG (0)
+#define FW_NO_HOTPLUG_NO_TIMEOUT (1)
 struct firmware {
 	size_t size;
 	u8 *data;
@@ -15,6 +17,11 @@ int request_firmware_nowait(
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 
+int request_firmware_nohotplug(
+	struct module *module,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context));
+
 void release_firmware(const struct firmware *fw);
 void register_firmware(const char *name, const u8 *data, size_t size);
 #endif

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Dmitry Torokhov]

On 6/8/05, Abhay Salunke <Abhay_Salunke@dell.com> wrote:
> This is a patch with modifications in firmware_class.c to have no hotplug
> support.
...

> @@ -87,7 +87,7 @@ static struct class firmware_class = {
>        .name           = "firmware",
>        .hotplug        = firmware_class_hotplug,
>        .release        = fw_class_dev_release,
> -};
> +};

Adds trailing whitespace.
> @@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
>                printk(KERN_ERR "%s: class_device_create_file failed\n",
>                       __FUNCTION__);
>                goto error_unreg;
> +r

What is this?
> -out:
> -       return retval;
> +       return _request_firmware(firmware_p, name, device, FW_DO_HOTPLUG);
>  }

Tab vs. space identation.

>  /* Async support */
>  struct firmware_work {
>        struct work_struct work;
> -       struct module *module;
> +       struct module *module;
>        const char *name;
>        struct device *device;
>        void *context;
> +       int hotplug;
>        void (*cont)(const struct firmware *fw, void *context);
>  };

I think it would be better if you just have request_firmware and
request_firmware_nowait accept timeout parameter that would override
default timeout in firmware_class. 0 would mean use default,
MAX_SCHED_TIMEOUT - wait indefinitely.

-- 
Dmitry

[patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay Salunke]

>> @@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
>>  		printk(KERN_ERR "%s: class_device_create_file failed\n",
>>  		       __FUNCTION__);
>>  		goto error_unreg;
>> +r
>>  	}

> Broken?

Oh! yes, fixed in the following patch 

By making a contribution to this project, I certify that:
The contribution was created in whole or in part by me and I have the
right to submit it under the open source license indicated in the file.
Resubmitting after cleaning up spaces/tabs etc...

Signed-off-by: Abhay Salunke <Abhay_Salunke@dell.com>

Thanks,
Abhay Salunke
Software Engineer.
DELL Inc

diff -uprN /usr/src/linux-2.6.11.11.ORIG/drivers/base/firmware_class.c /usr/src/linux-2.6.11.11/drivers/base/firmware_class.c
--- /usr/src/linux-2.6.11.11.ORIG/drivers/base/firmware_class.c	2005-06-08 09:49:57.874162352 -0500
+++ /usr/src/linux-2.6.11.11/drivers/base/firmware_class.c	2005-06-08 10:32:35.581331760 -0500
@@ -87,7 +87,7 @@ static struct class firmware_class = {
 	.name		= "firmware",
 	.hotplug	= firmware_class_hotplug,
 	.release	= fw_class_dev_release,
-};
+};  
 
 int
 firmware_class_hotplug(struct class_device *class_dev, char **envp,
@@ -376,6 +376,65 @@ out:
 	return retval;
 }
 
+static int
+_request_firmware(const struct firmware **firmware_p, const char *name,
+                 struct device *device, int fw_hotplug)
+{
+        struct class_device *class_dev;
+        struct firmware_priv *fw_priv;
+        struct firmware *firmware;
+        int retval;
+
+        if (!firmware_p)
+               return -EINVAL;
+
+        *firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+        if (!firmware) {
+                printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+                       __FUNCTION__);
+                retval = -ENOMEM;
+                goto out;
+        }
+        memset(firmware, 0, sizeof (*firmware));
+
+        retval = fw_setup_class_device(firmware, &class_dev, name, device);
+        if (retval)
+                goto error_kfree_fw;
+
+        fw_priv = class_get_devdata(class_dev);
+
+	if ( fw_hotplug == FW_DO_HOTPLUG) {
+	        if (loading_timeout) {
+        	        fw_priv->timeout.expires = 
+				jiffies + loading_timeout * HZ;
+                	add_timer(&fw_priv->timeout);
+        	}
+	       	kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
+	}
+
+        wait_for_completion(&fw_priv->completion);
+        set_bit(FW_STATUS_DONE, &fw_priv->status);
+
+        del_timer_sync(&fw_priv->timeout);
+
+        down(&fw_lock);
+        if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
+                retval = -ENOENT;
+                release_firmware(fw_priv->fw);
+                *firmware_p = NULL;
+        }
+        fw_priv->fw = NULL;
+        up(&fw_lock);
+        class_device_unregister(class_dev);
+        goto out;
+
+error_kfree_fw:
+        kfree(firmware);
+        *firmware_p = NULL;
+out:
+        return retval;
+}
+
 /**
  * request_firmware: - request firmware to hotplug and wait for it
  * Description:
@@ -392,56 +451,7 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
 		 struct device *device)
 {
-	struct class_device *class_dev;
-	struct firmware_priv *fw_priv;
-	struct firmware *firmware;
-	int retval;
-
-	if (!firmware_p)
-		return -EINVAL;
-
-	*firmware_p = firmware = kmalloc(sizeof (struct firmware), GFP_KERNEL);
-	if (!firmware) {
-		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
-		       __FUNCTION__);
-		retval = -ENOMEM;
-		goto out;
-	}
-	memset(firmware, 0, sizeof (*firmware));
-
-	retval = fw_setup_class_device(firmware, &class_dev, name, device);
-	if (retval)
-		goto error_kfree_fw;
-
-	fw_priv = class_get_devdata(class_dev);
-
-	if (loading_timeout) {
-		fw_priv->timeout.expires = jiffies + loading_timeout * HZ;
-		add_timer(&fw_priv->timeout);
-	}
-
-	kobject_hotplug(&class_dev->kobj, KOBJ_ADD);
-	wait_for_completion(&fw_priv->completion);
-	set_bit(FW_STATUS_DONE, &fw_priv->status);
-
-	del_timer_sync(&fw_priv->timeout);
-
-	down(&fw_lock);
-	if (!fw_priv->fw->size || test_bit(FW_STATUS_ABORT, &fw_priv->status)) {
-		retval = -ENOENT;
-		release_firmware(fw_priv->fw);
-		*firmware_p = NULL;
-	}
-	fw_priv->fw = NULL;
-	up(&fw_lock);
-	class_device_unregister(class_dev);
-	goto out;
-
-error_kfree_fw:
-	kfree(firmware);
-	*firmware_p = NULL;
-out:
-	return retval;
+	return _request_firmware(firmware_p, name, device, FW_DO_HOTPLUG);
 }
 
 /**
@@ -472,14 +482,15 @@ register_firmware(const char *name, cons
 	 * decide if firmware caching is reasonable just leave it as a
 	 * noop */
 }
-
+ 
 /* Async support */
 struct firmware_work {
 	struct work_struct work;
-	struct module *module;
+ 	struct module *module;
 	const char *name;
 	struct device *device;
 	void *context;
+	int hotplug;
 	void (*cont)(const struct firmware *fw, void *context);
 };
 
@@ -493,7 +504,7 @@ request_firmware_work_func(void *arg)
 		return 0;
 	}
 	daemonize("%s/%s", "firmware", fw_work->name);
-	request_firmware(&fw, fw_work->name, fw_work->device);
+	_request_firmware(&fw, fw_work->name, fw_work->device, fw_work->hotplug);
 	fw_work->cont(fw, fw_work->context);
 	release_firmware(fw);
 	module_put(fw_work->module);
@@ -501,23 +512,9 @@ request_firmware_work_func(void *arg)
 	return 0;
 }
 
-/**
- * request_firmware_nowait:
- *
- * Description:
- *	Asynchronous variant of request_firmware() for contexts where
- *	it is not possible to sleep.
- *
- *	@cont will be called asynchronously when the firmware request is over.
- *
- *	@context will be passed over to @cont.
- *
- *	@fw may be %NULL if firmware request fails.
- *
- **/
-int
-request_firmware_nowait(
-	struct module *module,
+static int
+_request_firmware_nowait(
+	struct module *module, int fw_hotplug,
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -528,7 +525,7 @@ request_firmware_nowait(
 	if (!fw_work)
 		return -ENOMEM;
 	if (!try_module_get(module)) {
-		kfree(fw_work);
+ 		kfree(fw_work);
 		return -EFAULT;
 	}
 
@@ -538,6 +535,7 @@ request_firmware_nowait(
 		.device = device,
 		.context = context,
 		.cont = cont,
+		.hotplug = fw_hotplug,
 	};
 
 	ret = kernel_thread(request_firmware_work_func, fw_work,
@@ -550,6 +548,41 @@ request_firmware_nowait(
 	return 0;
 }
 
+/**
+ * request_firmware_nowait:
+ *
+ * Description:
+ *	Asynchronous variant of request_firmware() for contexts where
+ *	it is not possible to sleep.
+ *
+ *	@cont will be called asynchronously when the firmware request is over.
+ *
+ *	@context will be passed over to @cont.
+ *
+ *	@fw may be %NULL if firmware request fails.
+ *
+ **/
+int
+request_firmware_nowait(
+	struct module *module,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	return _request_firmware_nowait(module, FW_DO_HOTPLUG, name, 
+					device, context, cont);
+}
+
+int
+request_firmware_nohotplug(
+	struct module *module,
+        const char *name, struct device *device,void *context,
+        void (*cont)(const struct firmware *fw, void *context))
+{
+	return _request_firmware_nowait(module, FW_NO_HOTPLUG_NO_TIMEOUT, 
+					name, device, context, cont);
+}
+
+
 static int __init
 firmware_class_init(void)
 {
@@ -568,6 +601,7 @@ firmware_class_init(void)
 	return error;
 
 }
+
 static void __exit
 firmware_class_exit(void)
 {
@@ -581,3 +615,5 @@ EXPORT_SYMBOL(release_firmware);
 EXPORT_SYMBOL(request_firmware);
 EXPORT_SYMBOL(request_firmware_nowait);
 EXPORT_SYMBOL(register_firmware);
+EXPORT_SYMBOL(request_firmware_nohotplug);
+
diff -uprN /usr/src/linux-2.6.11.11.ORIG/include/linux/firmware.h /usr/src/linux-2.6.11.11/include/linux/firmware.h
--- /usr/src/linux-2.6.11.11.ORIG/include/linux/firmware.h	2005-06-08 09:49:57.711187128 -0500
+++ /usr/src/linux-2.6.11.11/include/linux/firmware.h	2005-06-08 09:50:41.714497608 -0500
@@ -3,6 +3,8 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #define FIRMWARE_NAME_MAX 30 
+#define FW_DO_HOTPLUG (0)
+#define FW_NO_HOTPLUG_NO_TIMEOUT (1)
 struct firmware {
 	size_t size;
 	u8 *data;
@@ -15,6 +17,11 @@ int request_firmware_nowait(
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 
+int request_firmware_nohotplug(
+	struct module *module,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context));
+
 void release_firmware(const struct firmware *fw);
 void register_firmware(const char *name, const u8 *data, size_t size);
 #endif

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Greg KH]

On Wed, Jun 08, 2005 at 10:56:19AM -0500, Dmitry Torokhov wrote:
> On 6/8/05, Abhay Salunke <Abhay_Salunke@dell.com> wrote:
> > @@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
> >                printk(KERN_ERR "%s: class_device_create_file failed\n",
> >                       __FUNCTION__);
> >                goto error_unreg;
> > +r
> 
> What is this?

Proof he didn't test the code :(

> I think it would be better if you just have request_firmware and
> request_firmware_nowait accept timeout parameter that would override
> default timeout in firmware_class. 0 would mean use default,
> MAX_SCHED_TIMEOUT - wait indefinitely.

Yes and no.  Yes in that we should have a timeout value.  No in that 0
should be "forever" and we #define the current 10 second value.

thanks,

greg k-h

RE: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay_Salunke]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Wednesday, June 08, 2005 10:56 AM
> To: Salunke, Abhay
> Cc: linux-kernel@vger.kernel.org; Andrew Morton; Salunke, Abhay;
Domsch,
> Matt; Greg KH; Manuel Estrada Sainz
> Subject: Re: [patch 2.6.12-rc3] modifications in firmware_class.c to
> support nohotplug
> 
> On 6/8/05, Abhay Salunke <Abhay_Salunke@dell.com> wrote:
> > This is a patch with modifications in firmware_class.c to have no
> hotplug
> > support.
> ...
> 
> > @@ -87,7 +87,7 @@ static struct class firmware_class = {
> >        .name           = "firmware",
> >        .hotplug        = firmware_class_hotplug,
> >        .release        = fw_class_dev_release,
> > -};
> > +};
> 
> Adds trailing whitespace.
> > @@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
> >                printk(KERN_ERR "%s: class_device_create_file
failed\n",
> >                       __FUNCTION__);
> >                goto error_unreg;
> > +r
> 
> What is this?
> > -out:
> > -       return retval;
> > +       return _request_firmware(firmware_p, name, device,
> FW_DO_HOTPLUG);
> >  }
> 
> Tab vs. space identation.
> 
> >  /* Async support */
> >  struct firmware_work {
> >        struct work_struct work;
> > -       struct module *module;
> > +       struct module *module;
> >        const char *name;
> >        struct device *device;
> >        void *context;
> > +       int hotplug;
> >        void (*cont)(const struct firmware *fw, void *context);
> >  };
Will fix it all.
> 
> I think it would be better if you just have request_firmware and
> request_firmware_nowait accept timeout parameter that would override
> default timeout in firmware_class. 0 would mean use default,
> MAX_SCHED_TIMEOUT - wait indefinitely.

But we still need to avoid hotplug being invoked as we need it be a
manual process.

Thanks
Abhay

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Dmitry Torokhov]

On 6/8/05, Greg KH <greg@kroah.com> wrote:
> On Wed, Jun 08, 2005 at 10:56:19AM -0500, Dmitry Torokhov wrote:
> > On 6/8/05, Abhay Salunke <Abhay_Salunke@dell.com> wrote:
> > > @@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
> > >                printk(KERN_ERR "%s: class_device_create_file failed\n",
> > >                       __FUNCTION__);
> > >                goto error_unreg;
> > > +r
> >
> > What is this?
> 
> Proof he didn't test the code :(
> 
> > I think it would be better if you just have request_firmware and
> > request_firmware_nowait accept timeout parameter that would override
> > default timeout in firmware_class. 0 would mean use default,
> > MAX_SCHED_TIMEOUT - wait indefinitely.
> 
> Yes and no.  Yes in that we should have a timeout value.  No in that 0
> should be "forever" and we #define the current 10 second value.
> 

Are you saying that we should rip out of the firmware_class current
timeout attribute? I thought it was a nice to have system-wide defult
that can be adjusted by operator w/o need to recompile anything.

-- 
Dmitry

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Greg KH]

On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com wrote:
> > I think it would be better if you just have request_firmware and
> > request_firmware_nowait accept timeout parameter that would override
> > default timeout in firmware_class. 0 would mean use default,
> > MAX_SCHED_TIMEOUT - wait indefinitely.
> 
> But we still need to avoid hotplug being invoked as we need it be a
> manual process.

No, hotplug can happen just fine (it happens loads of times today for
things that people don't care about.)

thanks,

greg k-h

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Greg KH]

On Wed, Jun 08, 2005 at 11:09:53AM -0500, Dmitry Torokhov wrote:
> On 6/8/05, Greg KH <greg@kroah.com> wrote:
> > On Wed, Jun 08, 2005 at 10:56:19AM -0500, Dmitry Torokhov wrote:
> > > On 6/8/05, Abhay Salunke <Abhay_Salunke@dell.com> wrote:
> > > > @@ -364,6 +364,7 @@ fw_setup_class_device(struct firmware *f
> > > >                printk(KERN_ERR "%s: class_device_create_file failed\n",
> > > >                       __FUNCTION__);
> > > >                goto error_unreg;
> > > > +r
> > >
> > > What is this?
> > 
> > Proof he didn't test the code :(
> > 
> > > I think it would be better if you just have request_firmware and
> > > request_firmware_nowait accept timeout parameter that would override
> > > default timeout in firmware_class. 0 would mean use default,
> > > MAX_SCHED_TIMEOUT - wait indefinitely.
> > 
> > Yes and no.  Yes in that we should have a timeout value.  No in that 0
> > should be "forever" and we #define the current 10 second value.
> > 
> 
> Are you saying that we should rip out of the firmware_class current
> timeout attribute?

Change it from being a global to local to the firmware device?

> I thought it was a nice to have system-wide defult that can be
> adjusted by operator w/o need to recompile anything.

But (as recent udev bugs have proven) no one ever changes that default
value :(

In the end, I don't really care either way.  All I would like to see is
for there to be a timeout value for the function calls like you state
above.

thanks,

greg k-h

RE: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay_Salunke]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, June 08, 2005 11:10 AM
> To: Salunke, Abhay
> Cc: dtor_core@ameritech.net; linux-kernel@vger.kernel.org;
akpm@osdl.org;
> Domsch, Matt; ranty@debian.org
> Subject: Re: [patch 2.6.12-rc3] modifications in firmware_class.c to
> support nohotplug
> 
> On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com
wrote:
> > > I think it would be better if you just have request_firmware and
> > > request_firmware_nowait accept timeout parameter that would
override
> > > default timeout in firmware_class. 0 would mean use default,
> > > MAX_SCHED_TIMEOUT - wait indefinitely.
> >
> > But we still need to avoid hotplug being invoked as we need it be a
> > manual process.
> 
> No, hotplug can happen just fine (it happens loads of times today for
> things that people don't care about.)
> 
If hotplug happens the complete function is called which makes the
request_firmware return with a failure. 

Abhay

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Greg KH]

On Wed, Jun 08, 2005 at 11:23:30AM -0500, Abhay_Salunke@Dell.com wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Wednesday, June 08, 2005 11:10 AM
> > To: Salunke, Abhay
> > Cc: dtor_core@ameritech.net; linux-kernel@vger.kernel.org;
> akpm@osdl.org;
> > Domsch, Matt; ranty@debian.org
> > Subject: Re: [patch 2.6.12-rc3] modifications in firmware_class.c to
> > support nohotplug
> > 
> > On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com
> wrote:
> > > > I think it would be better if you just have request_firmware and
> > > > request_firmware_nowait accept timeout parameter that would
> override
> > > > default timeout in firmware_class. 0 would mean use default,
> > > > MAX_SCHED_TIMEOUT - wait indefinitely.
> > >
> > > But we still need to avoid hotplug being invoked as we need it be a
> > > manual process.
> > 
> > No, hotplug can happen just fine (it happens loads of times today for
> > things that people don't care about.)
> > 
> If hotplug happens the complete function is called which makes the
> request_firmware return with a failure. 

If this was true, then the current code would not work at all.

RE: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay_Salunke]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, June 08, 2005 11:27 AM
> To: Salunke, Abhay
> Cc: dtor_core@ameritech.net; linux-kernel@vger.kernel.org;
akpm@osdl.org;
> Domsch, Matt; ranty@debian.org
> Subject: Re: [patch 2.6.12-rc3] modifications in firmware_class.c to
> support nohotplug
> 
> On Wed, Jun 08, 2005 at 11:23:30AM -0500, Abhay_Salunke@Dell.com
wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Wednesday, June 08, 2005 11:10 AM
> > > To: Salunke, Abhay
> > > Cc: dtor_core@ameritech.net; linux-kernel@vger.kernel.org;
> > akpm@osdl.org;
> > > Domsch, Matt; ranty@debian.org
> > > Subject: Re: [patch 2.6.12-rc3] modifications in firmware_class.c
to
> > > support nohotplug
> > >
> > > On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com
> > wrote:
> > > > > I think it would be better if you just have request_firmware
and
> > > > > request_firmware_nowait accept timeout parameter that would
> > override
> > > > > default timeout in firmware_class. 0 would mean use default,
> > > > > MAX_SCHED_TIMEOUT - wait indefinitely.
> > > >
> > > > But we still need to avoid hotplug being invoked as we need it
be a
> > > > manual process.
> > >
> > > No, hotplug can happen just fine (it happens loads of times today
for
> > > things that people don't care about.)
> > >
> > If hotplug happens the complete function is called which makes the
> > request_firmware return with a failure.
> 
> If this was true, then the current code would not work at all.
Why not? Can't we avoid hotplug from running by not calling
kobject_hotplug()?
If hotplug does not happen we can do the stuff done by hotplug script
manually. And also the current code skips add_timer so not timeouts
irrespective of the timeout specified in /sys/class/firmware/timeout
thus preventing from breaking other's who are using timeout.

Thanks,
Abhay

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Dmitry Torokhov]

On 6/8/05, Greg KH <greg@kroah.com> wrote:
> On Wed, Jun 08, 2005 at 11:23:30AM -0500, Abhay_Salunke@Dell.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Wednesday, June 08, 2005 11:10 AM
> > > To: Salunke, Abhay
> > > Cc: dtor_core@ameritech.net; linux-kernel@vger.kernel.org;
> > akpm@osdl.org;
> > > Domsch, Matt; ranty@debian.org
> > > Subject: Re: [patch 2.6.12-rc3] modifications in firmware_class.c to
> > > support nohotplug
> > >
> > > On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com
> > wrote:
> > > > > I think it would be better if you just have request_firmware and
> > > > > request_firmware_nowait accept timeout parameter that would
> > override
> > > > > default timeout in firmware_class. 0 would mean use default,
> > > > > MAX_SCHED_TIMEOUT - wait indefinitely.
> > > >
> > > > But we still need to avoid hotplug being invoked as we need it be a
> > > > manual process.
> > >
> > > No, hotplug can happen just fine (it happens loads of times today for
> > > things that people don't care about.)
> > >
> > If hotplug happens the complete function is called which makes the
> > request_firmware return with a failure.
> 
> If this was true, then the current code would not work at all.
> 

What Abhay is trying to say is that default firmware.agent when it
does not find requested firmware file writes -1 (abort) to "loading"
attribute causing firmware_request to fail.

I think it should be fixed in firmware.agent though, not in kernel -
the agent shoudl just recognoze that sometimes not having firmware
file is ok.

-- 
Dmitry

RE: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay_Salunke]

> > > > On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com
> > > wrote:
> > > > > > I think it would be better if you just have request_firmware
and
> > > > > > request_firmware_nowait accept timeout parameter that would
> > > override
> > > > > > default timeout in firmware_class. 0 would mean use default,
> > > > > > MAX_SCHED_TIMEOUT - wait indefinitely.
> > > > >
> > > > > But we still need to avoid hotplug being invoked as we need it
be
> a
> > > > > manual process.
> > > >
> > > > No, hotplug can happen just fine (it happens loads of times
today
> for
> > > > things that people don't care about.)
> > > >
> > > If hotplug happens the complete function is called which makes the
> > > request_firmware return with a failure.
> >
> > If this was true, then the current code would not work at all.
> >
> 
> What Abhay is trying to say is that default firmware.agent when it
> does not find requested firmware file writes -1 (abort) to "loading"
> attribute causing firmware_request to fail.
> 
> I think it should be fixed in firmware.agent though, not in kernel -
> the agent shoudl just recognoze that sometimes not having firmware
> file is ok.
> 
Greg, any inputs on whether we change firmware.agent or we patch
firmware_class.c?

Thanks
Abhay

RE: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Abhay_Salunke]

> > > > -----Original Message-----
> > > > From: Greg KH [mailto:greg@kroah.com]
> > > > Sent: Wednesday, June 08, 2005 11:10 AM
> > > > To: Salunke, Abhay
> > > > Cc: dtor_core@ameritech.net; linux-kernel@vger.kernel.org;
> > > akpm@osdl.org;
> > > > Domsch, Matt; ranty@debian.org
> > > > Subject: Re: [patch 2.6.12-rc3] modifications in
firmware_class.c to
> > > > support nohotplug
> > > >
> > > > On Wed, Jun 08, 2005 at 11:04:09AM -0500, Abhay_Salunke@Dell.com
> > > wrote:
> > > > > > I think it would be better if you just have request_firmware
and
> > > > > > request_firmware_nowait accept timeout parameter that would
> > > override
> > > > > > default timeout in firmware_class. 0 would mean use default,
> > > > > > MAX_SCHED_TIMEOUT - wait indefinitely.
> > > > >
> > > > > But we still need to avoid hotplug being invoked as we need it
be
> a
> > > > > manual process.
> > > >
> > > > No, hotplug can happen just fine (it happens loads of times
today
> for
> > > > things that people don't care about.)
> > > >
> > > If hotplug happens the complete function is called which makes the
> > > request_firmware return with a failure.
> >
> > If this was true, then the current code would not work at all.
> >
> 
> What Abhay is trying to say is that default firmware.agent when it
> does not find requested firmware file writes -1 (abort) to "loading"
> attribute causing firmware_request to fail.
> 
> I think it should be fixed in firmware.agent though, not in kernel -
> the agent shoudl just recognoze that sometimes not having firmware
> file is ok.
> 
I tired to do the following 
1. echo 0 > /sys/class/firmware/timeout
2. modify the firmware.agent by commenting echo -1 when file is not
present as below.

    if [ -f "$FIRMWARE_DIR/$FIRMWARE" ]; then
        echo 1 > $SYSFS/$DEVPATH/loading
        cp "$FIRMWARE_DIR/$FIRMWARE" $SYSFS/$DEVPATH/data
        echo 0 > $SYSFS/$DEVPATH/loading
    else
   #     echo -1 > $SYSFS/$DEVPATH/loading
    Fi
3. load the dell_rbu driver : see dell_rbu code snipped below

device_initialize(&rbu_device);

strncpy(rbu_device.bus_id,"dell_rbu", BUS_ID_SIZE);

rc = request_firmware_nowait(THIS_MODULE,
		"dell_rbu", &rbu_device,
		&context,
		callbackfn);
	if(rc) {
		printk(KERN_ERR 
			"dcdrbu_init:"
			" request_firmware_nowait failed %d\n", rc);
	}

But this created a kernel Oops message as follows. Also note that
uncomment the line in firmware.agents fixed the Oops message.

Jun 10 19:29:39 localhost kernel: Unable to handle kernel NULL pointer
dereference at virtual address 00000000
Jun 10 19:29:39 localhost kernel:  printing eip:
Jun 10 19:29:39 localhost kernel: c0188b71
Jun 10 19:29:39 localhost kernel: *pde = 1fabe001
Jun 10 19:29:39 localhost kernel: Oops: 0000 [#2]
Jun 10 19:29:39 localhost kernel: SMP
Jun 10 19:29:39 localhost kernel: Modules linked in: dell_rbu(U) smbfs
parport_pc lp parport autofs4 sunrpc ipt_REJECT ipt_state ip_conntrack
iptable_filter ip_tables button battery ac md5 ipv6 ohci_hcd tg3 floppy
sg dm_snapshot dm_zero dm_mirror ext3 jbd dm_mod mptscsih mptbase sd_mod
scsi_mod
Jun 10 19:29:39 localhost kernel: CPU:    1
Jun 10 19:29:39 localhost kernel: EIP:    0060:[<c0188b71>]    Not
tainted VLI
Jun 10 19:29:39 localhost kernel: EFLAGS: 00010286   (2.6.9-5.ELsmp)
Jun 10 19:29:39 localhost kernel: EIP is at object_path_length+0x10/0x25
Jun 10 19:29:39 localhost kernel: eax: 00000000   ebx: 00000001   ecx:
ffffffff   edx: e0a8fb24
Jun 10 19:29:39 localhost kernel: esi: de3c089c   edi: 00000000   ebp:
dde2b000   esp: d7a3fe4c
Jun 10 19:29:40 localhost kernel: ds: 007b   es: 007b   ss: 0068
Jun 10 19:29:40 localhost kernel: Process bash (pid: 2825,
threadinfo=d7a3f000 task=d739e630)
Jun 10 19:29:40 localhost kernel: Stack: 00000003 e0a8fb24 c0188cf8
e0a8fb24 c031fdc0 de3c089c e0a8fb24 de1ff188
Jun 10 19:29:40 localhost kernel:        c0188dfa dde2b000 dde2b000
fffffff4 de3c089c d7a3ff0c c0188e47 d7a3f000
Jun 10 19:29:40 localhost kernel:        00000000 de3c089c d7a3ff0c
c0161400 dfe84b00 de3c089c 00000000 dfe84b00
Jun 10 19:29:40 localhost kernel: Call Trace:
Jun 10 19:29:40 localhost kernel:  [<c0188cf8>]
sysfs_get_target_path+0x19/0x59
Jun 10 19:29:40 localhost kernel:  [<c0188dfa>] sysfs_getlink+0xc2/0xe9
Jun 10 19:29:40 localhost kernel:  [<c0188e47>]
sysfs_follow_link+0x26/0x3e
Jun 10 19:29:40 localhost kernel:  [<c0161400>]
link_path_walk+0x8fa/0xba9
Jun 10 19:29:40 localhost kernel:  [<c01619c2>] path_lookup+0x144/0x174
Jun 10 19:29:40 localhost kernel:  [<c0161b06>] __user_walk+0x21/0x51
Jun 10 19:29:40 localhost kernel:  [<c015d0ad>] vfs_stat+0x14/0x3a
Jun 10 19:29:40 localhost kernel:  [<c015d6b6>] sys_stat64+0xf/0x23
Jun 10 19:29:40 localhost kernel:  [<c02c62a3>] syscall_call+0x7/0xb
Jun 10 19:29:40 localhost kernel: Code: fe ff ff e8 82 b1 13 00 e9 3a ff
ff ff 90 31 d2 8b 40 24 42 85 c0 75 f8 89 d0 c3 57 89 c2 53 bb 01 00 00
00 8b 3a 31 c0 83 c9 ff <f2> ae f7 d1 49 8b 52 24 8d 5c 0b 01 85 d2 75
e9 89 d8 5b 5f c3

I guess we need to modify the firmware_class.c code to fix it.
Please let me know your opinions.

Thanks
Abhay

Re: [patch 2.6.12-rc3] modifications in firmware_class.c to support nohotplug

[Dmitry Torokhov]

On 6/10/05, Abhay_Salunke@dell.com <Abhay_Salunke@dell.com> wrote:
> I tired to do the following
> 1. echo 0 > /sys/class/firmware/timeout
> 2. modify the firmware.agent by commenting echo -1 when file is not
> present as below.
> 
>    if [ -f "$FIRMWARE_DIR/$FIRMWARE" ]; then
>        echo 1 > $SYSFS/$DEVPATH/loading
>        cp "$FIRMWARE_DIR/$FIRMWARE" $SYSFS/$DEVPATH/data
>        echo 0 > $SYSFS/$DEVPATH/loading
>    else
>   #     echo -1 > $SYSFS/$DEVPATH/loading
>    Fi
> 3. load the dell_rbu driver : see dell_rbu code snipped below
> 
> device_initialize(&rbu_device);
> 
> strncpy(rbu_device.bus_id,"dell_rbu", BUS_ID_SIZE);
> 

I think if you add:

        kobject_set_name(&rbu_device.kobj, "%s", rbu_device.bus_id,"dell_rbu");

it will work much better. Also, are you initializing rbu_device with
all 0 to begin with?

-- 
Dmitry