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.
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
This archive was generated by hypermail 2b29 : Fri May 23 2003 - 22:00:49 EST