request_firmware() hotplug interface, third round.

Hi all,

This time, as Greg suggested, it is implemented on top of 'struct class' and 'struct class_device' but the driver interface is the same as last time.

Attached:
firmware.h
firmware_class.c:
The firmware support itself.
firmware_sample_driver.c:
Sample code on how to use from drivers.
hotplug:
A simple hotplug replacement for testing.
Makefile:
The obvious.
README:
Still pertinent pieces from the previous round.

How it works:
- Driver calls request_firmware()
- 'hotplug firmware' gets called with ACCTION=add
- /sysfs/class/firmware/dev_name/{data,loading} show up.

- echo 1 > /sysfs/class/firmware/dev_name/loading
- cat whatever_fw > /sysfs/class/firmware/dev_name/data
- echo 0 > /sysfs/class/firmware/dev_name/loading

- The call to request_firmware() returns with the firmware in a memory buffer and the driver can finish loading.
- Driver loads the firmware.
- Driver calls release_firmware().

Notes:
- Drivers can use request_firmware() or register their own 'firmware' class_device and implement the same interface for themselfs. If this code has better acceptance I'll also provide a sample of the later, and make it actually possible.

- There is a race, the hotplug event gets generated on class_device_register and before 'data' and 'loading' creation takes place. Maybe the hotplug support could wait for 'loading' to show up.

- register_firmware can not be implemented without some form of in-kernel firmware caching. And that is not implemented.

- fwfs could be used for firmware caching behind the scene allowing register_firmware to be implemented and the other uses. I could call it blobfs and make a subdirectory within for firmware purposes.

Hope you like it

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.

#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,
		      const char *device);

/* 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

#include <linux/device.h>
#include <linux/module.h>
#include <linux/init.h>
#include "firmware.h"

MODULE_AUTHOR("Manuel Estrada Sainz <[email protected]>"); MODULE_DESCRIPTION("Hotplug Firmware Loading Support"); MODULE_LICENSE("GPL");

#define MAX( a, b ) ( ( ( a ) > ( b ) ) ? ( a ) : ( b ) )

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;
	u8 loading;
	int alloc_size; 
};

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;
}

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)
{
	int loading = simple_strtol(buf, NULL, 10);
	struct firmware_priv *fw_priv = class_get_devdata(class_dev);

	if(fw_priv->loading && !loading)
		complete(&fw_priv->completion);
	fw_priv->loading = loading;
	return count;
}
CLASS_DEVICE_ATTR(loading, 0644,
		  firmware_loading_show, firmware_loading_store);

static ssize_t firmware_data_read(struct kobject *kobj,
struct sysfs_bin_buffer *buffer)

{
	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: size:%d count:%d offset:%lld\n", __FUNCTION__, 
	       buffer->size, buffer->count, buffer->offset);

	buffer->data = kmalloc(fw->size, GFP_KERNEL);
	if(!buffer->data)
		return -ENOMEM;
	buffer->size = fw->size;
	memcpy(buffer->data, fw->data, fw->size);
	return buffer->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 */
		kfree(fw_priv->fw->data);
		fw_priv->fw->data=NULL;
		fw_priv->fw->size=0;
		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,
				   struct sysfs_bin_buffer *buffer)
{
	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: size:%d count:%d offset:%lld\n", __FUNCTION__, 
	       buffer->size, buffer->count, buffer->offset);
	retval = fw_realloc_buffer(fw_priv, buffer->offset+buffer->count);
	if(retval)
		return retval;

	memcpy(fw->data+buffer->offset, buffer->data, buffer->count);

	buffer->offset += buffer->count;
	buffer->size = MAX(buffer->offset, buffer->size);
	fw->size=buffer->size;
	return buffer->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 int fw_setup_class_device(struct class_device *class_dev,
struct firmware_priv *fw_priv, const char *fw_name, const char *dev_name)

{
	int retval = 0;

	memset(fw_priv, 0, sizeof(*fw_priv));

	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';

	strncpy(class_dev->class_id, dev_name, BUS_ID_SIZE);
	class_dev->class_id[BUS_ID_SIZE-1] = '\0';

	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 out;
	}

	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);
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,
const char *device)
/* Maybe 'device' should be 'struct device *' */

{
	static struct class_device class_dev = {
		.class          = &firmware_class,
	};
	struct firmware_priv fw_priv;
	int retval;

	if(!firmware){
		retval = -EINVAL;
		goto out;
	}
	*firmware=NULL;

	retval = fw_setup_class_device(&class_dev, &fw_priv,
				       name, device);
	if(retval)
		goto out;

	wait_for_completion(&fw_priv.completion);

	fw_remove_class_device(&class_dev);

	if(fw_priv.fw->size)
		*firmware = fw_priv.fw;
	else {
		retval = -ENOENT;
		kfree(fw_priv.fw->data);
		kfree(fw_priv.fw);
	}
out:
	return retval;
}
void release_firmware (const struct firmware *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 */
}

static int __init firmware_class_init(void)

{
	int error;
        error = class_register(&firmware_class);
        if (error) {
		printk(KERN_ERR "class_register failed\n");
	}

        return error;

}
static void __exit firmware_class_exit(void)

{
	class_unregister(&firmware_class);
}

module_init(firmware_class_init);
module_exit(firmware_class_exit);

EXPORT_SYMBOL(release_firmware);
EXPORT_SYMBOL(request_firmware);
EXPORT_SYMBOL(register_firmware);
EXPORT_SYMBOL(firmware_class);

#!/bin/dash
sleep 1
OUTPUT=/dev/console
exec > $OUTPUT 2> $OUTPUT

TYPE="$1"
echo "TYPE: $TYPE"
if [ "$TYPE" != "firmware" ]; then
echo wrong param
exit 1
fi
if [ "$ACTION" = "add" ]; then
echo 1 > /sysfs/$DEVPATH/loading
echo "Hi there" > /sysfs/$DEVPATH/data
echo 0 > /sysfs/$DEVPATH/loading
ls -lR /sysfs/$DEVPATH
fi
set

request_firmware() hotplug interface:
------------------------------------

Why:
---

Today, the most extended way to use firmware in the Linux kernel is linking it statically in a header file. Which has political and technical issues:

1) Some firmware is not legal to redistribute.
2) The firmware occupies memory permanently, even though it often is just used once.
3) Some people, like the Debian crowd, don't consider some firmware free enough and remove entire drivers (e.g.: keyspan).

Notes:
-----

- Why OPTIONALLY caching the firmware in-kernel may be a good idea sometimes:

- If the device that needs the firmware is needed to access the filesystem. When upon some error the device has to be reset and the firmware reloaded, it won't be possible to get it from userspace. e.g.:
- A diskless client with a network card that needs firmware.
- The filesystem is stored in a disk behind an scsi device that needs firmware.
- On embedded systems (like install floppies) where there is no userspace hotplug support, 'cp firmware_file /firmware/' can be handy.

And the same device can be needed to access the filesystem or not depending on the setup, so I think that the choice on what firmware to cache should be left to userspace.

- Why register_firmware()+__init can be useful:
- For boot devices needing firmware.
- To make the transition easier:
The firmware can be declared __init and register_firmware() called on module_init. Then the firmware is warranted to be there even if "firmware hotplug userspace" is not there jet or it doesn't jet provide the needed firmware.
Once the firmware is widely available in userspace, it can be removed from the kernel. Or made optional (CONFIG_.*_FIRMWARE).

In either case, if firmware hotplug support is there, it can move the firmware out of kernel memory into the real filesystem for later usage (like the provided hotplug scripts do).

# Taken from orinoco-0.13d
VERSION = 0.13d

TOPDIR = $(shell pwd)
KERNEL_VERSION = $(shell uname -r) #KERNEL_SRC = /lib/modules/$(KERNEL_VERSION)/build KERNEL_SRC = /storage/pub/src/kernel/bk-cvs/linux-2.5

# if Rules.make exists in the kernel tree, we assume 2.4 style modules
# if it doesn't assume 2.5 style
OLDMAKE = $(wildcard $(KERNEL_SRC)/Rules.make)

MODULES = firmware_class.o firmware_sample_driver.o

all: modules

clean:

	rm -f core *.o *~ a.out *.d
	rm -f *.s *.i userhermes bufparse
	rm -f *.ko *.mod.c .*.o.cmd

ifeq (,$(OLDMAKE))
# 2.5 style modules, get the kernel makefiles to do the work
KBUILD_VERBOSE = 0 # default

obj-m := $(MODULES)

modules:

	$(MAKE) -C $(KERNEL_SRC) SUBDIRS=$(TOPDIR) KBUILD_VERBOSE=$(KBUILD_VERBOSE) modules

install: all

	$(MAKE) -C $(KERNEL_SRC) SUBDIRS=$(TOPDIR) KBUILD_VERBOSE=$(KBUILD_VERBOSE) modules_install

else
# 2.4 style modules
KERNEL_HEADERS = -I$(KERNEL_SRC)/include

MODULE_DIR_TOP = /lib/modules/$(KERNEL_VERSION) MODULE_DIR_WIRELESS = $(MODULE_DIR_TOP)/kernel/drivers/net/wireless

CPPFLAGS = -D__KERNEL__ -DPCMCIA_DEBUG=1 \
	-DMODULE -DEXPORT_SYMTAB \
	$(PCMCIA_HEADERS) $(KERNEL_HEADERS)

CFLAGS = -O2 -Wall -Wstrict-prototypes -fno-common -pipe $(EXTRACFLAGS)

MODVER = $(shell if cat $(KERNEL_SRC)/include/linux/autoconf.h 2>/dev/null | \
grep -q '^[[:space:]]*\#define[[:space:]]*CONFIG_MODVERSIONS[[:space:]]*1'; \
then echo 1; else echo 0; fi)

ifeq ($(MODVER),1)
MFLAG = -DMODVERSIONS -include $(KERNEL_SRC)/include/linux/modversions.h endif

modules: $(MODULES)

%.o: %.c

	$(CC) -MD $(CFLAGS) $(CPPFLAGS) $(MFLAG) -c $<

%.s: %.c

	$(CC) -MD $(CFLAGS) $(CPPFLAGS) -S $<

%.i: %.c

	$(CC) -MD $(CPPFLAGS) -E $< -o $@

endif

So, if I understand you correctly, RAM is only saved if a device is hotpluggable and needs firmware only upon intial connection. Which, if you do suspend to disk correctly, is no device.

And do I understand you correctly, you propose that request_firmware() wait for the hotplug script to write the firmware to sysfs? That means that request_firmware() is unusuable from the usual probe() methods. You cannot kill a part of the kernel if a script fails to perform correctly for some reason. Even worse, you cannot detect the script terminating abnormally in that design. You'd have to introduce some arbitrary timeout.

It seems to me that you introduce three new problems to get rid of one old problem.

Regards
Oliver

Hotpluggability is not required, it is the same for any module, which gets loaded while the system is running. Drivers don't even need to be aware of hotplug.

And adding some kind of persistence in the mixture so firmware can be included in the kernel image and later discarded/reconsidered even in-kernel drivers (meaning non modules) can benefit. Coordinating with initramfs as Pavel suggested should bring best results in this case.

Also, the hotplug event happens every time you call request_firmware(), not just on device load or upon initial connection. It is not the regular "device plug event" it is an special 'firmware' event. For example, on usb devices you would get two invocations of hotplug, one 'hotplug usb' and one 'hotplug firmware'.

In the case of suspending to disk, you would have to make sure that the firmware for the device that holds the rest of the firmware is already in fwfs or whatever persistence method gets finally implemented.

Yes.

At least usb's probe() can sleep, but that is a good point. How about:

int request_firmware_nowait (
const char *name, const char *device, void *context, void (*cont)(const struct firmware *fw, void context)
);

Then you can call request_firmware_nowait providing an appropriate 'cont' callback and 'context' pointer. Then when your callback gets called with the firmware you finish device setup.

Good point. Since it is easily solvable by hand:

echo 1 > /sysfs/class/firmware/dev_name/loading
echo 0 > /sysfs/class/firmware/dev_name/loading

I thought that it was OK. (I'll do the timeout)

Well, the device model doesn't provide that information :(

It would be great if it did.

Would a patch to wait for hotplug termination and provide termination status be accepted?

Adding an 'struct completion' and 'int status' to the right place should be just about it.

OK, I'll do that for now.

This is the kind of feedback I wanted, thanks a lot.

Let's see if I can remove all four problems now :)

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

Am Freitag, 16. Mai 2003 11:56 schrieb Manuel Estrada Sainz:

In that case they can contain the firmware and mark it __init. They need no RAM. They can even mark the code needed to put firmware into the device as __init.

How and what is the benefit? If you go low on battery you have to suspend, there's no choice. This means that you have to have it in RAM always.

In this form unworkable. Removing a module could kill the machine. That scheme requires that drivers formally register and unregister with fwfs and provide module pointers.
An awful lot of overhead.

No, it isn't. These writes must require CAP_HARDWARE, thus is no good.

No, you must not wait for user space.

Sorry.

Regards
Oliver

There are issues with that approach:

- Code with firmware will not be accepted in the kernel, there are already two drivers that are being hold back for that reason.
- Licensing issues.
- Memory consumption.
- "new" kernel policy.
- A device may need to reload firmware upon reset. And if you mark it __init, it will not be there.
- If you include the firmware in the code you have to recompile the kernel to update the firmware. Pavel, back me up on this one with the ACPI tables issue.

If the device losses the firmware upon suspend, the driver will have to reinitialize it as if it just got plugged, which somehow makes all devices hotplugable.

If the driver uses request_firmware(), it doesn't need to handle any special case, just initialize as usual and it will get the firmware when it needs it.

If the device is needed to access the filesystem, some kind of persistence will be needed, so the required firmware is already in kernel space. But the driver doesn't need to care, it will just ask for the firmware as usual.

Which brings me to another issue, the same device can be required to access the filesystem or not:
- In a diskless client, it is the network card
- In a live-cd it is the cdrom drive
- In a multi disk system just one of them will be holding required firmware.
So you can not decide at coding time, the latest at compile time, and ideally at runtime (which is what I am trying to do).

OK, how about:

int request_firmware_nowait (
struct module *module,
const char *name, const char *device, void *context, void (*cont)(const struct firmware *fw, void context)
);

request_firmware code will try_module_get/module_put as needed.

Would that fix the issue?

I'll do the timeout anyway, and make it configurable just in case.

And to get notified when userspace is done?

Don't be, this is still the kind of feedback I want. Weather I manage or not, I get the chance to fix the issues.

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.

So all firmware has to be permanently in RAM anyway?

How or precisely, how do you know that it gets it when it needs it? Are you planning to have a gray area where the kernel generates a special user space before everything else gets woken?

How? You cannot page out memory during resumption. You must not cause any access to disk during resumption.

Not with that interface.
You'd need drivers to register both with their subsystem and a firmware subsystem. But you cannot make device discovery wait for user space.
You'd have to rewrite the probe method of all drivers that need firmware.

Regards
Oliver

If you really can't access the filesystem on wakeup, you could pull the required firmware into ram upon suspend and remove it after wakeup. Then while the system is running it will not use kernel memory.

For this I proposed fwfs (a kernel friendly filesystem on top of ramfs), but it didn't get a good acceptance. And after reading Greg's recent post initramfs should be able to fill that gap.

You will have to make sure that the firmware is copied into RAM before suspension, either via fwfs, initramfs or whatever persistence method gets implemented.

The driver will not unload if request_firmware_nowait() has called try_module_get() on it.

But the device could get disconnected and in that case the firmware load should be canceled. I'll add request_firmware_cancel().

Why?, the current interface already provides termination notification.

Why not?

And anyway, request_firmware_nowait() could be used if that is an issue.

Keep in mind that the "firmware in a header" method is not allowed any more. Drivers needing firmware will have to get their probe method modified to get the firmware from userspace anyway.

Using request_firmware() if they can sleep (USB probe callbacks can sleep) is trivial, and using request_firmware_nowait() if they can't sleep is just a matter of splitting probe in two pieces.

So, what alternative are you proposing to get the firmware?

If your proposal is just keeping the firmware in a header so you just have it there all the time, you should discuss it with Alan, Greg, Jeff, et all, not me.

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.

A good bit of this is moot.

All updateable devices fall into one of three categories:

1) "Flashable", they will remember the image data until that image data is displaced by another image. (we are generally not talking about this kind of device here, but they deserve a nod for completeness.)

2) "Loadable && Suspendable", such a device will retain its firmware during a suspend, either because it will not actually be powered off, or because of some "magical" means (like a battery backup on-board activated by a please-suspend yourself call.) or, if it doesn't retain its whole firmware, it will at least retain enough internal state that it can pick up where it left off when the firmware is reloaded. (...which actually puts it mostly in the next case if the firmware comes from the kernel side as opposed to the device side...)

3) "Reload Only", such a device will lose its firmware and internal state completely when the device is suspended.

Given this simple reality, programs in session with the (almost exclusively existent) type three devices are simply going to lose their state across a suspend/resume. This is just like suspending your laptop with a telnet session open. When the resume happens, the telnet session will still be there, but the first time you use it, or it uses itself, it will see the socket is gone and eat itself.

Moving into the twilight zone of theoretical analysis.... (it's ok to stop reading here, the below is NOT cannon by any means, and is actually a little vague 8-)

Type one devices will do whatever they do.

In the case of type two devices, the magic will magically keep you covered.

If no programs are using the device, regardless of type, then there is no hurry regardless of intent.

Ok, that sounded a little fatuous, but frankly, any firmware-loadable device is going to come up out-of-state. Period. Definition of the box. Loading the firmware will bring it into state, sure. But the driver is going to have to be able to recognize the out-of-stateness of the device until after the load happens no matter what. Anything else is an oops waiting to happen.

So the person who writes the driver for a reloadable device will already have to have a way to park the driver until the firmware is loaded. That being said you now have an "infinite" amount of time to load the driver, and you, by definition, *can* now wait for enough user-space initialization to take place to handle the issues for you because all access will be blocked within the driver and the programs dependent on that driver will be hung waiting.

Remember that all your type-3 firmware-load-targets are small computers that, by definition, can't be suspended, if they suspended themselves they'd be type two, so "getting all in a hurry" to reboot them during system resume is valueless.

The only time this *couldn't* be the case is if you actually "suspend to" something behind a type-three device. *That* could only be resolved by keeping the firmware image in preserved ram. That would also be a very dumb thing to do for these self-same reasons.

Let's face it, the Type-3 devices are hideously peripheral anyway. Mouse touch-pads, serial dongles, second tier hubs, Ethernet cards that have already dropped all their connections and will have to be re-bound into the stack anyway. etc.

Firmware-able devices are slow external devices, with wait-states and protocols and firmware-loading interfaces. As the system comes to life the things created by the logic in the firmware won't exist to be interacted with until the firmware is (reloaded. (by definition there, proof by induction omitted for brevity... 8-). That says outright that you have a lot of dependency-order tasking to do during resume, which in turn means that a driver that doesn't park itself in stages, and which isn't operating in a context where waits and yields are ok (say user context or kernel-thread context), will either be NP complete with respect to its dependencies on other devices in the system, or it will crash and burn "sometimes" "for no apparent reason". So any such device, to be both firmware-loadable and resumable, will need to be designed to resume only after user context is available.

Once the resumeable firmware-load-safe driver has been constructed for the device, you can easily wait for user context. Any attempt to have a driver without that park-and-wait feature will likely oops anyway when it wanders naively onto the CPU and access features that don't exist yet.

Sadly, each such loadable device will suffer under the burden of the unique demands of its design, so it will be up to the driver writer for each device do meet those demands. Building unknown protections for as-yet non-existent devices into the kernel is not practical.

If you disagree, look again. Even something as simple as a serial port UART will need to have its baud-rate latch restored. If that serial port UART is an abstraction that exists solely because of a firmware image (Which is how FPGAs work for example) then if the driver for the serial port "wakes up" and tries to reload that divisor before the driver for the firmware load, you will oops, or at least hang or not ever really restore the state of the logical UART.

The furthest the kernel can go "safely and reasonably" to deal with these issues is (in vague abstract-o-vision theoretical terms):

1) Create a mid-transaction layer where the kernel is "just about" to call the file_ops structure members.

2) Define a well defined signal/means to wake a driver and have it go above that layer a la return -ERESTARTSYS and guarantee that this signal will be sent to every thread-of-control before a resume

3) Strongly urge device driver programmers to use and obey Number 2.

4) Provide a latch in number 1 that "parks" the request before being restarted, said latch to be set to park before the signal in number 2 is sent.

5) Add an optional driver entry point and callback interface that will let a driver control the un-park of its own operations, and check the parkedness of other drivers (for those cases when dependencies exist and can be determined before sleeping or as waking)

5a) Add a well defined all-bets-are-off interface that will error out some/all of calls on some/all of the parked threads when/if things become recognizably hopeless. (Say the USB serial port was unplugged, and not plugged back in, during the sleep.)

6) Provide a kernel thread (or threads) to give an independent user-context to the call-ins created in number 5.

(and so forth)

Some programmers will keep/move their firmware in/into ram. Some will arrange to allow for some user-space utility to reload the firmware (which is really for the best). Some will let the resume oops. Expecting a generic kernel mechanism to do the reload instead is paying the price of bloat for one throw at a constantly moving target.

===

So now, many drivers resume themselves because they are just data based abstractions. (see the entire VFS 8-)

Lots of drivers wake up, reinitialize their devices, and then carry on because their state was internally persistent. (see any block device beneath the VFS 8-)

Once the block devices are alive again, you can restart the virtual memory system. Once the VM system is up you can use paged memory for your resume data (firmware) anyway.

One quick non-blocking pass ought to be made through the Number-5's of the system to let the first order dependencies disappear.

It is now safe to wake the user contexts.

The theorized special kernel thread(s), which are user contexts, just keep calling the resume tasklets until the list is empty. From that vantage, just about any user-space program (a-la hotplug) can be called at leisure.

Any driver that expects to resume itself before the block devices (and virtual memory system) are ready bares the onus on itself to make sure it will be able to before things go to sleep. The only case where this actually has any point, is the case where the firmwareable device is a block device with a swap file/partition. And such a block device is evil anyway.

Wow, two rants in one night... 8-)

Rob.

-----Original Message-----
From: [email protected] [mailto:[email protected]]On Behalf Of Manuel Estrada Sainz
Sent: Friday, May 16, 2003 5:59 PM
To: Oliver Neukum
Cc: LKML; Simon Kelley; Alan Cox; Downing, Thomas; Greg KH; [email protected]; Pavel Roskin
Subject: Re: request_firmware() hotplug interface, third round.

If you really can't access the filesystem on wakeup, you could pull the required firmware into ram upon suspend and remove it after wakeup. Then while the system is running it will not use kernel memory.

For this I proposed fwfs (a kernel friendly filesystem on top of ramfs), but it didn't get a good acceptance. And after reading Greg's recent post initramfs should be able to fill that gap.

You will have to make sure that the firmware is copied into RAM before suspension, either via fwfs, initramfs or whatever persistence method gets implemented.

The driver will not unload if request_firmware_nowait() has called try_module_get() on it.

But the device could get disconnected and in that case the firmware load should be canceled. I'll add request_firmware_cancel().

termination

Why?, the current interface already provides termination notification.

Why not?

And anyway, request_firmware_nowait() could be used if that is an issue.

Keep in mind that the "firmware in a header" method is not allowed any more. Drivers needing firmware will have to get their probe method modified to get the firmware from userspace anyway.

Using request_firmware() if they can sleep (USB probe callbacks can sleep) is trivial, and using request_firmware_nowait() if they can't sleep is just a matter of splitting probe in two pieces.

So, what alternative are you proposing to get the firmware?

If your proposal is just keeping the firmware in a header so you just have it there all the time, you should discuss it with Alan, Greg, Jeff, et all, not me.

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Of course not you can just go back out to user space and ask for it

Already insoluble because of routing daemons.

Only if you program it wrongly. Its not exactly hard.

As to an interface. The simplest is probably

request_firmware()
and
request_firmware_nowait(......, workqueuehandler)

The issues brought up about it failing appear bogus too, if the hotplug run returns a non zero exit code you know about this already.

Alan

On Sat, May 17, 2003 at 02:23:32PM +0100, Alan Cox wrote:
[snip]

This is what I plan to use:

int request_firmware_nowait (
struct module *module,
const char *name, const char *device, void *context, void (*cont)(const struct firmware *fw, void *context));

Working code should be available later today.

In this case, hotplug gets called automatically by the device model, and there is no way to get the return value of the hotplug run.

If I get any good feedback on it, I'll try to make that return value available, in the mean while, I already implemented a timeout.

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.

I think you missed a lot of the previous discussion. Many high
level kernel developpers (Jeff, Alan, Greg) have said 'niet' to binary firmware linked with the kernel (unless there is source code available). Please assume that all drivers currently including firmware blobs in the kernel will need fixing and therefore should not be taken as example.

	The threads :
		http://marc.theaimsgroup.com/?t=105222131600002&r=1&w=2
		http://marc.theaimsgroup.com/?t=105045487300002&r=1&w=2
		http://marc.theaimsgroup.com/?t=105181861200001&r=1&w=2
		http://marc.theaimsgroup.com/?t=105295022700002&r=1&w=2
		http://marc.theaimsgroup.com/?t=105302951900003&r=1&w=2
	Yes, that's a lot to read, but this will explain the full

complexity of the situation.

It's not like we have a choice on this issue. The embedded
people are already grumbling.

Jean

Am Freitag, 16. Mai 2003 20:49 schrieb Jean Tourrilhes:

It seems to be a whole lot of laywering not technical discussion IMHO.

Please explain. Either firmware is in RAM or it is not.

Regards
Oliver

Excuse me, but I never said such a thing. I don't mind putting firmware blobs into kernel drivers, been doing it for years :)

I just stated that _if_ we want to move firmware to userspace, why not have a consistant interface that all kinds of drivers can use to accomplish this. That is what has been done with this code.

thanks,

greg k-h

Thats just because the interface is a little warped not the theory. On a resume you need to reload firmware and you already handle rediscovery on USB bus for example because the devices can change

Am Freitag, 16. Mai 2003 18:09 schrieb Alan Cox:

Right. But the order of resumption is fixed by hardware needs. So during resumption you cannot use block devices and therefore not start a hotplug script. Or did I miss something?

Regards
Oliver

For devices that aren't essentialy to get userspace running (e.g. network on a laptop running from local disk), the firmware request doesn't have to happen during the hairy part of resumption.

The device could just mark itself as unusable at suspend time, then at resume it schedule_work()s something to reload the firmware and complete reinitialization. Shortly after userspace is back in action, the device will come back to life.

David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Am Samstag, 17. Mai 2003 06:50 schrieb David Gibson:

Not true for network cards. Somebody might be running NFS over it. The problem is that you cannot tell (or rather shouldn't - it's a layering violation).

Anything that is used for paging needs to be back to life before you can think about resurrecting user space.
Also you need to bring keyboards back to life early to make sysreq work.

Secondly, you need a way to get essential devices to work in all cases. If you implement it, why not use it?

Is supposed to. You cannot put blind trust into that. You need to use a pretty arbitrary timeout to deal with this.

I fail to see technical improvements here.

Regards
Oliver

Potentially, yes, usually no (hence "running from local disk" above). This is why Manuel wants to make persistence runtime selectable.

It may well be a bit messy in the scripts to make sure all the right drivers have persistent firmwares, and I'm sure there will be some funny corner cases, but I don't think it's an insoluble problem.

If you have to be able to reinitialize any device from suspend without userland running, then clearly there is no way around having all necessary firmware images RAM resident. But there are many situations where that's not necessary - so why should they have to pay for the worst case.

So? Something could go wrong, but things can go wrong in lots of places, I don't see what's so especially dire about this situation?

Improvements over *what*. There is no existing method for pulling firmware images into the kernel.

David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

I was really wondering why MAX/MIN had not being generalized :)

All three fixed.

I don't know that much about initramfs, but how about, firmware images get included in the initramfs and copied into fwfs during early userspace?

I'll call it 'persistence' instead of 'caching' from now on.

And I guess that 'blobfs' would be a much better name than 'fwfs'. Meaning that it can be used to store any blob.

The interface would allow it, but some kind of persistence has to be there to make it possible.

- Rename 'fwfs' into 'blobfs'
- Use it as the persistence mechanism behind request_firmware()
- Use one level of hierarchy within the filesystem:
$BLOBFS_MNT/firmware/...

	$BLOBFS_MNT/crypto_keys/...
	...

I don't know if it would really be useful for crypto keys, but it looked like a good sample :)

request_firmware() would then just search with in $BLOBFS_MNT/firmware/

Regards

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.

initramfs can do this today. It isn't the issue for this firmware interface to solve. Same thing goes for the "resume the sleeping device" arguement. That's not this code's issue.

Hm, don't know the ACPI startup point in reference to when initramfs gets uncompressed. But I think you might be safe.

With initramfs, yes.

Again, with initramfs, yes.

thanks,

greg k-h

Hi all,

Why not doing that in open and require firmware data to contain size information? Good firmware formats contain already size, checksum and version information. Bad firmware can be wrapped to get these. It should be made a requirement to contain at least a size and a checksum.

To handle the big varieties of firmware formats, I would suggest to either wrap all in user space or define 3 functions per firmware format like the seq_file support. The thing is very similiar, except that we read from user space instead of writing.

fw_begin_firmware_store()
fw_next_firmware_bytes()
fw_end_firmware_store()

fw_begin_firmware_store() gets at most a page of data and should
evaluate from this data, how much bytes it still needs from user space. It will also setup a context and store it.

fw_next_firmware_bytes() will get passed more firmware bytes and
tells us again how much it still need. It will get passed the context setup by fw_begin_firmware_store().

This function can also abort a download by returning "no bytes needed anymore" and marking the firmware "invalid" in the context, which fw_end_firmware_store() will use to return "discard this firmware" to the firmware fs.

Also this function is not really necessary, if we set the filesize of the firmware (truncate()) in firmware fs after fw_begin_firmware_store() and let the VFS do its magic.

fw_end_firmware_store() will be called, after user space closed
the file descriptor (Note: This will handle SIGKILL also). It must decide, whether the downloaded firmware is valid and will be stored and can be used or will be discarded. It gets passed the context setup by fw_begin_firmware_store() and will free it's resources, if not needed anymore.

After fw_end_firmware_store(), the firmware can be downloaded to the device (not before!).

This is much simpler, then it sounds. The only problems are:
1. getting the size of the firmware to be downloaded

      a) firmware has always the same size, so this is a constant
      b) firmware has size encoded -> use this 
      c) firmware size is file size -> need to wrap this to be like 1.b)

2. decide, whether the firmware is valid

      a) checksum
      b) versions
      c) none -> trust or wrap to match 2.a) and/or 2.b)

What do you think?

Defining the prototypes and finding the places to hook into is left as an exercise to the reader ;-)

The current idea (special file sytem) is great, but the interface to the driver is not really perfect.

Regards

Ingo Oeser

The reason for such an interface is to be able to do it on top of sysfs and so that drivers can implement the same interface on themselfs.

They just need to export the device's firmware memory as 'data' via a class_device of class firmware_class.

They also need to know when the load starts to set it up and when the load finishes to get the device going again with the new firmware, for that is 'loading' when they get a '1' they have to get ready for the load and when they get a '0' they have to get the device back working.

Doing that, they would get the hotplug event and compatible hotplug support for free. Coping the firmware image in the appropriate directory would be the only userspace setup needed.

Feedback on the interface (data/loading) would also be welcomed, I am probably biased by the hardware I have at hand.

It is currently not possible, but the interface means to allow it.

That could be done, but I am not sure the complexity is worth it. If a privileged user wants to shoot on his foot, he can do so may other ways already.

I think that this is an overkill, the most I can see reasonable is forcing an standard header on all firmware images with size/checksum. And still I am not sure it is worth it.

In which way is it not perfect?

Detailed criticism gives me the chance to try to get things better :)

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.

First off, nice, this looks a lot better, good job.

Can you just send this as a patch to the current kernel next time? It's much easier to read and test with that way :)

I didn't see this in the files you attached.

Yeah, I agree with your comment in the code, I think a struct device * should be passed here. Or at least somewhere...

I don't see why you need to add a new environment variable in your firmware_class_hotplug() call. What is the FIRMWARE variable for, if we already have a device symlink back to the device that is asking for the firmware? Oh, you don't have that :)

If you pass a struct device to request_firmware(), then you get a symlink to the device for free. You can also set the class_id to the device bus_id, watching out for name collisions (bus_ids are only unique per bus type, so different bus types can use the same bus id, but in reality they rarely do.)

Nice, but can't you get rid of the loading file by just relying on open() and close()? Oh wait, sysfs doesn't pass that down to you, hm, looks like you need that info. But does the new binary interface in sysfs that just got merged into the tree provide that info for you?

request_firmware() can't use a static struct class_device, like you have it, in order to work properly for multiple calls to request_firmware() at the same time by different drivers. Just create a new struct class_device, and put it on a list, like I had to do for the tty class code (and i2c_dev class code, but that isn't in the kernel to look at yet...)

Other than those very minor tweaks, I like this interface, it's looking very good. I wouldn't worry about any "checksum" calcuation crud, it's up to the userspace tool dumping the firmware to the kernel to make sure it's writing correct data, not the kernel.

thanks,

greg k-h

Thanks for the positive feedback.

The next one will come in that form.

Sorry, I thought I did, attached now.

To make compatibility with 2.4 kernel easier, I think that I'll add a new 'struct device *' parameter to request_firmware(). On 2.4 kernels it can be an unused 'void *'. Does that sound too ugly?

The same device can ask for different firmware images.

Orinoco USB devices have one image for the USB<->PCMCIA bridge, one secondary firmware for the hermes chipset, and if we managed to get it into master mode tertiary firmware for the hermes chipset. So just knowing which device we have doesn't tell which firmware we need.

I guess that all three images could be packed into a single file. I find the FIRMWARE variable cleaner, but I really wouldn't mind.

OK, sounds good. I'll try that.

Nop. Although since I seam to be the first user of that interface, adding support for that shouldn't break anything.

Sorry, I don't know how that 'static' got there, I just wanted to allocate it on the stack. But I guess that it should be dynamically allocated anyway. Do I really need to put it on a list?

That is what I thought.

Thank you

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.

#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include "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 void sample_firmware_load(char *firmware, int size)

{
	printk("firmware_sample_driver: firmware: %s\n", firmware);
}

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", "ghost0")!=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 */
	printk("firmware_sample_driver: a ghost device got inserted :)\n");

        if(request_firmware(NULL, "sample_driver_fw", "ghost0")!=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 int sample_init(void)
{

#ifdef WE_CAN_NEED_FIRMWARE_BEFORE_USERSPACE_IS_AVAILABLE
	register_firmware("sample_driver_fw", inkernel_firmware,
			  sizeof(inkernel_firmware));
#endif
	/* since there is no real hardware insertion I just call the
	 * sample probe functions here */
	sample_probe_specific();
	sample_probe_default();
	return 0;
}

static void __exit sample_exit(void)
{
}

module_init (sample_init);
module_exit (sample_exit);

MODULE_LICENSE("GPL");

Yeah, don't use void * if you can ever help it. As there will be two different versions for two different kernels, just don't have that paramater, or make it a char * like you have now for 2.4. That seems to make sense for 2.4 where you don't have a struct device.

Ah, that makes more sense now. Ok, I have no problem with it.

If you want to delete it later, you have to have some way to find it again. If you are just adding it and then removing it in the same function, just allocate it dynamically, register it, sleep, and then free it. So then you would not need a list.

thanks,

greg k-h

Given this, would it be better to make the sysfs node name depend on which firmware we're loading - rather than "data" always. I realise we could just require firmware requests for a particular device instance to be serialised, however my instinct says using different nodes would be more robust: it will be easier to figure out what's gone wrong if a script error or a kernel bug has resulted in attempting to load two images at once.

David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

I think that is a pretty good assumption.

It won't be me how loads two different firmwares concurrently to the same device :)

It would also complicate both kernel and userspace code.

For this matter, I could add a readonly 'name' which gives you the same string as $FIRMWARE. That way if something goes wrong you can easily find out which firmware image the kernel is expecting.

This will get caught, because sysfs won't allow two entries with the same name.

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.

But what if the close() is due to irregular termination? If the script is killed, do you download half a firmware?

Regards
Oliver

Good point. Actually I don't think that the binary interface for sysfs passes open and close down to the lower levels, so it's a moot point.

echo... works for me.

thanks,

greg k-h

Howdy,

Not to throw in two-cents you didn't ask for but...

[Quick Disclaimer: The following is extemporaneous thought based on god knows what all, but it *sounds* reasonable to me just now... 8-)]

I have a custom device with a set of FPGAs (Field Programmable Gate Arrays) on it, that have to have their firmware written into them after each power on (etc.)

My solution was rather more direct than those listed below.

Firmware loading, exclusivity, and correctness is best (and quite usefully) thought of as a one-writer-many-readers lock, which can be implemented completely and deterministically in the (various) open routines.

Consider the following two example nodes: (I will not use sysfs specific speak because I am not familiar enough with sysfs to avoid putting noise in my signal)

/devfsnodething/device/user-access
/devfsnodething/device/firmware-programming

if you want to "cat my-firmware-image

open-for-write to succeed if two assertions are met:

1) nobody else is writing the firmware
2) nobody else is already using the user-access point.

There is also a list of "things firmware does to a device" and similar realities that can be deterministically known about any device. This list isn't exhaustive, but it is nearly so. Consider these as candidate flags and entry points suitable for framing in a driver_info like structure of pointers to functions (etc):

FIRMWARE_FLASHABLE == set in any module to indicate that the system should produce/use/honor a firmware-programming device node.

FIRMWARE_FLASH_DEVICE == set on any device_info structure for the access point for loading a firmware image. (User-access points don't have this flag set.)

atomic(FIRMWARE_NEEDS_FLASH) == set in a device that is FLASHABLE and doesn't have a currently valid flash image in place. Acts as the guardians against opens of the user-access device(s).

FIRMWARE_FLASH_WAIT == open of firmware-programming device should block, waiting for a chance to flash the device until other flash writers or user-access users are done. Non-Blocking opens will not block if this is set. Blocking opens will wait if this is set. If this is not set, any other node in the device being open will make the open of the firmware-programming device return EBUSY.

dev_Can_Program() == Called during open of firmware-programming device file, Tells you if the device can be flashed (now|again). Implements checks for re-programmability (do you have to power-off or remove and reinsert device in order to change the firmware, has the firmware already been flashed to the device, etc.) Returns READY (0), EFAIL, EAGAIN (maybe others?). Non ready returns will cause the open to fail. Blocking of the open happens in this routine if it is going to happen at all.

dev_Invalidate_Device() == called during open, or first write operation, to dismantle user-access devices and list the firmware image as incomplete or unstable (etc). If the firmware is being "rewritten" this routine is responsible for clearing the old firmware. It is also responsible for removing user-access devices from the devfs file system and

dev_Check_Program() == Called during close (?) of the firmware-programming device file to validate that the firmware was accepted by the device. Returns OK or not.

dev_Enumerate_Device() == Called during close (?) for the firmware programming device file (or maybe a kernel tasklet etc) after dev_Check_Program() to give the driver a chance to create the user-access nodes. (This is useful, for instance, when the firmware will "create" within the device, a variable set of user access points.

So, the above covers almost all variations of the cases where a device needs/may want a firmware file via different combinations of the flags and routines. (I am sure I may have missed some cases, but you get the point.) It does get rid of the whole "echo 1 >" nonsense as a separate action. (which is good, because the non-script versions of such activities gets annoying, and you don't want to activate a device with detectably bad firmware image in it under any circumstances.)

So What Rob Would Do(tm) (all rights [to the phrase] reserved) 8-):

1) Define a firmware_ops structure with the necessary entry points and data items in it.

2) Add a (normally null) pointer to same to the sysfs_thingy, file_ops, device_info, and module_info structures. (I really haven't thought about where this pointer will go.)

3) Consider any sysfs/device/module/file_info with a pointer to the same instance of this structure as under the exclusive-open (readers/writer) protections (giving you a nice, safe, and universally detectable place to deal with status inquiries, locks, and whatnot for the devices, of the "oh look, that pointer is non-null, we need to be careful" variety.] (So only one FIRMWARE_FLASH_DEVICE with a particular firmware_ops structure can be opened at one time. A non FIRMWARE_FLASH_DEVICE may not be opened if a non FIRMWARE_FLASH_DEVICE is opened which has the same firmware_info pointer. As many non FIRMWARE_FLASH_DEVICES as you choose may be opened at one time. (hence the readers/writer like interface)

4) Take a step back, realize that this is all really a "lets make a meta-device coordinator" pointer/dependency-system and see if the work has already been (mostly?) done elsewhere, or if the consideration should be broadened just a hair beyond actual firmware.

=====

What I have done to "fake all this" on my little embedded box using only pre-hotplug-support facilities:

/etc/modules.conf:
post-install cpld /opt/casabyte/bin/do_flash
above tequal0 cpld

/opt/casabyte/bin/do_flash:
modprobe fpgaflash
cat firmware >/dev/FPGAFlash/ALL
rmmod fpgaflash

The "cpld" (Complex Programable Logic Device) is the thing that lets me "see" the pins on the FPGA(s) and this driver creates and owns the region of memory and IO necessary to both flash the device and operate whatever image is placed into the flash. "Attaching" and "Detaching" from the cpld driver acts as an exclusion primitive (lock) and only one lower-level driver can be attached.

The fpgaflash is a lower level driver that can only flash the FPGAs and has all the checks and such to make sure that any given FPGA is flashed once. It handles all the clearing and whatnot. (Since the cpld can flash all of the FPGAs at once if you want, the driver dynamically creates /dev/FPGAFLASH/1 through 4, and "ALL"). When it is removed, those devices obviously disappear.

The tequal0 driver is the real point, it will fail to load if at least one FPGA hasn't been successfully flashed. It dynamically allocates the devices based on the information it gets from cpld about what is flashed with what.

Now all that is done, with "well designed" drivers, with clear dependencies, and six lines of script/configuration.

This would be directly analogous to just having the flashing of firmware (start and finish) re-invoke the hotplug system as the "character of the flashable device" changes from "dumb thing that needs an image" as one device, and "smart thing ready to serve" exists in sequence.

=====

In short (I know, too late), I think about moving firmware into my FPGAs as a first cousin to formatting a ram-disk-buffer. It is a nearly-deterministic strict-dependency activity that uses one (almost always) character device as a latch that creates/activates or removes/deactivates one-or-more dependent devices.

This is true simply because of the before and after nature of a programmable device.

If you feel you must codify the idea of these dependencies into the kernel (as opposed to the dirver(s) and the(ir) configurations) then don't do it by half measures. Third-level manipulations (the echo(s) below) are *NOT* your friend. Transform the device presentation (in the devfs or the sysfs) as the device changes. These file systems are dynamic for a reason.

The case of, say a USB root hub with a built in serial port who's controller needs to be flashed. If the controller hasn't been flashed (correctly) or is being re-flashed the serial port just doesn't really exist until that operation completes successfully. If the controller can not be re-flashed, then the device representing the opportunity to flash the controller becomes meaningless (and should probably disappear) when a successful flash has been completed. If the serial port is opened, the flash device should be blocked.

It's simple logical exclusion, and we have primitives for all that if you want to leave the invalid devices visible while they are invalid. Agree on a place in the abstract, or hard-wire the exclusion into the mechanism (via the shared info structure?), but oh-god-please don't put the in-kernel exclusion into the user-space runtime domain.

All other considerations aside, (e.g. the new binary interface just merged in etc.) it isn't the open-ness and close-ness that matters, it's the programmable-device-ready-or-not-ness. Irregular termination close == bad firmware. Programmable device not ready == bad firmware. (and so on). The writer of any particular driver *should* already be looking for the un-ready and the illegal-simultaneous-access cases (both in the firmware-flasher and the user-access parts) or the driver is going to oops anyway.

Pardon my rant... 8-)

Rob.

P.S. I "improperly" use "flash" to talk about the "load" of program code and data into devices which wont retain that information permanently. It's morally wrong, but I work with a bunch of people who don't know the difference and aggressively use the improper term. I live with hearing that day-in and day-out from the management level here. I took no effort to correct that idiomatic mistake in the above as I have aggressively fostered a blind-spot in my head on this topic to keep my sanity 8-). Sorry if anybody was confused. I will not entertain "we are not talking about flashing, we are talking about loading runtime firmware" responses. The non persistent "load" case is covered by default, but the "reload" of non persistent images, and the "(re)flash" of persistent images are both covered by the proper intersection of the set of cases discussed above.

So ptptpth.... 8-)

-----Original Message-----
From: [email protected] [mailto:[email protected]]On Behalf Of Greg KH
Sent: Friday, May 16, 2003 5:04 PM
To: Oliver Neukum
Cc: Manuel Estrada Sainz; LKML; Simon Kelley; Alan Cox; Downing, Thomas; [email protected]; Pavel Roskin
Subject: Re: request_firmware() hotplug interface, third round.

Good point. Actually I don't think that the binary interface for sysfs passes open and close down to the lower levels, so it's a moot point.

echo... works for me.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I think we'd be better off checking for this case by requiring the firmware to include a length field and adapting the binary interface so that we can see the open/close boundary. The "loading" thing is pretty damn ugly.

Plus with the "loading" file, a interrupted load will just appear to be unterminated - and if the scripts run again they'll have to check that the loading file was 0 to start with - and if it isn't there's nothing they can do except wait for the dead load to time out.

Better to catch the close, check the length, then return the firmware or throw the junk image away as appropriate.

David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

I did look for open/close first and finally added the 'loading' because I didn't have them.

I don't thing it is so ugly, but if I could see open/close and the consensus is that 'loading' should go, I'll do it.

The script will not automatically run at least until the timeout is expired.

But in case you are doing things by hand, how about:

	$ echo cancel > .../loading

	or if you want to keep the content numeric:

	$ echo -1 > .../loading

This will also allow the regular script to just cancel the load in case of error, like if the firmware image is not available or a read error happened while reading it.

I'll implement that and the other stuff that came out of Oliver's comments later today and post the new code.

If 'loading' stays the above should fix your timeout issue, and if it goes, yes, that is probably the way to go.

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.

Hmm... on consideration I don't think it's as ugly as I first thought. But I think I have a better idea, see below...

How about combining these two ideas: instead of "loading" and "data" we have "size" and "data". First you write the size, then the data - the driver accepts it once it gets the expected number of bytes. Writing a new size throws away any partial image that's there, and restarts the upload. Writing 0 cancels the upload entirely, and the driver will presumably fail to initialize (or maybe use a default image if it has one).

There's another question - what happens if userspace tries to write an image when the driver hasn't requested one through hotplug?

David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

[snip]

Sounds good. I'll try that.

If you use request_firmware() directly, userspace will not find any file to write to :-)

If you really want to be able to do that, instead of using request_firmware() you could register a class_device of class firmware and implement 'size'_show/store and 'data'_read/write. Thus exposing the firmware memory to userspace. The sequence would be like this:
- On size_store you setup the download.
- You can write directly to the device's memory on data_write.
- Once you get all data you get the device back working.

With this approach, you get the hotplug event for free and can take advantage of compatible userspace scripts.

This is not currently possible, but should be easy to do if found interesting. I'll provide a (non-functional) sample next time.

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]

I just thought this over. This makes more requirements for the userspace scripts, they will need some way to get the size of the image: stat, or ls and some crude regex.

And we can have the same effect with loading/data:

echo 1 > .../loading:
Will start a load, discarding any previous partial load.
echo 0 > .../loading:
Will conclude the load and handle the data to the driver code.
echo -1 > .../loading:
Will conclude the load with an error and the driver won't get any firmware, failing or using firmware in some flash if available.

This way, the script also won't have to check the value of 'loading'.

How does that sound?

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.

Hrm... it still seems a bit icky to me, but I'm not really sure why. I think it would be a bit better if you called it "control" or something instead of "loading". "loading" seems to imply a boolean, which this isn't anymore.

David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

It is still some kind of boolean, but I don't mind to follow the crowd on this one.

What do you guys thing is best?

a) loading
b) control
c) other:_____

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.

d) Don't care.

"loading" or "control" is fine with me, as long as it's documented :)

thanks,

greg k-h

Once we agree on the code, I'll write some documentation for it.

Did you guys see my last post on Saturday? It is supposed to fix all issues, including request_firmware_nowait() and a couple of sysfs patches.

Maybe it went by unnoticed, or am I just too impatient?

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.

[snip]

When I updated my tree (via bk cvs gateway) to make the patch I noticed some changes in sysfs's binary support.

In general, they look good, but the size of files is set in sysfs_create_bin_file and not changeable later. This breaks firmware_class.c :(

With current request_firmware(), the drivers don't tell the size of the firmware, and in some cases they don't even know, so changing the interface is no good.

I also don't understand why sysfs needs to keep a copy of the data in it's own buffer. It has to ask the driver for any read/write anyway, the previous approach of one page at a time looked better to me and saves some kernel memory :-).

And the size checks could be skipped in case of zero size.

I'll include a change proposal to sysfs/bin.c next time.

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.

Ok, if that is true, then we could also have this tool enforce a size. Otherwise we are reading and reading without ever ending and allocating a lot of kernel resources while we are at it.

If we know the size, then we also now start and end. So the "loading" attribute can certainly go.

Regards

Ingo Oeser

A privileged script will be able to kill the system anyway it wants, I don't think that this is needed.

However, forcing an standard header on firmware images including the size would allow size checks without the need for an specialized tool.

Or to prevent memory reallocation every PAGE_SIZE bytes, a tentative size could be written to 'loading' so its meaning would become:

loading > 1:
Start/restart a load of the specified size.
loading = 1:
Start/restart a load of unknown size.
loading = 0:
Finish load.
loading = -1:
Cancel load.

I personally like to have explicit start/end, which also allows cancellation and restart of the firmware load.

Regards

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.