request_firmware() hotplug interface, third round and a halve

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

diff --exclude=CVS -urN linux-2.5.orig/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
@@ -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/include/linux/firmware.h linux-2.5.mine/include/linux/firmware.h
@@ -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

diff --exclude=CVS -urN linux-2.5.orig/fs/sysfs/bin.c linux-2.5.mine/fs/sysfs/bin.c
@@ -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;
 

diff --exclude=CVS -urN linux-2.5.orig/fs/sysfs/inode.c linux-2.5.mine/fs/sysfs/inode.c
@@ -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;

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

Nice.

Looks sane.

Thanks.

Is this really needed? Especially as you now have:

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:

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
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Done, I just sent updated patches.

Thanks

Manuel

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

[snip]

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.

Done.

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".

OK, I also removed equivalent macros from class.c

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.

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.

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

-ENOPATCHES :)

[snip]

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.

Done.

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".

OK, I also removed equivalent macros from class.c

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.

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.

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

diff -u linux-2.5.mine/drivers/base/Makefile linux-2.5.mine/drivers/base/Makefile
@@ -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:

@@ -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:

@@ -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:

@@ -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:

@@ -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:

@@ -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:

@@ -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_sample_firmware_class.c linux-2.5.mine/drivers/base/firmware_sample_firmware_class.c
@@ -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)

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.

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.

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

Done. I am not a good documentation writer, but the attached patches should include enough documentation to understand how things work.

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]

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

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?

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

diff -u linux-2.5.mine/drivers/base/class.c linux-2.5.mine/drivers/base/class.c
@@ -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:

@@ -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
@@ -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
@@ -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/include/linux/firmware.h linux-2.5.mine/include/linux/firmware.h
@@ -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
@@ -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
@@ -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

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

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

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

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

diff --exclude=CVS -urN linux-2.5.orig/drivers/base/base.h linux-2.5.mine/drivers/base/base.h
@@ -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
@@ -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)

diff --exclude=CVS -urN linux-2.5.orig/drivers/base/class.c linux-2.5.mine/drivers/base/class.c
@@ -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
@@ -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 *

Thanks.

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

-pat

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

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

Thanks,

-pat

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

Just ignore the sample then.

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

Either one.

-pat

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.

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.

@@ -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);
 

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

Yep, that is perfect.

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 <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
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.