request_firmware() hotplug interface, third round and a halve

request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

 Hi,

 There is some new stuff:
 -----------------------

 - 'struct device *device' is used instead of 'char *device'. So we now
   have the device symlink :)

 - request_firmware_nowait is implemented, please comment on it.
 
 - It doesn't abuse the stack any more, or at least not that much :)
 
 - There is a timeout, changeable from userspace. Feedback on a
   reasonable default value appreciated.
 
 - Extended 'loading' semantics:
 	echo 1 > loading:
		start a new load, and flush any data from a previous
		partial load.
	echo 0 > loading:
		finish load.
	echo -1 > loading:
		cancel load and give an error to the driver.
		
 - adapted to new sysfs interface.
 
 - it is now a patch against current bk-cvs gateway.
 
 - There is a *working* sample on how to use the firmware class without
   using request_firmware(). It is not clean, but it works, if it is
   found to be interesting it should be further polished.

 Some random notes:
 -----------------

 - request_firmware_cancel is not needed, request_firmware_nowait will
   lock the client module into memory until after triggering the
   callback.
   The only other problem that I can think of is the device getting
   unplugged while waiting for firmware, so the driver callback should
   first make sure that the device is still there.

 - firmware-class.diff adds both samples to the kernel tree, that
   doesn't mean that I want them there, it is just for easier testing.

 Patches:
 -------

 - firmware-class.diff:
 	firmware_class code and the two samples.

 - sysfs-bin-flexible-size.diff:
 	Make dynamically sized files possible.
	And return the right value on successful write.

 - sysfs-bin-lost-dget.diff:
 	I was having trouble when calling request_firmware() from a
	work queue, and after a little investigations it seams that this
	dget got lost along the way. Adding it back fixed the issue.
	Or am I causing a dentry leak now?

PS: If you guys consider this abusive CC:ing, please complain.
-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: firmware-class.diff --]
[-- Type: text/plain, Size: 22011 bytes --]

diff --exclude=CVS -urN linux-2.5.orig/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
--- linux-2.5.orig/drivers/base/Makefile	2003-05-17 20:44:03.000000000 +0200
+++ linux-2.5.mine/drivers/base/Makefile	2003-05-17 23:17:21.000000000 +0200
@@ -3,4 +3,6 @@
 obj-y			:= core.o sys.o interface.o power.o bus.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o
+obj-m			:= firmware_class.o firmware_sample_driver.o \
+			   firmware_sample_firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o  memblk.o
diff --exclude=CVS -urN linux-2.5.orig/drivers/base/firmware_class.c linux-2.5.mine/drivers/base/firmware_class.c
--- linux-2.5.orig/drivers/base/firmware_class.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.mine/drivers/base/firmware_class.c	2003-05-17 22:44:27.000000000 +0200
@@ -0,0 +1,425 @@
+/*
+ * firmware_class.c - Multi purpose firmware loading support
+ *
+ * Copyright (c) 2003 Manuel Estrada Sainz <ranty@debian.org>
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/timer.h>
+#include <asm/hardirq.h>
+
+#include "linux/firmware.h"
+
+MODULE_AUTHOR("Manuel Estrada Sainz <ranty@debian.org>");
+MODULE_DESCRIPTION("Multi purpose firmware loading support");
+MODULE_LICENSE("GPL");
+
+static int loading_timeout = 10; /* In seconds */
+
+static inline struct class_device *to_class_dev(struct kobject *obj)
+{
+	return container_of(obj,struct class_device,kobj);
+}
+static inline
+struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
+{
+	return container_of(_attr,struct class_device_attribute,attr);
+}
+
+int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+
+struct firmware_priv {
+	char fw_id[FIRMWARE_NAME_MAX];
+	struct completion completion;
+	struct bin_attribute attr_data;
+	struct firmware *fw;
+	s32 loading:2;
+	u32 abort:1;
+	int alloc_size;
+	struct timer_list timeout;
+};
+
+static ssize_t firmware_timeout_show(struct class *class, char *buf)
+{
+	return sprintf(buf, "%d\n", loading_timeout);
+}
+static ssize_t firmware_timeout_store(struct class *class,
+				      const char *buf, size_t count)
+{
+	loading_timeout = simple_strtol(buf, NULL, 10);
+	return count;
+}
+CLASS_ATTR(timeout, 0644, firmware_timeout_show, firmware_timeout_store);
+
+int firmware_class_hotplug(struct class_device *dev, char **envp,
+			   int num_envp, char *buffer, int buffer_size);
+
+struct class firmware_class = {
+        .name           = "firmware",
+	.hotplug        = firmware_class_hotplug,
+};
+
+
+int firmware_class_hotplug(struct class_device *class_dev, char **envp,
+			   int num_envp, char *buffer, int buffer_size)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	int i=0;
+	char *scratch=buffer;
+
+	if (buffer_size < (FIRMWARE_NAME_MAX+10))
+		return -ENOMEM;
+
+	envp [i++] = scratch;
+	scratch += sprintf(scratch, "FIRMWARE=%s", fw_priv->fw_id) + 1;
+	return 0;
+}
+
+static ssize_t firmware_loading_show(struct class_device *class_dev, char *buf)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	return sprintf(buf, "%d\n", fw_priv->loading);
+}
+static ssize_t firmware_loading_store(struct class_device *class_dev,
+				      const char *buf, size_t count)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	int prev_loading = fw_priv->loading;
+
+	fw_priv->loading = simple_strtol(buf, NULL, 10);
+	
+	switch(fw_priv->loading){
+	case -1:
+		fw_priv->abort = 1;
+		wmb();
+		complete(&fw_priv->completion);
+		break;
+	case 1:
+		kfree(fw_priv->fw->data);
+		fw_priv->fw->data=NULL;
+		fw_priv->fw->size=0;
+		fw_priv->alloc_size=0;
+		break;
+	case 0:
+		if(prev_loading==1)
+			complete(&fw_priv->completion);
+		break;
+	}
+
+	return count;
+}
+CLASS_DEVICE_ATTR(loading, 0644,
+		  firmware_loading_show, firmware_loading_store);
+
+static ssize_t firmware_data_read(struct kobject *kobj,
+				  char *buffer, loff_t offset, size_t count)
+{
+	struct class_device *class_dev = to_class_dev(kobj);
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	struct firmware *fw = fw_priv->fw;
+
+	printk("%s: count:%d offset:%lld\n", __FUNCTION__, count, offset);
+
+	if (offset > fw->size)
+		return 0;
+	if (offset + count > fw->size)
+		count = fw->size - offset;
+
+	memcpy(buffer, fw->data, fw->size);
+	return count;
+}
+static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
+{
+	u8 *new_data;
+
+	if (min_size <= fw_priv->alloc_size)
+		return 0;
+
+	new_data = kmalloc(fw_priv->alloc_size+PAGE_SIZE, GFP_KERNEL);
+	if(!new_data){
+		printk(KERN_ERR "%s: unable to alloc buffer\n",
+		       __FUNCTION__);
+		/* Make sure that we don't keep incomplete data */
+		fw_priv->abort = 1;
+		return -ENOMEM;
+	}
+	fw_priv->alloc_size += PAGE_SIZE;
+	if(fw_priv->fw->data){
+		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
+		kfree(fw_priv->fw->data);
+	}
+	fw_priv->fw->data=new_data;
+	BUG_ON(min_size > fw_priv->alloc_size);
+	return 0;
+}
+static ssize_t firmware_data_write(struct kobject *kobj,
+				   char *buffer, loff_t offset, size_t count)
+{
+	struct class_device *class_dev = to_class_dev(kobj);
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	struct firmware *fw = fw_priv->fw;
+	int retval;
+
+	printk("%s: count:%d offset:%lld\n", __FUNCTION__, count, offset);
+	retval = fw_realloc_buffer(fw_priv, offset+count);
+	if(retval){
+		printk("%s: retval:%d\n", __FUNCTION__, retval);
+		return retval;
+	}
+	printk("%s: retval:%d\n", __FUNCTION__, retval);
+
+	memcpy(fw->data+offset, buffer, count);
+
+	fw->size = max_t(size_t, offset+count, fw->size);
+
+	return count;
+}
+static struct bin_attribute firmware_attr_data_tmpl = {
+	.attr = {.name = "data", .mode = 0644},
+	.size = 0,
+	.read = firmware_data_read,
+	.write = firmware_data_write,
+};
+static void firmware_class_timeout(u_long data)
+{
+	struct firmware_priv *fw_priv = (struct firmware_priv *)data;
+	fw_priv->abort=1;
+	wmb();
+	complete(&fw_priv->completion);	
+}
+static inline void fw_setup_class_device_id(struct class_device *class_dev,
+				       struct device *dev)
+{
+#warning we should watch out for name collisions
+	strncpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
+	class_dev->class_id[BUS_ID_SIZE-1] = '\0';
+}
+static int fw_setup_class_device(struct class_device *class_dev,
+				 const char *fw_name,
+				 struct device *device)
+{
+	int retval = 0;
+	struct firmware_priv *fw_priv = kmalloc(sizeof(struct firmware_priv),
+						GFP_KERNEL);
+
+	if(!fw_priv){
+		retval = -ENOMEM;
+		goto out;
+	}
+	memset(fw_priv, 0, sizeof(*fw_priv));
+	memset(class_dev, 0, sizeof(*class_dev));
+
+	init_completion(&fw_priv->completion);
+	memcpy(&fw_priv->attr_data, &firmware_attr_data_tmpl,
+	       sizeof(firmware_attr_data_tmpl));
+
+	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
+	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
+
+	fw_setup_class_device_id(class_dev, device);
+	class_dev->dev = device;
+
+	fw_priv->timeout.function = firmware_class_timeout;
+	fw_priv->timeout.data = (u_long)fw_priv;
+	init_timer(&fw_priv->timeout);
+
+	class_dev->class = &firmware_class,
+	class_set_devdata(class_dev, fw_priv);
+	retval = class_device_register(class_dev);
+	if (retval){
+		printk(KERN_ERR "%s: class_device_register failed\n",
+		       __FUNCTION__);
+		goto error_free_fw_priv;
+	}
+
+	retval = sysfs_create_bin_file(&class_dev->kobj,
+				       &fw_priv->attr_data);
+	if (retval){
+		printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
+		       __FUNCTION__);
+		goto error_unreg_class_dev;
+	}
+
+	retval = class_device_create_file(class_dev,
+					  &class_device_attr_loading);
+	if (retval){
+		printk(KERN_ERR "%s: class_device_create_file failed\n",
+		       __FUNCTION__);
+		goto error_remove_data;
+	}
+
+	fw_priv->fw=kmalloc(sizeof(struct firmware), GFP_KERNEL);
+	if(!fw_priv->fw){
+		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
+		       __FUNCTION__);
+		retval = -ENOMEM;
+		goto error_remove_loading;
+	}
+	memset(fw_priv->fw, 0, sizeof(*fw_priv->fw));
+
+	goto out;
+	
+error_remove_loading:
+	class_device_remove_file(class_dev, &class_device_attr_loading);
+error_remove_data:
+	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
+error_unreg_class_dev:
+	class_device_unregister(class_dev);
+error_free_fw_priv:
+	kfree(fw_priv);
+out:
+	return retval;
+}
+static void fw_remove_class_device(struct class_device *class_dev)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	class_device_remove_file(class_dev, &class_device_attr_loading);
+	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
+	class_device_unregister(class_dev);
+}
+
+int request_firmware (const struct firmware **firmware, const char *name,
+		      struct device *device)
+{
+	struct class_device *class_dev = kmalloc(sizeof(struct class_device),
+						 GFP_KERNEL);
+	struct firmware_priv *fw_priv;
+	int retval;
+
+	if(!class_dev)
+		return -ENOMEM;
+
+	if(!firmware){
+		retval = -EINVAL;
+		goto out;
+	}
+	*firmware=NULL;
+
+	retval = fw_setup_class_device(class_dev, name, device);
+	if(retval)
+		goto out;
+
+	fw_priv = class_get_devdata(class_dev);
+
+	fw_priv->timeout.expires = jiffies + loading_timeout*HZ;
+	add_timer(&fw_priv->timeout);
+
+	wait_for_completion(&fw_priv->completion);
+
+	del_timer(&fw_priv->timeout);
+	fw_remove_class_device(class_dev);
+
+	if(fw_priv->fw->size && !fw_priv->abort){
+		*firmware = fw_priv->fw;
+	} else {
+		retval = -ENOENT;
+		kfree(fw_priv->fw->data);
+		kfree(fw_priv->fw);
+	}
+	kfree(fw_priv);
+out:
+	kfree(class_dev);
+	return retval;
+}
+void release_firmware (const struct firmware *fw)
+{
+	if(fw){
+		kfree(fw->data);
+		kfree(fw);
+	}
+}
+
+void register_firmware (const char *name, const u8 *data, size_t size)
+{
+	/* This is meaningless without firmware caching, so until we
+	 * decide if firmware caching is reasonable just leave it as a
+	 * noop */
+}
+
+/* Async support */
+struct firmware_work {
+	struct work_struct work;
+	struct module *module;
+	const char *name;
+	struct device *device;
+	void *context;
+	void (*cont)(const struct firmware *fw, void *context);
+};
+
+static void request_firmware_work_func(void *arg)
+{
+	struct firmware_work *fw_work = arg;
+	const struct firmware *fw;
+	if(!arg)
+		return;
+	request_firmware(&fw, fw_work->name, fw_work->device);
+	fw_work->cont(fw, fw_work->context);
+	release_firmware(fw);
+	module_put(fw_work->module);
+	kfree(fw_work);
+}
+
+int request_firmware_nowait (
+	struct module *module,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	struct firmware_work *fw_work = kmalloc(sizeof(struct firmware_work),
+						GFP_ATOMIC);
+	if(!fw_work)
+		return -ENOMEM;
+	if(!try_module_get(module)){
+		kfree(fw_work);
+		return -EFAULT;
+	}
+
+	*fw_work = (struct firmware_work){
+		.module = module,
+		.name = name,
+		.device = device,
+		.context = context,
+		.cont = cont,
+	};
+	INIT_WORK(&fw_work->work, request_firmware_work_func, fw_work);
+
+	schedule_work(&fw_work->work);
+	return 0;
+}
+
+
+
+static int __init firmware_class_init(void)
+{
+	int error;
+        error = class_register(&firmware_class);
+        if (error) {
+		printk(KERN_ERR "%s: class_register failed\n",
+		       __FUNCTION__);
+	}
+	error = class_create_file(&firmware_class, &class_attr_timeout);
+	if (error){
+		printk(KERN_ERR "%s: class_create_file failed\n",
+		       __FUNCTION__);
+		class_unregister(&firmware_class);
+	}
+        return error;
+
+}
+static void __exit firmware_class_exit(void)
+{
+	class_remove_file(&firmware_class, &class_attr_timeout);
+	class_unregister(&firmware_class);
+}
+module_init(firmware_class_init);
+module_exit(firmware_class_exit);
+
+EXPORT_SYMBOL(release_firmware);
+EXPORT_SYMBOL(request_firmware);
+EXPORT_SYMBOL(request_firmware_nowait);
+EXPORT_SYMBOL(register_firmware);
+EXPORT_SYMBOL(firmware_class);
diff --exclude=CVS -urN linux-2.5.orig/drivers/base/firmware_sample_driver.c linux-2.5.mine/drivers/base/firmware_sample_driver.c
--- linux-2.5.orig/drivers/base/firmware_sample_driver.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.mine/drivers/base/firmware_sample_driver.c	2003-05-17 23:20:45.000000000 +0200
@@ -0,0 +1,115 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+
+#include "linux/firmware.h"
+
+#define WE_CAN_NEED_FIRMWARE_BEFORE_USERSPACE_IS_AVAILABLE
+#ifdef WE_CAN_NEED_FIRMWARE_BEFORE_USERSPACE_IS_AVAILABLE
+char __init inkernel_firmware[] = "let's say that this is firmware\n";
+#endif
+
+static struct device ghost_device = {
+	.name      = "Ghost Device",
+	.bus_id    = "ghost0",
+};
+
+
+static void sample_firmware_load(char *firmware, int size)
+{
+	u8 buf[size+1];
+	memcpy(buf, firmware, size);
+	buf[size] = '\0';
+	printk("firmware_sample_driver: firmware: %s\n", buf);
+}
+
+static void sample_probe_default(void)
+{
+	/* uses the default method to get the firmware */
+        const struct firmware *fw_entry;
+	printk("firmware_sample_driver: a ghost device got inserted :)\n");
+
+        if(request_firmware(&fw_entry, "sample_driver_fw", &ghost_device)!=0)
+	{
+		printk(KERN_ERR
+		       "firmware_sample_driver: Firmware not available\n");
+		return;
+	}
+	
+	sample_firmware_load(fw_entry->data, fw_entry->size);
+
+	release_firmware(fw_entry);
+
+	/* finish setting up the device */
+}
+static void sample_probe_specific(void)
+{
+	/* Uses some specific hotplug support to get the firmware from
+	 * userspace  directly into the hardware, or via some sysfs file */
+
+	/* NOTE: This currently doesn't work */
+
+	printk("firmware_sample_driver: a ghost device got inserted :)\n");
+
+        if(request_firmware(NULL, "sample_driver_fw", &ghost_device)!=0)
+	{
+		printk(KERN_ERR
+		       "firmware_sample_driver: Firmware load failed\n");
+		return;
+	}
+	
+	/* request_firmware blocks until userspace finished, so at
+	 * this point the firmware should be already in the device */
+
+	/* finish setting up the device */
+}
+static void sample_probe_async_cont(const struct firmware *fw, void *context)
+{
+	if(!fw){
+		printk(KERN_ERR
+		       "firmware_sample_driver: firmware load failed\n");
+		return;
+	}
+
+	printk("firmware_sample_driver: device pointer \"%s\"\n",
+	       (char *)context);
+	sample_firmware_load(fw->data, fw->size);
+}
+static void sample_probe_async(void)
+{
+	/* Let's say that I can't sleep */
+	int error;
+	error = request_firmware_nowait (THIS_MODULE,
+					 "sample_driver_fw", &ghost_device,
+					 "my device pointer",
+					 sample_probe_async_cont);
+	if(error){
+		printk(KERN_ERR 
+		       "firmware_sample_driver:"
+		       " request_firmware_nowait failed\n");
+	}
+}
+
+static int sample_init(void)
+{
+#ifdef WE_CAN_NEED_FIRMWARE_BEFORE_USERSPACE_IS_AVAILABLE
+	register_firmware("sample_driver_fw", inkernel_firmware,
+			  sizeof(inkernel_firmware));
+#endif
+	device_initialize(&ghost_device);
+	/* since there is no real hardware insertion I just call the
+	 * sample probe functions here */
+	sample_probe_specific();
+	sample_probe_default();
+	sample_probe_async();
+	return 0;
+}
+static void __exit sample_exit(void)
+{
+}
+
+module_init (sample_init);
+module_exit (sample_exit);
+
+MODULE_LICENSE("GPL");
diff --exclude=CVS -urN linux-2.5.orig/drivers/base/firmware_sample_firmware_class.c linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c
--- linux-2.5.orig/drivers/base/firmware_sample_firmware_class.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c	2003-05-17 23:19:44.000000000 +0200
@@ -0,0 +1,200 @@
+/*
+ * firmware_class.c - Multi purpose firmware loading support
+ *
+ * Copyright (c) 2003 Manuel Estrada Sainz <ranty@debian.org>
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/timer.h>
+#include <asm/hardirq.h>
+
+#include "linux/firmware.h"
+
+MODULE_AUTHOR("Manuel Estrada Sainz <ranty@debian.org>");
+MODULE_DESCRIPTION("Hackish sample for using firmware class directly");
+MODULE_LICENSE("GPL");
+
+static inline struct class_device *to_class_dev(struct kobject *obj)
+{
+	return container_of(obj,struct class_device,kobj);
+}
+static inline
+struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
+{
+	return container_of(_attr,struct class_device_attribute,attr);
+}
+
+int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+
+struct firmware_priv {
+	char fw_id[FIRMWARE_NAME_MAX];
+	s32 loading:2;
+	u32 abort:1;
+};
+
+extern struct class firmware_class;
+
+static ssize_t firmware_loading_show(struct class_device *class_dev, char *buf)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	return sprintf(buf, "%d\n", fw_priv->loading);
+}
+static ssize_t firmware_loading_store(struct class_device *class_dev,
+				      const char *buf, size_t count)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	int prev_loading = fw_priv->loading;
+
+	fw_priv->loading = simple_strtol(buf, NULL, 10);
+	
+	switch(fw_priv->loading){
+	case -1:
+		/* abort load an panic */
+		break;
+	case 1:
+		/* setup load */
+		break;
+	case 0:
+		if(prev_loading==1){
+			/* finish load and get the device back to working
+			 * state */
+		}
+		break;
+	}
+
+	return count;
+}
+CLASS_DEVICE_ATTR(loading, 0644,
+		  firmware_loading_show, firmware_loading_store);
+
+static ssize_t firmware_data_read(struct kobject *kobj,
+				  char *buffer, loff_t offset, size_t count)
+{
+	struct class_device *class_dev = to_class_dev(kobj);
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	/* read from the devices firmware memory */
+
+	return count;
+}
+static ssize_t firmware_data_write(struct kobject *kobj,
+				   char *buffer, loff_t offset, size_t count)
+{
+	struct class_device *class_dev = to_class_dev(kobj);
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	/* write to the devices firmware memory */
+
+	return count;
+}
+static struct bin_attribute firmware_attr_data = {
+	.attr = {.name = "data", .mode = 0644},
+	.size = 0,
+	.read = firmware_data_read,
+	.write = firmware_data_write,
+};
+static int fw_setup_class_device(struct class_device *class_dev,
+				 const char *fw_name,
+				 struct device *device)
+{
+	int retval = 0;
+	struct firmware_priv *fw_priv = kmalloc(sizeof(struct firmware_priv),
+						GFP_KERNEL);
+
+	if(!fw_priv){
+		retval = -ENOMEM;
+		goto out;
+	}
+	memset(fw_priv, 0, sizeof(*fw_priv));
+	memset(class_dev, 0, sizeof(*class_dev));
+
+	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
+	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
+
+	strncpy(class_dev->class_id, device->bus_id, BUS_ID_SIZE);
+	class_dev->class_id[BUS_ID_SIZE-1] = '\0';
+	class_dev->dev = device;
+
+	class_dev->class = &firmware_class,
+	class_set_devdata(class_dev, fw_priv);
+	retval = class_device_register(class_dev);
+	if (retval){
+		printk(KERN_ERR "%s: class_device_register failed\n",
+		       __FUNCTION__);
+		goto error_free_fw_priv;
+	}
+
+	retval = sysfs_create_bin_file(&class_dev->kobj, &firmware_attr_data);
+	if (retval){
+		printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
+		       __FUNCTION__);
+		goto error_unreg_class_dev;
+	}
+
+	retval = class_device_create_file(class_dev,
+					  &class_device_attr_loading);
+	if (retval){
+		printk(KERN_ERR "%s: class_device_create_file failed\n",
+		       __FUNCTION__);
+		goto error_remove_data;
+	}
+
+	goto out;
+	
+error_remove_data:
+	sysfs_remove_bin_file(&class_dev->kobj, &firmware_attr_data);
+error_unreg_class_dev:
+	class_device_unregister(class_dev);
+error_free_fw_priv:
+	kfree(fw_priv);
+out:
+	return retval;
+}
+static void fw_remove_class_device(struct class_device *class_dev)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+
+	class_device_remove_file(class_dev, &class_device_attr_loading);
+	sysfs_remove_bin_file(&class_dev->kobj, &firmware_attr_data);
+	class_device_unregister(class_dev);
+}
+
+static struct class_device *class_dev;
+
+static struct device my_device = {
+	.name      = "Sample Device",
+	.bus_id    = "my_dev0",
+};
+
+static int __init firmware_sample_init(void)
+{
+	int error;
+
+	device_initialize(&my_device);
+	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
+	if(!class_dev)
+		return -ENOMEM;
+
+	error = fw_setup_class_device(class_dev, "my_firmware_image",
+				      &my_device);
+	if(error){
+		kfree(class_dev);
+		return error;
+	}
+        return 0;
+
+}
+static void __exit firmware_sample_exit(void)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	fw_remove_class_device(class_dev);
+	kfree(fw_priv);
+	kfree(class_dev);
+}
+module_init(firmware_sample_init);
+module_exit(firmware_sample_exit);
+
diff --exclude=CVS -urN linux-2.5.orig/include/linux/firmware.h linux-2.5.mine/include/linux/firmware.h
--- linux-2.5.orig/include/linux/firmware.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.mine/include/linux/firmware.h	2003-05-17 22:16:36.000000000 +0200
@@ -0,0 +1,20 @@
+#ifndef _LINUX_FIRMWARE_H
+#define _LINUX_FIRMWARE_H
+#include <linux/module.h>
+#include <linux/types.h>
+#define FIRMWARE_NAME_MAX 30 
+struct firmware {
+	size_t size;
+	u8 *data;
+};
+int request_firmware (const struct firmware **fw, const char *name,
+		      struct device *device);
+int request_firmware_nowait (
+	struct module *module,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context));
+/* Maybe 'device' should be 'struct device *' */
+
+void release_firmware (const struct firmware *fw);
+void register_firmware (const char *name, const u8 *data, size_t size);
+#endif

[-- Attachment #3: sysfs-bin-flexible-size.diff --]
[-- Type: text/plain, Size: 1313 bytes --]

diff --exclude=CVS -urN linux-2.5.orig/fs/sysfs/bin.c linux-2.5.mine/fs/sysfs/bin.c
--- linux-2.5.orig/fs/sysfs/bin.c	2003-05-17 20:44:03.000000000 +0200
+++ linux-2.5.mine/fs/sysfs/bin.c	2003-05-17 14:53:01.000000000 +0200
@@ -30,10 +30,15 @@
 	loff_t offs = *off;
 	int ret;
 
-	if (offs > size)
-		return 0;
-	if (offs + count > size)
-		count = size - offs;
+	if (count > PAGE_SIZE)
+		count = PAGE_SIZE;
+
+	if(size){
+		if (offs > size)
+			return 0;
+		if (offs + count > size)
+			count = size - offs;
+	}
 
 	ret = fill_read(dentry, buffer, offs, count);
 	if (ret < 0) 
@@ -69,10 +74,14 @@
 	loff_t offs = *off;
 	int ret;
 
-	if (offs > size)
-		return 0;
-	if (offs + count > size)
-		count = size - offs;
+	if (count > PAGE_SIZE)
+		count = PAGE_SIZE;
+	if (size) {
+		if (offs > size)
+			return 0;
+		if (offs + count > size)
+			count = size - offs;
+	}
 
 	ret = -EFAULT;
 	if (copy_from_user(buffer + offs, userbuf, count))
@@ -81,7 +90,7 @@
 	count = flush_write(dentry, buffer, offs, count);
 	if (count > 0)
 		*off = offs + count;
-	ret = 0;
+	ret = count;
  Done:
 	return ret;
 }
@@ -102,7 +111,7 @@
 		goto Done;
 
 	error = -ENOMEM;
-	file->private_data = kmalloc(attr->size, GFP_KERNEL);
+	file->private_data = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!file->private_data)
 		goto Done;
 

[-- Attachment #4: sysfs-bin-lost-dget.diff --]
[-- Type: text/plain, Size: 476 bytes --]

diff --exclude=CVS -urN linux-2.5.orig/fs/sysfs/inode.c linux-2.5.mine/fs/sysfs/inode.c
--- linux-2.5.orig/fs/sysfs/inode.c	2003-05-17 20:44:03.000000000 +0200
+++ linux-2.5.mine/fs/sysfs/inode.c	2003-05-17 20:30:34.000000000 +0200
@@ -60,9 +60,10 @@
  Proceed:
 	if (init)
 		error = init(inode);
-	if (!error)
+	if (!error){
 		d_instantiate(dentry, inode);
-	else
+		dget(dentry); /* Extra count - pin the dentry in core */
+	} else
 		iput(inode);
  Done:
 	return error;

Re: request_firmware() hotplug interface, third round and a halve

[Greg KH]

Oops, forgot to respond to this, sorry...

On Sun, May 18, 2003 at 12:19:22AM +0200, Manuel Estrada Sainz wrote:
>  Hi,
> 
>  There is some new stuff:
>  -----------------------
> 
>  - 'struct device *device' is used instead of 'char *device'. So we now
>    have the device symlink :)

Nice.

>  - request_firmware_nowait is implemented, please comment on it.

Looks sane.

>  - It doesn't abuse the stack any more, or at least not that much :)

Thanks.

>  - There is a timeout, changeable from userspace. Feedback on a
>    reasonable default value appreciated.

Is this really needed?  Especially as you now have:

>  - Extended 'loading' semantics:
>  	echo 1 > loading:
> 		start a new load, and flush any data from a previous
> 		partial load.
> 	echo 0 > loading:
> 		finish load.
> 	echo -1 > loading:
> 		cancel load and give an error to the driver.

Looks good.

I'd recommend sending the sysfs patches to Pat Mochel.  He's the one to
take those.

Some minor comments about the code:

> diff --exclude=CVS -urN linux-2.5.orig/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
> --- linux-2.5.orig/drivers/base/Makefile	2003-05-17 20:44:03.000000000 +0200
> +++ linux-2.5.mine/drivers/base/Makefile	2003-05-17 23:17:21.000000000 +0200
> @@ -3,4 +3,6 @@
>  obj-y			:= core.o sys.o interface.o power.o bus.o \
>  			   driver.o class.o platform.o \
>  			   cpu.o firmware.o init.o
> +obj-m			:= firmware_class.o firmware_sample_driver.o \
> +			   firmware_sample_firmware_class.o

Why make the firmware_class.o always a module?  Shouldn't it only be
included in the core, if a driver that uses it is selected?

> +static inline struct class_device *to_class_dev(struct kobject *obj)
> +{
> +	return container_of(obj,struct class_device,kobj);
> +}
> +static inline
> +struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
> +{
> +	return container_of(_attr,struct class_device_attribute,attr);
> +}

Move these two to drivers/base/base.h as they shouldn't be defined in
two different files.

> +int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
> +int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);

If you need these, add them to include/linux/sysfs.h.

> +struct firmware_priv {
> +	char fw_id[FIRMWARE_NAME_MAX];
> +	struct completion completion;
> +	struct bin_attribute attr_data;
> +	struct firmware *fw;
> +	s32 loading:2;
> +	u32 abort:1;

Why s32 and u32?  Why not just ints for both of them?

> +struct class firmware_class = {
> +        .name           = "firmware",
> +	.hotplug        = firmware_class_hotplug,
> +};

Oops, forgot tabs there...

> +	switch(fw_priv->loading){

Please add a space before the '{'.

> +	case 0:
> +		if(prev_loading==1)

And a space after the if.  You do this in lots of places.

Other than those very minor things, this looks quite good.

thanks,

greg k-h

Re: request_firmware() hotplug interface, third round and a halve

[David Gibson]

On Wed, May 21, 2003 at 12:23:18AM -0700, Greg Kroah-Hartman wrote:
> > +struct firmware_priv {
> > +	char fw_id[FIRMWARE_NAME_MAX];
> > +	struct completion completion;
> > +	struct bin_attribute attr_data;
> > +	struct firmware *fw;
> > +	s32 loading:2;
> > +	u32 abort:1;
> 
> Why s32 and u32?  Why not just ints for both of them?

And for that matter, I don't think there's any point using bitfields,
61 bits is not worth it.

-- 
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

On Wed, May 21, 2003 at 12:23:18AM -0700, Greg KH wrote:
> Oops, forgot to respond to this, sorry...
> 
> On Sun, May 18, 2003 at 12:19:22AM +0200, Manuel Estrada Sainz wrote:
[snip]
> >  - There is a timeout, changeable from userspace. Feedback on a
> >    reasonable default value appreciated.
> 
> Is this really needed?  Especially as you now have:

 There is currently no way to know if hotplug couldn't be called at all
 or if it failed because it didn't have firmware load support.

 If that happens, we would be waiting for ever. And I'd rather make that
 a countable number of seconds :)

 I'll make '0' mean no timeout at all.

> >  - Extended 'loading' semantics:
> >  	echo 1 > loading:
> > 		start a new load, and flush any data from a previous
> > 		partial load.
> > 	echo 0 > loading:
> > 		finish load.
> > 	echo -1 > loading:
> > 		cancel load and give an error to the driver.
> 
> Looks good.
> 
> I'd recommend sending the sysfs patches to Pat Mochel.  He's the one to
> take those.

 Done.
 
> Some minor comments about the code:
> 
> > diff --exclude=CVS -urN linux-2.5.orig/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
> > --- linux-2.5.orig/drivers/base/Makefile	2003-05-17 20:44:03.000000000 +0200
> > +++ linux-2.5.mine/drivers/base/Makefile	2003-05-17 23:17:21.000000000 +0200
> > @@ -3,4 +3,6 @@
> >  obj-y			:= core.o sys.o interface.o power.o bus.o \
> >  			   driver.o class.o platform.o \
> >  			   cpu.o firmware.o init.o
> > +obj-m			:= firmware_class.o firmware_sample_driver.o \
> > +			   firmware_sample_firmware_class.o
> 
> Why make the firmware_class.o always a module?  Shouldn't it only be
> included in the core, if a driver that uses it is selected?

 That was the quickest way to get modules to play with :-)

 I am not sure on how to implement "if a driver that uses it is
 selected" and not sure on where to add the Kconfig entries to make it
 available to out-of-kernel modules.

 If the approach in the attached patches is still not right, please
 complain.
 
 Maybe kbuild could allow forcing one option from another, a companion
 for 'depends', lets call it 'hard_depends'

	 depends FOO
		If FOO is not there the entry won't even be shown in the
		menu.
	 hard_depends FOO 
		FOO gets set to satisfy the dependency.

 When trying to deselect an item that is hard_depended upon, the system
 could complain and tell you which options should be deselected or even
 offer to do it automatically.

 This would also simplify the selection of crc32 and
 compression/decompression code. And allow removal of all those comments
 like "Video4Linux support is needed for USB Multimedia device support".

> > +static inline struct class_device *to_class_dev(struct kobject *obj)
> > +{
> > +	return container_of(obj,struct class_device,kobj);
> > +}
> > +static inline
> > +struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
> > +{
> > +	return container_of(_attr,struct class_device_attribute,attr);
> > +}
> 
> Move these two to drivers/base/base.h as they shouldn't be defined in
> two different files.

 OK, I also removed equivalent macros from class.c
 
> > +int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
> > +int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
> 
> If you need these, add them to include/linux/sysfs.h.
> 
> > +struct firmware_priv {
> > +	char fw_id[FIRMWARE_NAME_MAX];
> > +	struct completion completion;
> > +	struct bin_attribute attr_data;
> > +	struct firmware *fw;
> > +	s32 loading:2;
> > +	u32 abort:1;
> 
> Why s32 and u32?  Why not just ints for both of them?

 Since they are bit fields, I wanted to have more control over them and a
 single bit bit-field should be unsigned, but I guess that David is
 right, it is not worth it. Both are full int's now.
 
> > +struct class firmware_class = {
> > +        .name           = "firmware",
> > +	.hotplug        = firmware_class_hotplug,
> > +};
> 
> Oops, forgot tabs there...
> 
> > +	switch(fw_priv->loading){
> 
> Please add a space before the '{'.
> 
> > +	case 0:
> > +		if(prev_loading==1)
> 
> And a space after the if.  You do this in lots of places.

 Fixed, and just in case, I put it through Lindent which BTW is a little
 evil :-) so I hand removed the evilness I could not take.

> Other than those very minor things, this looks quite good.

 Great.

 Patches:
	firmware-class.diff:
		The code itself against bk-cvs.

	class-casts.diff:
		to_class_dev/to_class_dev_attr changes against bk-cvs.

	sysfs-bin-header.diff
	sysfs-bin-lost-dget.diff
	sysfs-bin-flexible-size.diff:
		Just for completeness, since the above will not work
		without them, but I already send them to Pat Mochel in a
		separate mail.

 	incremental.diff:
		Incremental patch for easier reading, although there are
		lots of formating changes.

 Thanks

 	Manuel
-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

On Wed, May 21, 2003 at 05:44:56PM +1000, David Gibson wrote:
> On Wed, May 21, 2003 at 12:23:18AM -0700, Greg Kroah-Hartman wrote:
> > > +struct firmware_priv {
> > > +	char fw_id[FIRMWARE_NAME_MAX];
> > > +	struct completion completion;
> > > +	struct bin_attribute attr_data;
> > > +	struct firmware *fw;
> > > +	s32 loading:2;
> > > +	u32 abort:1;
> > 
> > Why s32 and u32?  Why not just ints for both of them?
> 
> And for that matter, I don't think there's any point using bitfields,
> 61 bits is not worth it.

 Done, I just sent updated patches.

 Thanks

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]

On Wed, May 21, 2003 at 12:23:18AM -0700, Greg KH wrote:
> Oops, forgot to respond to this, sorry...
> 
> On Sun, May 18, 2003 at 12:19:22AM +0200, Manuel Estrada Sainz wrote:
[snip]
> >  - There is a timeout, changeable from userspace. Feedback on a
> >    reasonable default value appreciated.
> 
> Is this really needed?  Especially as you now have:

 There is currently no way to know if hotplug couldn't be called at all
 or if it failed because it didn't have firmware load support.

 If that happens, we would be waiting for ever. And I'd rather make that
 a countable number of seconds :)

 I'll make '0' mean no timeout at all.

> >  - Extended 'loading' semantics:
> >  	echo 1 > loading:
> > 		start a new load, and flush any data from a previous
> > 		partial load.
> > 	echo 0 > loading:
> > 		finish load.
> > 	echo -1 > loading:
> > 		cancel load and give an error to the driver.
> 
> Looks good.
> 
> I'd recommend sending the sysfs patches to Pat Mochel.  He's the one to
> take those.

 Done.
 
> Some minor comments about the code:
> 
> > diff --exclude=CVS -urN linux-2.5.orig/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
> > --- linux-2.5.orig/drivers/base/Makefile	2003-05-17 20:44:03.000000000 +0200
> > +++ linux-2.5.mine/drivers/base/Makefile	2003-05-17 23:17:21.000000000 +0200
> > @@ -3,4 +3,6 @@
> >  obj-y			:= core.o sys.o interface.o power.o bus.o \
> >  			   driver.o class.o platform.o \
> >  			   cpu.o firmware.o init.o
> > +obj-m			:= firmware_class.o firmware_sample_driver.o \
> > +			   firmware_sample_firmware_class.o
> 
> Why make the firmware_class.o always a module?  Shouldn't it only be
> included in the core, if a driver that uses it is selected?

 That was the quickest way to get modules to play with :-)

 I am not sure on how to implement "if a driver that uses it is
 selected" and not sure on where to add the Kconfig entries to make it
 available to out-of-kernel modules.

 If the approach in the attached patches is still not right, please
 complain.
 
 Maybe kbuild could allow forcing one option from another, a companion
 for 'depends', lets call it 'hard_depends'

	 depends FOO
		If FOO is not there the entry won't even be shown in the
		menu.
	 hard_depends FOO 
		FOO gets set to satisfy the dependency.

 When trying to deselect an item that is hard_depended upon, the system
 could complain and tell you which options should be deselected or even
 offer to do it automatically.

 This would also simplify the selection of crc32 and
 compression/decompression code. And allow removal of all those comments
 like "Video4Linux support is needed for USB Multimedia device support".

> > +static inline struct class_device *to_class_dev(struct kobject *obj)
> > +{
> > +	return container_of(obj,struct class_device,kobj);
> > +}
> > +static inline
> > +struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
> > +{
> > +	return container_of(_attr,struct class_device_attribute,attr);
> > +}
> 
> Move these two to drivers/base/base.h as they shouldn't be defined in
> two different files.

 OK, I also removed equivalent macros from class.c
 
> > +int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
> > +int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
> 
> If you need these, add them to include/linux/sysfs.h.
> 
> > +struct firmware_priv {
> > +	char fw_id[FIRMWARE_NAME_MAX];
> > +	struct completion completion;
> > +	struct bin_attribute attr_data;
> > +	struct firmware *fw;
> > +	s32 loading:2;
> > +	u32 abort:1;
> 
> Why s32 and u32?  Why not just ints for both of them?

 Since they are bit fields, I wanted to have more control over them and a
 single bit bit-field should be unsigned, but I guess that David is
 right, it is not worth it. Both are full int's now.
 
> > +struct class firmware_class = {
> > +        .name           = "firmware",
> > +	.hotplug        = firmware_class_hotplug,
> > +};
> 
> Oops, forgot tabs there...
> 
> > +	switch(fw_priv->loading){
> 
> Please add a space before the '{'.
> 
> > +	case 0:
> > +		if(prev_loading==1)
> 
> And a space after the if.  You do this in lots of places.

 Fixed, and just in case, I put it through Lindent which BTW is a little
 evil :-) so I hand removed the evilness I could not take.

> Other than those very minor things, this looks quite good.

 Great.

 Patches:
	firmware-class.diff:
		The code itself against bk-cvs.

	class-casts.diff:
		to_class_dev/to_class_dev_attr changes against bk-cvs.

	sysfs-bin-header.diff
	sysfs-bin-lost-dget.diff
	sysfs-bin-flexible-size.diff:
		Just for completeness, since the above will not work
		without them, but I already send them to Pat Mochel in a
		separate mail.

 	incremental.diff:
		Incremental patch for easier reading, although there are
		lots of formating changes.

 Thanks

 	Manuel

 PS: Sorry, I forgot the patches last time :(
 PS2: Now that I really include the patches I realized their size, and
 compressed some.
-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: incremental.diff --]
[-- Type: text/plain, Size: 18688 bytes --]

diff -u linux-2.5.mine/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
--- linux-2.5.mine/drivers/base/Makefile	2003-05-17 23:17:21.000000000 +0200
+++ linux-2.5.mine/drivers/base/Makefile	2003-05-21 16:25:44.000000000 +0200
@@ -5,4 +5,10 @@
 			   cpu.o firmware.o init.o
-obj-m			:= firmware_class.o firmware_sample_driver.o \
-			   firmware_sample_firmware_class.o
+obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
+
+#The next three lines are just to make testing easier and should not get into
+#the kernel
+obj-$(CONFIG_FW_LOADER_SAMPLE)	+= firmware_class.o
+obj-$(CONFIG_FW_LOADER_SAMPLE)	+= firmware_sample_driver.o \
+				   firmware_sample_firmware_class.o
+
 obj-$(CONFIG_NUMA)	+= node.o  memblk.o
reverted:
--- include/linux/sysfs.h	21 May 2003 13:49:29 -0000
+++ include/linux/sysfs.h	15 May 2003 23:50:23 -0000	1.9
@@ -23,9 +23,6 @@
 	ssize_t (*write)(struct kobject *, char *, loff_t, size_t);
 };
 
-int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
-int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
-
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
only in patch2:
unchanged:
--- linux-2.5.orig/drivers/base/base.h	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/drivers/base/base.h	2003-05-21 10:25:09.000000000 +0200
@@ -4,3 +4,12 @@
 extern int bus_add_driver(struct device_driver *);
 extern void bus_remove_driver(struct device_driver *);
 
+static inline struct class_device *to_class_dev(struct kobject *obj)
+{
+	return container_of(obj,struct class_device,kobj);
+}
+static inline
+struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
+{
+	return container_of(_attr,struct class_device_attribute,attr);
+}
only in patch2:
unchanged:
--- linux-2.5.orig/drivers/base/class.c	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/drivers/base/class.c	2003-05-21 10:19:41.000000000 +0200
@@ -148,9 +148,6 @@
 }
 
 
-#define to_class_dev(obj) container_of(obj,struct class_device,kobj)
-#define to_class_dev_attr(_attr) container_of(_attr,struct class_device_attribute,attr)
-
 static ssize_t
 class_device_attr_show(struct kobject * kobj, struct attribute * attr,
 		       char * buf)
only in patch2:
unchanged:
--- linux-2.5.orig/drivers/base/Kconfig.lib	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.mine/drivers/base/Kconfig.lib	2003-05-21 16:24:14.000000000 +0200
@@ -0,0 +1,17 @@
+#
+# To be sourced from lib/Kconfig
+#
+
+config FW_LOADER
+	tristate "Hotplug firmware loading support"
+	---help---
+	  This option is provided for the case where no in-kernel-tree modules
+	  require hotplug firmware loading support, but a module built outside
+	  the kernel tree does.
+
+config FW_LOADER_SAMPLE
+	tristate "Hotplug firmware loading samples"
+	---help---
+	  This should not get in the kernel, it is just here to make playing
+	  with firmware loading easier.
+
only in patch2:
unchanged:
--- linux-2.5.orig/lib/Kconfig	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/lib/Kconfig	2003-05-21 16:12:41.000000000 +0200
@@ -26,5 +26,6 @@
 		(PPP_DEFLATE=m || JFFS2_FS=m || CRYPTO_DEFLATE=m)
 	default y if PPP_DEFLATE=y || JFFS2_FS=y || CRYPTO_DEFLATE=y
 
-endmenu
+source "drivers/base/Kconfig.lib"
 
+endmenu
only in patch2:
unchanged:
--- linux-2.5.orig/include/linux/sysfs.h	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/include/linux/sysfs.h	2003-05-21 10:22:08.000000000 +0200
@@ -23,6 +23,9 @@
 	ssize_t (*write)(struct kobject *, char *, loff_t, size_t);
 };
 
+int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
diff -u linux-2.5.mine/drivers/base/firmware_class.c linux-2.5.mine/drivers/base/firmware_class.c
--- linux-2.5.mine/drivers/base/firmware_class.c	2003-05-17 22:44:27.000000000 +0200
+++ linux-2.5.mine/drivers/base/firmware_class.c	2003-05-21 16:38:06.000000000 +0200
@@ -11,88 +11,82 @@
 #include <linux/timer.h>
 #include <asm/hardirq.h>
 
-#include "linux/firmware.h"
+#include <linux/firmware.h>
+#include "base.h"
 
 MODULE_AUTHOR("Manuel Estrada Sainz <ranty@debian.org>");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
-static int loading_timeout = 10; /* In seconds */
-
-static inline struct class_device *to_class_dev(struct kobject *obj)
-{
-	return container_of(obj,struct class_device,kobj);
-}
-static inline
-struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
-{
-	return container_of(_attr,struct class_device_attribute,attr);
-}
-
-int sysfs_create_bin_file(struct kobject * kobj, struct bin_attribute * attr);
-int sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr);
+static int loading_timeout = 10;	/* In seconds */
 
 struct firmware_priv {
 	char fw_id[FIRMWARE_NAME_MAX];
 	struct completion completion;
 	struct bin_attribute attr_data;
 	struct firmware *fw;
-	s32 loading:2;
-	u32 abort:1;
+	int loading;
+	int abort;
 	int alloc_size;
 	struct timer_list timeout;
 };
 
-static ssize_t firmware_timeout_show(struct class *class, char *buf)
+static ssize_t
+firmware_timeout_show(struct class *class, char *buf)
 {
 	return sprintf(buf, "%d\n", loading_timeout);
 }
-static ssize_t firmware_timeout_store(struct class *class,
-				      const char *buf, size_t count)
+static ssize_t
+firmware_timeout_store(struct class *class, const char *buf, size_t count)
 {
 	loading_timeout = simple_strtol(buf, NULL, 10);
 	return count;
 }
+
 CLASS_ATTR(timeout, 0644, firmware_timeout_show, firmware_timeout_store);
 
 int firmware_class_hotplug(struct class_device *dev, char **envp,
 			   int num_envp, char *buffer, int buffer_size);
 
 struct class firmware_class = {
-        .name           = "firmware",
-	.hotplug        = firmware_class_hotplug,
+	.name		= "firmware",
+	.hotplug	= firmware_class_hotplug,
 };
 
-
-int firmware_class_hotplug(struct class_device *class_dev, char **envp,
-			   int num_envp, char *buffer, int buffer_size)
+int
+firmware_class_hotplug(struct class_device *class_dev, char **envp,
+		       int num_envp, char *buffer, int buffer_size)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
-	int i=0;
-	char *scratch=buffer;
+	int i = 0;
+	char *scratch = buffer;
 
-	if (buffer_size < (FIRMWARE_NAME_MAX+10))
+	if (buffer_size < (FIRMWARE_NAME_MAX + 10))
+		return -ENOMEM;
+	if (num_envp < 1)
 		return -ENOMEM;
 
-	envp [i++] = scratch;
+	envp[i++] = scratch;
 	scratch += sprintf(scratch, "FIRMWARE=%s", fw_priv->fw_id) + 1;
 	return 0;
 }
 
-static ssize_t firmware_loading_show(struct class_device *class_dev, char *buf)
+static ssize_t
+firmware_loading_show(struct class_device *class_dev, char *buf)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
 	return sprintf(buf, "%d\n", fw_priv->loading);
 }
-static ssize_t firmware_loading_store(struct class_device *class_dev,
-				      const char *buf, size_t count)
+static ssize_t
+firmware_loading_store(struct class_device *class_dev,
+		       const char *buf, size_t count)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
 	int prev_loading = fw_priv->loading;
 
 	fw_priv->loading = simple_strtol(buf, NULL, 10);
-	
-	switch(fw_priv->loading){
+
+	switch (fw_priv->loading) {
 	case -1:
 		fw_priv->abort = 1;
 		wmb();
@@ -100,23 +94,25 @@
 		break;
 	case 1:
 		kfree(fw_priv->fw->data);
-		fw_priv->fw->data=NULL;
-		fw_priv->fw->size=0;
-		fw_priv->alloc_size=0;
+		fw_priv->fw->data = NULL;
+		fw_priv->fw->size = 0;
+		fw_priv->alloc_size = 0;
 		break;
 	case 0:
-		if(prev_loading==1)
+		if (prev_loading == 1)
 			complete(&fw_priv->completion);
 		break;
 	}
 
 	return count;
 }
+
 CLASS_DEVICE_ATTR(loading, 0644,
 		  firmware_loading_show, firmware_loading_store);
 
-static ssize_t firmware_data_read(struct kobject *kobj,
-				  char *buffer, loff_t offset, size_t count)
+static ssize_t
+firmware_data_read(struct kobject *kobj,
+		   char *buffer, loff_t offset, size_t count)
 {
 	struct class_device *class_dev = to_class_dev(kobj);
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
@@ -132,32 +128,33 @@
 	memcpy(buffer, fw->data, fw->size);
 	return count;
 }
-static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
+static int
+fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 {
 	u8 *new_data;
 
 	if (min_size <= fw_priv->alloc_size)
 		return 0;
 
-	new_data = kmalloc(fw_priv->alloc_size+PAGE_SIZE, GFP_KERNEL);
-	if(!new_data){
-		printk(KERN_ERR "%s: unable to alloc buffer\n",
-		       __FUNCTION__);
+	new_data = kmalloc(fw_priv->alloc_size + PAGE_SIZE, GFP_KERNEL);
+	if (!new_data) {
+		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
 		/* Make sure that we don't keep incomplete data */
 		fw_priv->abort = 1;
 		return -ENOMEM;
 	}
 	fw_priv->alloc_size += PAGE_SIZE;
-	if(fw_priv->fw->data){
+	if (fw_priv->fw->data) {
 		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
 		kfree(fw_priv->fw->data);
 	}
-	fw_priv->fw->data=new_data;
+	fw_priv->fw->data = new_data;
 	BUG_ON(min_size > fw_priv->alloc_size);
 	return 0;
 }
-static ssize_t firmware_data_write(struct kobject *kobj,
-				   char *buffer, loff_t offset, size_t count)
+static ssize_t
+firmware_data_write(struct kobject *kobj,
+		    char *buffer, loff_t offset, size_t count)
 {
 	struct class_device *class_dev = to_class_dev(kobj);
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
@@ -165,16 +162,15 @@
 	int retval;
 
 	printk("%s: count:%d offset:%lld\n", __FUNCTION__, count, offset);
-	retval = fw_realloc_buffer(fw_priv, offset+count);
-	if(retval){
+	retval = fw_realloc_buffer(fw_priv, offset + count);
+	if (retval) {
 		printk("%s: retval:%d\n", __FUNCTION__, retval);
 		return retval;
 	}
-	printk("%s: retval:%d\n", __FUNCTION__, retval);
 
-	memcpy(fw->data+offset, buffer, count);
+	memcpy(fw->data + offset, buffer, count);
 
-	fw->size = max_t(size_t, offset+count, fw->size);
+	fw->size = max_t(size_t, offset + count, fw->size);
 
 	return count;
 }
@@ -184,41 +180,42 @@
 	.read = firmware_data_read,
 	.write = firmware_data_write,
 };
-static void firmware_class_timeout(u_long data)
+static void
+firmware_class_timeout(u_long data)
 {
 	struct firmware_priv *fw_priv = (struct firmware_priv *)data;
-	fw_priv->abort=1;
+	fw_priv->abort = 1;
 	wmb();
-	complete(&fw_priv->completion);	
+	complete(&fw_priv->completion);
 }
-static inline void fw_setup_class_device_id(struct class_device *class_dev,
-				       struct device *dev)
+static inline void
+fw_setup_class_device_id(struct class_device *class_dev, struct device *dev)
 {
 #warning we should watch out for name collisions
 	strncpy(class_dev->class_id, dev->bus_id, BUS_ID_SIZE);
-	class_dev->class_id[BUS_ID_SIZE-1] = '\0';
+	class_dev->class_id[BUS_ID_SIZE - 1] = '\0';
 }
-static int fw_setup_class_device(struct class_device *class_dev,
-				 const char *fw_name,
-				 struct device *device)
+static int
+fw_setup_class_device(struct class_device *class_dev,
+		      const char *fw_name, struct device *device)
 {
 	int retval = 0;
-	struct firmware_priv *fw_priv = kmalloc(sizeof(struct firmware_priv),
+	struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
 						GFP_KERNEL);
 
-	if(!fw_priv){
+	if (!fw_priv) {
 		retval = -ENOMEM;
 		goto out;
 	}
-	memset(fw_priv, 0, sizeof(*fw_priv));
-	memset(class_dev, 0, sizeof(*class_dev));
+	memset(fw_priv, 0, sizeof (*fw_priv));
+	memset(class_dev, 0, sizeof (*class_dev));
 
 	init_completion(&fw_priv->completion);
 	memcpy(&fw_priv->attr_data, &firmware_attr_data_tmpl,
-	       sizeof(firmware_attr_data_tmpl));
+	       sizeof (firmware_attr_data_tmpl));
 
 	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
-	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
+	fw_priv->fw_id[FIRMWARE_NAME_MAX - 1] = '\0';
 
 	fw_setup_class_device_id(class_dev, device);
 	class_dev->dev = device;
@@ -227,18 +224,17 @@
 	fw_priv->timeout.data = (u_long)fw_priv;
 	init_timer(&fw_priv->timeout);
 
-	class_dev->class = &firmware_class,
+	class_dev->class = &firmware_class;
 	class_set_devdata(class_dev, fw_priv);
 	retval = class_device_register(class_dev);
-	if (retval){
+	if (retval) {
 		printk(KERN_ERR "%s: class_device_register failed\n",
 		       __FUNCTION__);
 		goto error_free_fw_priv;
 	}
 
-	retval = sysfs_create_bin_file(&class_dev->kobj,
-				       &fw_priv->attr_data);
-	if (retval){
+	retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
+	if (retval) {
 		printk(KERN_ERR "%s: sysfs_create_bin_file failed\n",
 		       __FUNCTION__);
 		goto error_unreg_class_dev;
@@ -246,23 +242,23 @@
 
 	retval = class_device_create_file(class_dev,
 					  &class_device_attr_loading);
-	if (retval){
+	if (retval) {
 		printk(KERN_ERR "%s: class_device_create_file failed\n",
 		       __FUNCTION__);
 		goto error_remove_data;
 	}
 
-	fw_priv->fw=kmalloc(sizeof(struct firmware), GFP_KERNEL);
-	if(!fw_priv->fw){
+	fw_priv->fw = kmalloc(sizeof (struct firmware), GFP_KERNEL);
+	if (!fw_priv->fw) {
 		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
 		       __FUNCTION__);
 		retval = -ENOMEM;
 		goto error_remove_loading;
 	}
-	memset(fw_priv->fw, 0, sizeof(*fw_priv->fw));
+	memset(fw_priv->fw, 0, sizeof (*fw_priv->fw));
 
 	goto out;
-	
+
 error_remove_loading:
 	class_device_remove_file(class_dev, &class_device_attr_loading);
 error_remove_data:
@@ -274,7 +270,8 @@
 out:
 	return retval;
 }
-static void fw_remove_class_device(struct class_device *class_dev)
+static void
+fw_remove_class_device(struct class_device *class_dev)
 {
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
 
@@ -283,38 +280,41 @@
 	class_device_unregister(class_dev);
 }
 
-int request_firmware (const struct firmware **firmware, const char *name,
-		      struct device *device)
+int
+request_firmware(const struct firmware **firmware, const char *name,
+		 struct device *device)
 {
-	struct class_device *class_dev = kmalloc(sizeof(struct class_device),
+	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
 						 GFP_KERNEL);
 	struct firmware_priv *fw_priv;
 	int retval;
 
-	if(!class_dev)
+	if (!class_dev)
 		return -ENOMEM;
 
-	if(!firmware){
+	if (!firmware) {
 		retval = -EINVAL;
 		goto out;
 	}
-	*firmware=NULL;
+	*firmware = NULL;
 
 	retval = fw_setup_class_device(class_dev, name, device);
-	if(retval)
+	if (retval)
 		goto out;
 
 	fw_priv = class_get_devdata(class_dev);
 
-	fw_priv->timeout.expires = jiffies + loading_timeout*HZ;
-	add_timer(&fw_priv->timeout);
+	if (loading_timeout) {
+		fw_priv->timeout.expires = jiffies + loading_timeout * HZ;
+		add_timer(&fw_priv->timeout);
+	}
 
 	wait_for_completion(&fw_priv->completion);
 
 	del_timer(&fw_priv->timeout);
 	fw_remove_class_device(class_dev);
 
-	if(fw_priv->fw->size && !fw_priv->abort){
+	if (fw_priv->fw->size && !fw_priv->abort) {
 		*firmware = fw_priv->fw;
 	} else {
 		retval = -ENOENT;
@@ -326,15 +326,18 @@
 	kfree(class_dev);
 	return retval;
 }
-void release_firmware (const struct firmware *fw)
+
+void
+release_firmware(const struct firmware *fw)
 {
-	if(fw){
+	if (fw) {
 		kfree(fw->data);
 		kfree(fw);
 	}
 }
 
-void register_firmware (const char *name, const u8 *data, size_t size)
+void
+register_firmware(const char *name, const u8 *data, size_t size)
 {
 	/* This is meaningless without firmware caching, so until we
 	 * decide if firmware caching is reasonable just leave it as a
@@ -351,11 +354,12 @@
 	void (*cont)(const struct firmware *fw, void *context);
 };
 
-static void request_firmware_work_func(void *arg)
+static void
+request_firmware_work_func(void *arg)
 {
 	struct firmware_work *fw_work = arg;
 	const struct firmware *fw;
-	if(!arg)
+	if (!arg)
 		return;
 	request_firmware(&fw, fw_work->name, fw_work->device);
 	fw_work->cont(fw, fw_work->context);
@@ -364,21 +368,22 @@
 	kfree(fw_work);
 }
 
-int request_firmware_nowait (
+int
+request_firmware_nowait(
 	struct module *module,
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
-	struct firmware_work *fw_work = kmalloc(sizeof(struct firmware_work),
+	struct firmware_work *fw_work = kmalloc(sizeof (struct firmware_work),
 						GFP_ATOMIC);
-	if(!fw_work)
+	if (!fw_work)
 		return -ENOMEM;
-	if(!try_module_get(module)){
+	if (!try_module_get(module)) {
 		kfree(fw_work);
 		return -EFAULT;
 	}
 
-	*fw_work = (struct firmware_work){
+	*fw_work = (struct firmware_work) {
 		.module = module,
 		.name = name,
 		.device = device,
@@ -391,30 +396,30 @@
 	return 0;
 }
 
-
-
-static int __init firmware_class_init(void)
+static int __init
+firmware_class_init(void)
 {
 	int error;
-        error = class_register(&firmware_class);
-        if (error) {
-		printk(KERN_ERR "%s: class_register failed\n",
-		       __FUNCTION__);
+	error = class_register(&firmware_class);
+	if (error) {
+		printk(KERN_ERR "%s: class_register failed\n", __FUNCTION__);
 	}
 	error = class_create_file(&firmware_class, &class_attr_timeout);
-	if (error){
+	if (error) {
 		printk(KERN_ERR "%s: class_create_file failed\n",
 		       __FUNCTION__);
 		class_unregister(&firmware_class);
 	}
-        return error;
+	return error;
 
 }
-static void __exit firmware_class_exit(void)
+static void __exit
+firmware_class_exit(void)
 {
 	class_remove_file(&firmware_class, &class_attr_timeout);
 	class_unregister(&firmware_class);
 }
+
 module_init(firmware_class_init);
 module_exit(firmware_class_exit);
 
diff -u linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c
--- linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c	2003-05-17 23:19:44.000000000 +0200
+++ linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c	2003-05-21 15:58:34.000000000 +0200
@@ -32,8 +32,8 @@
 
 struct firmware_priv {
 	char fw_id[FIRMWARE_NAME_MAX];
-	s32 loading:2;
-	u32 abort:1;
+	int loading;
+	int abort;
 };
 
 extern struct class firmware_class;
@@ -68,8 +68,8 @@
 
 	return count;
 }
-CLASS_DEVICE_ATTR(loading, 0644,
-		  firmware_loading_show, firmware_loading_store);
+static CLASS_DEVICE_ATTR(loading, 0644,
+			 firmware_loading_show, firmware_loading_store);
 
 static ssize_t firmware_data_read(struct kobject *kobj,
 				  char *buffer, loff_t offset, size_t count)

[-- Attachment #3: class-casts.diff.bz2 --]
[-- Type: application/octet-stream, Size: 493 bytes --]

[-- Attachment #4: firmware-class.diff.bz2 --]
[-- Type: application/octet-stream, Size: 5045 bytes --]

[-- Attachment #5: sysfs-bin-flexible-size.diff.bz2 --]
[-- Type: application/octet-stream, Size: 600 bytes --]

[-- Attachment #6: sysfs-bin-header.diff.bz2 --]
[-- Type: application/octet-stream, Size: 348 bytes --]

[-- Attachment #7: sysfs-bin-lost-dget.diff.bz2 --]
[-- Type: application/octet-stream, Size: 327 bytes --]

Re: request_firmware() hotplug interface, third round and a halve

[Greg KH]

On Wed, May 21, 2003 at 08:34:35PM +0200, Manuel Estrada Sainz wrote:
> 
>  If the approach in the attached patches is still not right, please
>  complain.

-ENOPATCHES  :)

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Greg KH]

On Wed, May 21, 2003 at 08:52:12PM +0200, Manuel Estrada Sainz wrote:
> On Wed, May 21, 2003 at 12:23:18AM -0700, Greg KH wrote:
> > Oops, forgot to respond to this, sorry...
> > 
> > On Sun, May 18, 2003 at 12:19:22AM +0200, Manuel Estrada Sainz wrote:
> [snip]
> > >  - There is a timeout, changeable from userspace. Feedback on a
> > >    reasonable default value appreciated.
> > 
> > Is this really needed?  Especially as you now have:
> 
>  There is currently no way to know if hotplug couldn't be called at all
>  or if it failed because it didn't have firmware load support.
> 
>  If that happens, we would be waiting for ever. And I'd rather make that
>  a countable number of seconds :)
> 
>  I'll make '0' mean no timeout at all.

Ok, that's fine with me.  A bit of documentation for all of this might
be nice.  Just add some kerneldoc comments to your public functions, and
you should be fine.

>  I am not sure on how to implement "if a driver that uses it is
>  selected" and not sure on where to add the Kconfig entries to make it
>  available to out-of-kernel modules.

You could do something like what has been done for the mii module.  Look
at lib/Makefile and drivers/usb/net/Makefile.mii for an example.

I'm not saying that this is the best way, but it could be one solution.
Ideally, the user would never have to select the firmware core option,
it would just get automatically built if a driver that needs it is also
selected.

>  Patches:

The patches look great.  Add a bit of documentation, and some build
integration, and I'd think you are finished.  Unless anyone else has any
objections?

Very nice job, thanks again for doing this.

greg k-h

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Sam Ravnborg]

On Wed, May 21, 2003 at 08:52:12PM +0200, Manuel Estrada Sainz wrote:
>  
>  Maybe kbuild could allow forcing one option from another, a companion
>  for 'depends', lets call it 'hard_depends'
> 
> 	 depends FOO
> 		If FOO is not there the entry won't even be shown in the
> 		menu.
> 	 hard_depends FOO 
> 		FOO gets set to satisfy the dependency.
> 

Roman Zippel introduced: "enable" in his latest Kconfig goodies:
http://marc.theaimsgroup.com/?l=linux-kernel&m=105274692328723&w=2

Would that be usefull for you?
This would allow you to set CONFIG_LOADER for the drivers that
really needs it.

IIRC this is not included in Linus-BK yet.

	Sam

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

On Wed, May 21, 2003 at 11:51:16PM +0200, Sam Ravnborg wrote:
> On Wed, May 21, 2003 at 08:52:12PM +0200, Manuel Estrada Sainz wrote:
> >  
> >  Maybe kbuild could allow forcing one option from another, a companion
> >  for 'depends', lets call it 'hard_depends'
> > 
> > 	 depends FOO
> > 		If FOO is not there the entry won't even be shown in the
> > 		menu.
> > 	 hard_depends FOO 
> > 		FOO gets set to satisfy the dependency.
> > 
> 
> Roman Zippel introduced: "enable" in his latest Kconfig goodies:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=105274692328723&w=2
> 
> Would that be usefull for you?
> This would allow you to set CONFIG_LOADER for the drivers that
> really needs it.

 Yep, that is perfect.
 
> IIRC this is not included in Linus-BK yet.

 Nop :( once it is in, I'll start using it. Oh, and I volunteer to
 cleanup the CRC32 stuff on top of that :)

 Have a nice day

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

[-- Attachment #1: Type: text/plain, Size: 3856 bytes --]

On Wed, May 21, 2003 at 01:07:36PM -0700, Greg KH wrote:
> On Wed, May 21, 2003 at 08:52:12PM +0200, Manuel Estrada Sainz wrote:
> > On Wed, May 21, 2003 at 12:23:18AM -0700, Greg KH wrote:
> > > Oops, forgot to respond to this, sorry...
> > > 
> > > On Sun, May 18, 2003 at 12:19:22AM +0200, Manuel Estrada Sainz wrote:
> > [snip]
> > > >  - There is a timeout, changeable from userspace. Feedback on a
> > > >    reasonable default value appreciated.
> > > 
> > > Is this really needed?  Especially as you now have:
> > 
> >  There is currently no way to know if hotplug couldn't be called at all
> >  or if it failed because it didn't have firmware load support.
> > 
> >  If that happens, we would be waiting for ever. And I'd rather make that
> >  a countable number of seconds :)
> > 
> >  I'll make '0' mean no timeout at all.
> 
> Ok, that's fine with me.  A bit of documentation for all of this might
> be nice.  Just add some kerneldoc comments to your public functions, and
> you should be fine.

 Done. I am not a good documentation writer, but the attached patches
 should include enough documentation to understand how things work.
 
> >  I am not sure on how to implement "if a driver that uses it is
> >  selected" and not sure on where to add the Kconfig entries to make it
> >  available to out-of-kernel modules.
> 
> You could do something like what has been done for the mii module.  Look
> at lib/Makefile and drivers/usb/net/Makefile.mii for an example.
> 
> I'm not saying that this is the best way, but it could be one solution.
> Ideally, the user would never have to select the firmware core option,
> it would just get automatically built if a driver that needs it is also
> selected.

 But if a driver not in the kernel tree needs it, the user should be
 able to enable it unconditionally, like the CRC32 stuff.

 And currently there is no in-kernel driver that uses it, so the only
 way to enable it is manually. 

 Once Atmel PCMCIA driver (which will use it) gets in, the new 'enable'
 kconfig keyword should be already available and no makefile tricks will
 be needed.

[snip]
> Add a bit of documentation, and some build integration,

 Until drivers start using it, there is not much build integration to
 do.

> and I'd think you are finished.  Unless anyone else has any
> objections?

 Simon Kelley had an objection, it was badly corrupting kernel memory :-/.

 It was a bug in the sysfs pieces which is now fixed.

 So I am finished :-) What do I do now? Should I send it to Linus or you
 take care of that?

> Very nice job, thanks again for doing this.

 No problem.

 News:
 	- Fixed the sysfs corruption problem.
	- Use refcounting with class_device instances.
		- I had to add support for 'release forwarding' to
		  the class subsystem.
	- Removed firmware_sample_firmware_class.c
		- It is ugly, if found interesting, it should get
		  polished first.
	- Removed FW_LOADER_SAMPLE Kconfig/Makefile stuff.
	- Some more formatting changes.

 Patches:
 	incremental.diff:
		For easier reading as usual.
	
 	firmware-class.diff.bz2:
		The code itself

	firmware-class-sample-driver.diff.bz2:
		The sample driver, for easier ignoring :)

	class-casts+release.diff.bz2:
		to_class_dev/to_class_dev_attr and 'release forwarding'
		changes.
	
	sysfs-bin-header.diff.bz2
	sysfs-bin-lost-dget.diff.bz2
	sysfs-bin-flexible-size.diff.bz2:
		sysfs bits. This time, they don't corrupt kernel memory :-/
		(I'll send them to Patrick Mochel in a few moments.)

 Have a nice day

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: incremental.diff --]
[-- Type: text/plain, Size: 10672 bytes --]

diff -u linux-2.5.mine/drivers/base/class.c linux-2.5.mine/drivers/base/class.c
--- linux-2.5.mine/drivers/base/class.c	2003-05-21 10:19:41.000000000 +0200
+++ linux-2.5.mine/drivers/base/class.c	2003-05-22 16:51:46.000000000 +0200
@@ -179,7 +179,15 @@
 	.store	= class_device_attr_store,
 };
 
+static void class_dev_release(struct kobject * kobj)
+{
+	struct class_device *class_dev = to_class_dev(kobj);
+	if (class_dev->release)
+		class_dev->release(class_dev);
+}
+
 static struct kobj_type ktype_class_device = {
+	.release	= &class_dev_release,
 	.sysfs_ops	= &class_dev_sysfs_ops,
 };
 
only in patch2:
unchanged:
--- linux-2.5.orig/include/linux/device.h	2003-05-22 13:05:16.000000000 +0200
+++ linux-2.5.mine/include/linux/device.h	2003-05-22 12:05:45.000000000 +0200
@@ -204,6 +204,7 @@
 	void			* class_data;	/* class-specific data */
 
 	char	class_id[BUS_ID_SIZE];	/* unique to this class */
+	void	(*release)(struct class_device * class_dev);
 };
 
 static inline void *
diff -u linux-2.5.mine/drivers/base/Kconfig.lib linux-2.5.mine/drivers/base/Kconfig.lib
--- linux-2.5.mine/drivers/base/Kconfig.lib	2003-05-21 16:24:14.000000000 +0200
+++ linux-2.5.mine/drivers/base/Kconfig.lib	2003-05-22 13:11:42.000000000 +0200
@@ -12,6 +11,0 @@
-config FW_LOADER_SAMPLE
-	tristate "Hotplug firmware loading samples"
-	---help---
-	  This should not get in the kernel, it is just here to make playing
-	  with firmware loading easier.
-
diff -u linux-2.5.mine/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
--- linux-2.5.mine/drivers/base/Makefile	2003-05-21 16:25:44.000000000 +0200
+++ linux-2.5.mine/drivers/base/Makefile	2003-05-22 13:12:20.000000000 +0200
@@ -6,9 +6,2 @@
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
-
-#The next three lines are just to make testing easier and should not get into
-#the kernel
-obj-$(CONFIG_FW_LOADER_SAMPLE)	+= firmware_class.o
-obj-$(CONFIG_FW_LOADER_SAMPLE)	+= firmware_sample_driver.o \
-				   firmware_sample_firmware_class.o
-
 obj-$(CONFIG_NUMA)	+= node.o  memblk.o
diff -u linux-2.5.mine/drivers/base/firmware_class.c linux-2.5.mine/drivers/base/firmware_class.c
--- linux-2.5.mine/drivers/base/firmware_class.c	2003-05-21 16:38:06.000000000 +0200
+++ linux-2.5.mine/drivers/base/firmware_class.c	2003-05-22 16:49:39.000000000 +0200
@@ -3,6 +3,19 @@
  *
  * Copyright (c) 2003 Manuel Estrada Sainz <ranty@debian.org>
  *
+ * Simple hotplug script sample:
+ * 
+ *	HOTPLUG_FW_DIR=/usr/lib/hotplug/firmware/
+ *	echo 1 > /sysfs/$DEVPATH/loading
+ *	cat $HOTPLUG_FW_DIR/$FIRMWARE > /sysfs/$DEVPATH/data
+ *	echo 0 > /sysfs/$DEVPATH/loading
+ *
+ * To cancel the load in case of error:
+ *
+ *	echo -1 > /sysfs/$DEVPATH/loading
+ *
+ * Both $DEVPATH and $FIRMWARE are already provided in the environment.
+ *
  */
 
 #include <linux/device.h>
@@ -36,6 +49,17 @@
 {
 	return sprintf(buf, "%d\n", loading_timeout);
 }
+
+/**
+ * firmware_timeout_store:
+ * Description:
+ *	Sets the number of seconds to wait for the firmware.  Once
+ *	this expires an error will be return to the driver and no
+ *	firmware will be provided.
+ *
+ *	Note: zero means 'wait for ever'
+ *  
+ **/
 static ssize_t
 firmware_timeout_store(struct class *class, const char *buf, size_t count)
 {
@@ -77,6 +101,16 @@
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
 	return sprintf(buf, "%d\n", fw_priv->loading);
 }
+
+/**
+ * firmware_loading_store: - loading control file
+ * Description:
+ *	The relevant values are: 
+ *
+ *	 1: Start a load, discarding any previous partial load.
+ *	 0: Conclude the load and handle the data to the driver code.
+ *	-1: Conclude the load with an error and discard any written data.
+ **/
 static ssize_t
 firmware_loading_store(struct class_device *class_dev,
 		       const char *buf, size_t count)
@@ -125,7 +159,7 @@
 	if (offset + count > fw->size)
 		count = fw->size - offset;
 
-	memcpy(buffer, fw->data, fw->size);
+	memcpy(buffer, fw->data + offset, count);
 	return count;
 }
 static int
@@ -152,6 +186,15 @@
 	BUG_ON(min_size > fw_priv->alloc_size);
 	return 0;
 }
+
+/**
+ * firmware_data_write:
+ *
+ * Description:
+ *
+ *	Data written to the 'data' attribute will be later handled to
+ *	the driver as a firmware image.
+ **/
 static ssize_t
 firmware_data_write(struct kobject *kobj,
 		    char *buffer, loff_t offset, size_t count)
@@ -180,10 +223,17 @@
 	.read = firmware_data_read,
 	.write = firmware_data_write,
 };
+
+static void
+fw_class_dev_release(struct class_device *class_dev)
+{
+	kfree(class_dev);
+}
+
 static void
 firmware_class_timeout(u_long data)
 {
-	struct firmware_priv *fw_priv = (struct firmware_priv *)data;
+	struct firmware_priv *fw_priv = (struct firmware_priv *) data;
 	fw_priv->abort = 1;
 	wmb();
 	complete(&fw_priv->completion);
@@ -196,16 +246,18 @@
 	class_dev->class_id[BUS_ID_SIZE - 1] = '\0';
 }
 static int
-fw_setup_class_device(struct class_device *class_dev,
+fw_setup_class_device(struct class_device **class_dev_p,
 		      const char *fw_name, struct device *device)
 {
 	int retval = 0;
 	struct firmware_priv *fw_priv = kmalloc(sizeof (struct firmware_priv),
 						GFP_KERNEL);
+	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
+						 GFP_KERNEL);
 
-	if (!fw_priv) {
+	if (!fw_priv || !class_dev) {
 		retval = -ENOMEM;
-		goto out;
+		goto error_kfree;
 	}
 	memset(fw_priv, 0, sizeof (*fw_priv));
 	memset(class_dev, 0, sizeof (*class_dev));
@@ -221,16 +273,17 @@
 	class_dev->dev = device;
 
 	fw_priv->timeout.function = firmware_class_timeout;
-	fw_priv->timeout.data = (u_long)fw_priv;
+	fw_priv->timeout.data = (u_long) fw_priv;
 	init_timer(&fw_priv->timeout);
 
+	class_dev->release = fw_class_dev_release;
 	class_dev->class = &firmware_class;
 	class_set_devdata(class_dev, fw_priv);
 	retval = class_device_register(class_dev);
 	if (retval) {
 		printk(KERN_ERR "%s: class_device_register failed\n",
 		       __FUNCTION__);
-		goto error_free_fw_priv;
+		goto error_kfree;
 	}
 
 	retval = sysfs_create_bin_file(&class_dev->kobj, &fw_priv->attr_data);
@@ -265,9 +318,12 @@
 	sysfs_remove_bin_file(&class_dev->kobj, &fw_priv->attr_data);
 error_unreg_class_dev:
 	class_device_unregister(class_dev);
-error_free_fw_priv:
+error_kfree:
 	kfree(fw_priv);
+	kfree(class_dev);
+	*class_dev_p = NULL;
 out:
+	*class_dev_p = class_dev;
 	return retval;
 }
 static void
@@ -280,25 +336,32 @@
 	class_device_unregister(class_dev);
 }
 
+/** 
+ * request_firmware: - request firmware to hotplug and wait for it
+ * Description:
+ *	@firmware will be used to return a firmware image by the name
+ *	of @name for device @device.
+ *
+ *	Should be called from user context where sleeping is allowed.
+ *
+ *	@name will be use as $FIRMWARE in the hotplug environment and
+ *	should be distinctive enough not to be confused with any other
+ *	firmware image for this or any other device.
+ **/
 int
 request_firmware(const struct firmware **firmware, const char *name,
 		 struct device *device)
 {
-	struct class_device *class_dev = kmalloc(sizeof (struct class_device),
-						 GFP_KERNEL);
+	struct class_device *class_dev;
 	struct firmware_priv *fw_priv;
 	int retval;
 
-	if (!class_dev)
-		return -ENOMEM;
+	if (!firmware)
+		return -EINVAL;
 
-	if (!firmware) {
-		retval = -EINVAL;
-		goto out;
-	}
 	*firmware = NULL;
 
-	retval = fw_setup_class_device(class_dev, name, device);
+	retval = fw_setup_class_device(&class_dev, name, device);
 	if (retval)
 		goto out;
 
@@ -323,10 +386,12 @@
 	}
 	kfree(fw_priv);
 out:
-	kfree(class_dev);
 	return retval;
 }
 
+/**
+ * release_firmware: - release the resource associated with a firmware image
+ **/
 void
 release_firmware(const struct firmware *fw)
 {
@@ -336,6 +401,15 @@
 	}
 }
 
+/**
+ * register_firmware: - provide a firmware image for later usage
+ * 
+ * Description:
+ *	Make sure that @data will be available by requesting firmware @name.
+ *
+ *	Note: This will not be possible until some kind of persistence
+ *	is available.
+ **/
 void
 register_firmware(const char *name, const u8 *data, size_t size)
 {
@@ -368,6 +442,20 @@
 	kfree(fw_work);
 }
 
+/**
+ * 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,
diff -u linux-2.5.mine/include/linux/firmware.h linux-2.5.mine/include/linux/firmware.h
--- linux-2.5.mine/include/linux/firmware.h	2003-05-17 22:16:36.000000000 +0200
+++ linux-2.5.mine/include/linux/firmware.h	2003-05-22 16:55:06.000000000 +0200
@@ -9,12 +9,12 @@
 };
-int request_firmware (const struct firmware **fw, const char *name,
-		      struct device *device);
-int request_firmware_nowait (
+int request_firmware(const struct firmware **fw, const char *name,
+		     struct device *device);
+int request_firmware_nowait(
 	struct module *module,
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 /* Maybe 'device' should be 'struct device *' */
 
-void release_firmware (const struct firmware *fw);
-void register_firmware (const char *name, const u8 *data, size_t size);
+void release_firmware(const struct firmware *fw);
+void register_firmware(const char *name, const u8 *data, size_t size);
 #endif
diff -u linux-2.5.mine/fs/sysfs/bin.c linux-2.5.mine/fs/sysfs/bin.c
--- linux-2.5.mine/fs/sysfs/bin.c	2003-05-17 14:53:01.000000000 +0200
+++ linux-2.5.mine/fs/sysfs/bin.c	2003-05-22 16:52:42.000000000 +0200
@@ -33,7 +33,7 @@
 	if (count > PAGE_SIZE)
 		count = PAGE_SIZE;
 
-	if(size){
+	if (size) {
 		if (offs > size)
 			return 0;
 		if (offs + count > size)
@@ -46,7 +46,7 @@
 	count = ret;
 
 	ret = -EFAULT;
-	if (copy_to_user(userbuf, buffer + offs, count) != 0)
+	if (copy_to_user(userbuf, buffer, count) != 0)
 		goto Done;
 
 	*off = offs + count;
@@ -84,7 +84,7 @@
 	}
 
 	ret = -EFAULT;
-	if (copy_from_user(buffer + offs, userbuf, count))
+	if (copy_from_user(buffer, userbuf, count))
 		goto Done;
 
 	count = flush_write(dentry, buffer, offs, count);
diff -u linux-2.5.mine/fs/sysfs/inode.c linux-2.5.mine/fs/sysfs/inode.c
--- linux-2.5.mine/fs/sysfs/inode.c	2003-05-17 20:30:34.000000000 +0200
+++ linux-2.5.mine/fs/sysfs/inode.c	2003-05-22 16:53:59.000000000 +0200
@@ -60,7 +60,7 @@
  Proceed:
 	if (init)
 		error = init(inode);
-	if (!error){
+	if (!error) {
 		d_instantiate(dentry, inode);
 		dget(dentry); /* Extra count - pin the dentry in core */
 	} else

[-- Attachment #3: firmware-class.diff.bz2 --]
[-- Type: application/octet-stream, Size: 4334 bytes --]

[-- Attachment #4: firmware-class-sample-driver.diff.bz2 --]
[-- Type: application/octet-stream, Size: 1340 bytes --]

[-- Attachment #5: class-casts+release.diff.bz2 --]
[-- Type: application/octet-stream, Size: 763 bytes --]

[-- Attachment #6: sysfs-bin-flexible-size.diff.bz2 --]
[-- Type: text/plain, Size: 645 bytes --]

BZh91AY&SYV†³N\0\x01¿ß€\x020`\x7fÿ¿o¯ß\x10¿ïßú@\x02¼\x03 \0æ\x04ÄÐa2dÉ‘„Á4Ó#\x13\0C\0Ô\x06ˆ@\r\x1a\x1a\0\0\0\0\0\0æ\x04ÄÐa2dÉ‘„Á4Ó#\x13\0C\0T’\x13@ÑO*x™=Iä›PÉ“&!&jié^[MLš\f)©‚ââË•w±e”U÷Ü]kª­K—)K®RZÖ¸«4Ï\r\µsE–œ×Nꪌ\x12Ê²¦4j¸´b­Xá}õ‘&jSÝ!ÎìNÆç>šèê3ï4O_M?5©âÓôÚ¦ÚN¦µ‘z”ÁÓùqö\x1e6ÖÅŽ\x05%¬¢¸t{/dÃ2°/½¾Æq™àpÖñÖ·\x12Òs|m"ç…Ÿ¿˜ô.¾â¶¬§²•ø´2ˆ¤û©V²hÇÀû2ár–•­âóóùß®Dá°«[¡]\x1fb­;Óe¦5›Ls˜›OïB¬wš\a\x02C€ã\x10@[áC\x04P¶˜JeE¡©ZMb€@€\x12J;\bÊm”<\x12ëÄ"\0µ\x12Ý\a8b\x1f’O:¨M\0#Bcœ,*X¢›-íø/fq'¥RM\x15#qæ\x16>óª?—–øùF/3eºžøܧ¶3½QÖÁòWÇËò]–w®›—ºôÇìÉñëŽF1Í=o§gú÷»?	ïtíœ_áÊr]R½F§ºãø\x150‰ÕË;oy;Õ#Ž|$Ý|Ûg#ÒQFËäžO¬†\x13"‘mérQ\x19žíÌCIÆXÐľ¤5ë`}NV\rÆ\x062MK´K\x15ƹ™×§kµÌÕ\x1a\x12øß$²¡Ác5>¥<\x06ÒŠ\x1a\x1d±°¶·Ì¸NÉ°ÉT5ì±Ë¯|dTbîî™ã‰·N¥Ä—Ž’´“S.\x06sJ‰µBÒªœ³¿S¼`Ë\x05ù£“‚1\x13Ÿ\x18ÎkdÇIªa°Õ¹¼àf’hq«ÿ\x17rE8PV†³N

[-- Attachment #7: sysfs-bin-header.diff.bz2 --]
[-- Type: application/octet-stream, Size: 348 bytes --]

[-- Attachment #8: sysfs-bin-lost-dget.diff.bz2 --]
[-- Type: application/octet-stream, Size: 330 bytes --]

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Patrick Mochel]

I don't have any problems with the interface, and am ok with the driver 
core and sysfs changes. I have the sysfs binary file patch, but would you 
mind sending me the class device release() patch separately? 

Also, what about just creating a drivers/firmware/ directory (or 
drivers/base/firmware/) for the core code? 

Thanks,


	-pat

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

On Thu, May 22, 2003 at 09:14:26AM -0700, Patrick Mochel wrote:
> 
> I don't have any problems with the interface, and am ok with the driver 
> core and sysfs changes. I have the sysfs binary file patch, but would you 
> mind sending me the class device release() patch separately? 

 Sorry. Since it is small I'll simply split class-casts+release.diff and
 attach both pieces just in case.
 
> Also, what about just creating a drivers/firmware/ directory (or 
> drivers/base/firmware/) for the core code? 

 Making a directory for a single source file seams a little overkill,
 although at some point could be interesting to have all firmware images
 in the kernel on the same directory to be copied to initramfs or
 elsewhere.

 For now I don't think it is worth it's own directory, but I don't
 really care as long as the functionality is there.

 If I had to choose I would take drivers/base/firmware/
 
 Thanks

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: class-casts.diff --]
[-- Type: text/plain, Size: 1207 bytes --]

diff --exclude=CVS -urN linux-2.5.orig/drivers/base/base.h linux-2.5.mine/drivers/base/base.h
--- linux-2.5.orig/drivers/base/base.h	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/drivers/base/base.h	2003-05-21 10:25:09.000000000 +0200
@@ -4,3 +4,12 @@
 extern int bus_add_driver(struct device_driver *);
 extern void bus_remove_driver(struct device_driver *);
 
+static inline struct class_device *to_class_dev(struct kobject *obj)
+{
+	return container_of(obj,struct class_device,kobj);
+}
+static inline
+struct class_device_attribute *to_class_dev_attr(struct attribute *_attr)
+{
+	return container_of(_attr,struct class_device_attribute,attr);
+}
diff --exclude=CVS -urN linux-2.5.orig/drivers/base/class.c linux-2.5.mine/drivers/base/class.c
--- linux-2.5.orig/drivers/base/class.c	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/drivers/base/class.c	2003-05-22 16:51:46.000000000 +0200
@@ -148,9 +148,6 @@
 }
 
 
-#define to_class_dev(obj) container_of(obj,struct class_device,kobj)
-#define to_class_dev_attr(_attr) container_of(_attr,struct class_device_attribute,attr)
-
 static ssize_t
 class_device_attr_show(struct kobject * kobj, struct attribute * attr,
 		       char * buf)

[-- Attachment #3: class-device-release.diff --]
[-- Type: text/plain, Size: 1076 bytes --]

diff --exclude=CVS -urN linux-2.5.orig/drivers/base/class.c linux-2.5.mine/drivers/base/class.c
--- linux-2.5.orig/drivers/base/class.c	2003-05-21 17:38:39.000000000 +0200
+++ linux-2.5.mine/drivers/base/class.c	2003-05-22 16:51:46.000000000 +0200
@@ -182,7 +179,15 @@
 	.store	= class_device_attr_store,
 };
 
+static void class_dev_release(struct kobject * kobj)
+{
+	struct class_device *class_dev = to_class_dev(kobj);
+	if (class_dev->release)
+		class_dev->release(class_dev);
+}
+
 static struct kobj_type ktype_class_device = {
+	.release	= &class_dev_release,
 	.sysfs_ops	= &class_dev_sysfs_ops,
 };
 
diff --exclude=CVS -urN linux-2.5.orig/include/linux/device.h linux-2.5.mine/include/linux/device.h
--- linux-2.5.orig/include/linux/device.h	2003-05-22 13:05:16.000000000 +0200
+++ linux-2.5.mine/include/linux/device.h	2003-05-22 12:05:45.000000000 +0200
@@ -204,6 +204,7 @@
 	void			* class_data;	/* class-specific data */
 
 	char	class_id[BUS_ID_SIZE];	/* unique to this class */
+	void	(*release)(struct class_device * class_dev);
 };
 
 static inline void *

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Patrick Mochel]

>  Sorry. Since it is small I'll simply split class-casts+release.diff and
>  attach both pieces just in case.

Thanks.

>  If I had to choose I would take drivers/base/firmware/

Ok. Maybe I mis-read that part of the patch; I thought there was more than 
one file. No worries, then. 


	-pat

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

On Thu, May 22, 2003 at 09:38:55AM -0700, Patrick Mochel wrote:
> 
> >  Sorry. Since it is small I'll simply split class-casts+release.diff and
> >  attach both pieces just in case.
> 
> Thanks.
> 
> >  If I had to choose I would take drivers/base/firmware/
> 
> Ok. Maybe I mis-read that part of the patch; I thought there was more than 
> one file. No worries, then. 

 Actually it is two files:

 	firmware_class.c:
		the code itself
	firmware_sample_driver.c:
		sample code for driver writers.

 If you don't like having firmware_sample_driver.c there, it could be
 moved to Documentation/ or put in some web page.

 Thanks

 	Manuel

 
-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Patrick Mochel]

>  Actually it is two files:
> 
>  	firmware_class.c:
> 		the code itself
> 	firmware_sample_driver.c:
> 		sample code for driver writers.
> 
>  If you don't like having firmware_sample_driver.c there, it could be
>  moved to Documentation/ or put in some web page.

I'd prefer that personally, with a comment on the location in 
firmware_class.c.

Thanks,


	-pat

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

On Thu, May 22, 2003 at 09:51:36AM -0700, Patrick Mochel wrote:
> 
> >  Actually it is two files:
> > 
> >  	firmware_class.c:
> > 		the code itself
> > 	firmware_sample_driver.c:
> > 		sample code for driver writers.
> > 
> >  If you don't like having firmware_sample_driver.c there, it could be
> >  moved to Documentation/ or put in some web page.
> 
> I'd prefer that personally, with a comment on the location in 
> firmware_class.c.

 You mean moving it to Documentation or puting it in some web page?

 Just ignore the sample then.

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Patrick Mochel]

>  You mean moving it to Documentation or puting it in some web page?

Either one. 


	-pat

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Simon Kelley]

Works great for me now.

It's noisy: I'd remove/disable the printks in firmware_data_read and firmware_data_write
before it goes in.

Cheers,

Simon.

Re: [PATCH] Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

On Thu, May 22, 2003 at 06:57:30PM +0100, Simon Kelley wrote:
> Works great for me now.
> 
> It's noisy: I'd remove/disable the printks in firmware_data_read and 
> firmware_data_write
> before it goes in.

 I'd rather not resend it all again.
 
 Attached goes an incremental patch just in case, but I don't mind
 resending it later, once the bulk of it goes in.

 Have a nice day

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

[-- Attachment #2: revised-noise.diff --]
[-- Type: text/plain, Size: 728 bytes --]

--- drivers/base/firmware_class.c	2003/05/22 14:49:44	1.14
+++ drivers/base/firmware_class.c	2003/05/22 18:07:38
@@ -152,8 +152,6 @@
 	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
 	struct firmware *fw = fw_priv->fw;
 
-	printk("%s: count:%d offset:%lld\n", __FUNCTION__, count, offset);
-
 	if (offset > fw->size)
 		return 0;
 	if (offset + count > fw->size)
@@ -204,12 +202,9 @@
 	struct firmware *fw = fw_priv->fw;
 	int retval;
 
-	printk("%s: count:%d offset:%lld\n", __FUNCTION__, count, offset);
 	retval = fw_realloc_buffer(fw_priv, offset + count);
-	if (retval) {
-		printk("%s: retval:%d\n", __FUNCTION__, retval);
+	if (retval)
 		return retval;
-	}
 
 	memcpy(fw->data + offset, buffer, count);
 

Re: request_firmware() hotplug interface, third round and a halve

[Manuel Estrada Sainz]

On Sun, May 18, 2003 at 08:14:06PM +0400, Alexey Mahotkin wrote:
> 
> Two comments on firmware_class_hotplug(), maybe both are irrelevant.
> 
> +int firmware_class_hotplug(struct class_device *class_dev, char **envp,
> +			   int num_envp, char *buffer, int buffer_size)
> +{
> +	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
> +	int i=0;
> +	char *scratch=buffer;
> +
> +	if (buffer_size < (FIRMWARE_NAME_MAX+10))
> +		return -ENOMEM;
> +
> +	envp [i++] = scratch;
> +	scratch += sprintf(scratch, "FIRMWARE=%s", fw_priv->fw_id) + 1;
> +	return 0;
> +}
> +
> 
> First, I do not understand how the environment is handled here. 

 You can take a look at lib/kobject.c:kset_hotplug()

> You're just setting first element of provided environment to
> "FIRMWARE=%s", possibly overwriting the existing value.

 envp points to the first empty slot of the environment being
 constructed, so no, I am not overwriting anything.

> Then why are you incrementing `i'?  Why are you using `i' at all?  Why
> are you incrementing `scratch'?

 The code is ready to add a new environment variable if needed. If we
 really don't need more variables, it could be simplified, but I don't
 think it is so important.
 
> Ah, it seems like you should be using num_envp somehow, and you're not.
 
 OK, I just added:
	if (num_envp < 1)
		return -ENOMEM;

> Also, environment pointer list must be terminated with a NULL pointer. Is
> it not done or is that handled somewhere else?

 The envp array we get is reset to zero, so anything we don't explicitly
 set is already NULL.

> The machine I have 2.5.69 sources is not reachable now so I cannot
> check it.  Sorry if I am wrong.

 The num_envp issue was real, and anyway, I appreciate a little
 feedback, it seams like people don't hack the kernel on Sunday :-P

 Thanks

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Re: request_firmware() hotplug interface, third round and a halve

[Alexey Mahotkin]

Two comments on firmware_class_hotplug(), maybe both are irrelevant.

+int firmware_class_hotplug(struct class_device *class_dev, char **envp,
+			   int num_envp, char *buffer, int buffer_size)
+{
+	struct firmware_priv *fw_priv = class_get_devdata(class_dev);
+	int i=0;
+	char *scratch=buffer;
+
+	if (buffer_size < (FIRMWARE_NAME_MAX+10))
+		return -ENOMEM;
+
+	envp [i++] = scratch;
+	scratch += sprintf(scratch, "FIRMWARE=%s", fw_priv->fw_id) + 1;
+	return 0;
+}
+

First, I do not understand how the environment is handled here.  You're
just setting first element of provided environment to "FIRMWARE=%s",
possibly overwriting the existing value.  Then why are you incrementing
`i'?  Why are you using `i' at all?  Why are you incrementing `scratch'?

Ah, it seems like you should be using num_envp somehow, and you're not.

Also, environment pointer list must be terminated with a NULL pointer. Is
it not done or is that handled somewhere else?  The machine I have 2.5.69
sources is not reachable now so I cannot check it.  Sorry if I am wrong.

--alexm