Re: [patch 2.6.12-rc3] modified firmware_class.c to add a newfunction request_firmware_nowait_nohotplug

[randy_dunlap]

On Fri, 8 Jul 2005 19:16:57 -0500 Abhay Salunke wrote:

| This is the patch to add dell_rbu driver. This patch requires the 
| firmware_class.c patch sent earlier which adds request_firmware_nowait_nohotplug 
| function. 
| Andrew , 
| Could you add this patch to the -mm tree. This patch was submitted about a
| week ago for review.
| 
| Signed-off-by: Abhay Salunke <Abhay_Salunke@xxxxxxxx>
| 
| Thanks
| Abhay

General comment:

The patches have lots of trailing whitespace.  Please remove all of that.

| diff -uprN linux-2.6.11.11.orig/drivers/firmware/dell_rbu.c linux-2.6.11.11.new/drivers/firmware/dell_rbu.c
| --- linux-2.6.11.11.orig/drivers/firmware/dell_rbu.c	1969-12-31 18:00:00.000000000 -0600
| +++ linux-2.6.11.11.new/drivers/firmware/dell_rbu.c	2005-06-30 15:37:27.000000000 -0500
| @@ -0,0 +1,627 @@
| +/*
| + * dell_rbu.c
| + * Bios Update driver for Dell systems
| + * Author: Dell Inc
| + *         Abhay Salunke <abhay_salunke@xxxxxxxx>
| + *
| + * Copyright (C) 2005 Dell Inc.
| + *
| + * Remote BIOS Update (rbu) driver is used for updating DELL BIOS by 
| + * creating entries in the /sys file systems on Linux 2.6 and higher 
| + * kernels. The driver supports two mechanism to update the BIOS namely 
                                       mechanisms              BIOS,
| + * contiguous and packetized. Both these methods still require having some
| + * application to set the CMOS bit indicating the BIOS to update itself 
| + * after a reboot.
| + *
| + * Contiguous method:
| + * This driver writes the incoming data in a monolithic image by allocating 
| + * contiguous physical pages large enough to accommodate the incoming BIOS 
| + * image size.  
| + *
| + * Packetized method:
| + * The driver writes the incoming packet image by allocating a new packet 
| + * on every time the packet data is written. This driver requires an 
      each time that the
| + * application to break the BIOS image in to fixed sized packet chunks.
                                          into
| + *
| + * See Documentation/dell_rbu.txt for more info.
| + *
| + */
| +#include <linux/version.h>
| +#include <linux/config.h>
| +#include <linux/init.h>
| +#include <linux/module.h>
| +#include <linux/string.h>
| +#include <linux/errno.h>
| +#include <linux/blkdev.h>
| +#include <linux/device.h>
| +#include <linux/spinlock.h>
| +#include <linux/moduleparam.h>
| +#include <linux/firmware.h>
| +#include <linux/dma-mapping.h>
| +
| +MODULE_AUTHOR("Abhay Salunke <abhay_salunke@xxxxxxxx>");
| +MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
| +MODULE_LICENSE("GPL");
| +MODULE_VERSION("1.0");
| +
| +#define BIOS_SCAN_LIMIT 0xffffffff

Please remove this, it's not used.

| +#define MAX_IMAGE_LENGTH 16
| +static struct _rbu_data {
| +	void *image_update_buffer;
| +	unsigned long image_update_buffer_size;
| +	unsigned long bios_image_size;
| +	spinlock_t lock;
| +	unsigned long packet_read_count;
| +	unsigned long packet_write_count;
| +	unsigned long num_packets;
| +	unsigned long packetsize;
| +} rbu_data;
| +
| +static char image_type[MAX_IMAGE_LENGTH] = "mono";
| +module_param_string(image_type, image_type, sizeof (image_type), 0);

Why is permission 0?  0 means not visible in sysfs.
But you tested this, so.... ?

| +MODULE_PARM_DESC(image_type, "BIOS image type. choose- mono or packet");
| +
| +struct packet_data {
| +	struct list_head list;
| +	size_t length;
| +	void *data;
| +	int ordernum;
| +};
| +
| +static struct packet_data packet_data_head;
| +
| +struct platform_device *rbu_device;
| +int context;
| +static dma_addr_t dell_rbu_dmaaddr;
| +
| +static int
| +fill_last_packet(void *data, size_t length)
| +{
| +	struct list_head *ptemp_list;
| +	struct packet_data *packet = NULL;
| +	int packet_count = 0;
| +
| +	pr_debug("fill_last_packet: entry \n");
                                    entry\n          (other places also)
| +
| +	if (!rbu_data.num_packets) {
| +		pr_debug("fill_last_packet: num_packets=0\n");
| +		return -ENOMEM;
| +	}
| +
| +	packet_count = rbu_data.num_packets;
| +
| +	ptemp_list = (&packet_data_head.list)->prev;
| +
| +	packet = list_entry(ptemp_list, struct packet_data, list);
| +
| +	if ((rbu_data.packet_write_count + length) > rbu_data.packetsize) {
| +		pr_debug("dell_rbu:%s: packet size data "
| +			"overrun\n", __FUNCTION__);
| +		return -EINVAL;
| +	}
| +
| +	pr_debug("fill_last_packet : buffer = %p\n", packet->data);
| +
| +	memcpy((packet->data + rbu_data.packet_write_count), data, length);
| +
| +	if ((rbu_data.packet_write_count + length) == rbu_data.packetsize) {
| +		/*
| +		 * this was the last data chunk in the packet
| +		 * so reinitialize the packet data counter to zero
| +		 */
| +		rbu_data.packet_write_count = 0;
| +	} else
| +		rbu_data.packet_write_count += length;
| +
| +	pr_debug("fill_last_packet: exit \n");
                                    exit\n         (other places too)
| +	return 0;
| +}
| +
| +static int
| +create_packet(size_t length)
| +{
| +	struct packet_data *newpacket;
| +	int ordernum = 0;
| +
| +	pr_debug("create_packet: entry \n");
| +
| +	if (!rbu_data.packetsize) {
| +		pr_debug("create_packet: packetsize not specified\n");
| +		return -EINVAL;
| +	}
| +
| +	newpacket = kmalloc(sizeof (struct packet_data), GFP_KERNEL);
| +	if (!newpacket) {
| +		printk(KERN_WARNING
| +			"dell_rbu:%s: failed to allocate new "
| +			"packet\n", __FUNCTION__);
| +		return -ENOMEM;
| +	}
| +
| +	ordernum = get_order(length);
| +	/*
| +	 * there is no upper limit on memory 
| +	 * address for packetized mechanism 
| +	 */
| +	newpacket->data = (unsigned char *) __get_free_pages(GFP_KERNEL,
| +		ordernum);
| +
| +	pr_debug("create_packet: newpacket %p\n", newpacket->data);
                      actually   newpacket->data
| +
| +	if (!newpacket->data) {
| +		printk(KERN_WARNING
| +			"dell_rbu:%s: failed to allocate new "
| +			"packet\n", __FUNCTION__);
| +		kfree(newpacket);
| +		return -ENOMEM;
| +	}
| +
| +	newpacket->ordernum = ordernum;
| +	++rbu_data.num_packets;
| +	/*
| +	 * initialize the newly created packet headers 
| +	 */
| +	INIT_LIST_HEAD(&newpacket->list);
| +	list_add_tail(&newpacket->list, &packet_data_head.list);
| +	/*
| +	 * packets have fixed size 
| +	 */
| +	newpacket->length = rbu_data.packetsize;
| +
| +	pr_debug("create_packet: exit \n");
| +
| +	return 0;
| +}
| +
| +static int
| +packetize_data(void *data, size_t length)
| +{
| +	int rc = 0;
| +
| +	if (!rbu_data.packet_write_count) {
| +		if ((rc = create_packet(length)))
| +			return rc;
| +	}
| +	if ((rc = fill_last_packet(data, length)))
Just do:
	rc = fill_packet(data, length);
	return rc;

| +		return rc;
| +
| +	return rc;
| +}
| +
| +static int
| +packet_read_list(char *data, size_t * pread_length)
| +{
| +	struct list_head *ptemp_list;
| +	int temp_count = 0;
| +	int bytes_copied = 0;
| +	int bytes_read = 0;
| +	int remaining_bytes = 0;
| +	char *pdest = data;
| +
| +	/* check if we have any packets */
| +	if (0 == rbu_data.num_packets)

Please use (variable == constant) form.

| +		return -ENOMEM;
| +
| +	remaining_bytes = *pread_length;
| +	bytes_read = rbu_data.packet_read_count;
| +
| +	ptemp_list = (&packet_data_head.list)->next;
| +	while (!list_empty(ptemp_list)) {
| +		bytes_copied = do_packet_read(pdest, ptemp_list,
| +			remaining_bytes, bytes_read, &temp_count);
| +		remaining_bytes -= bytes_copied;
| +		bytes_read += bytes_copied;
| +		pdest += bytes_copied;
| +		/*
| +		 * check if we reached end of buffer before reaching the
| +		 * last packet
| +		 */
| +		if (remaining_bytes == 0)
| +			break;
| +
| +		ptemp_list = ptemp_list->next;
| +	}
| +	/*finally set the bytes read */
        /* finally
| +	*pread_length = bytes_read - rbu_data.packet_read_count;
| +	rbu_data.packet_read_count = bytes_read;
| +	return 0;
| +}
| +
| +
| +static ssize_t
| +read_packet_data(char *buffer, loff_t pos, size_t count)
| +{
| +	int retval;
| +	size_t bytes_left;
| +	size_t data_length;
| +	char *ptempBuf = buffer;
| +	unsigned long imagesize;
| +
| +	/* check to see if we have something to return */
| +	if (rbu_data.num_packets == 0) {
| +		pr_debug("read_packet_data: no packets written\n");
| +		retval = -ENOMEM;
| +		goto read_rbu_data_exit;
| +	}
| +
| +	imagesize = rbu_data.num_packets * rbu_data.packetsize;
| +
| +	if (pos > imagesize) {
| +		retval = 0;
| +		printk(KERN_WARNING "dell_rbu:read_packet_data: "
| +			"data underrun\n");
| +		goto read_rbu_data_exit;
| +	}
| +
| +	bytes_left = imagesize - pos;
| +	data_length = min(bytes_left, count);
| +
| +	if ((retval = packet_read_list(ptempBuf, &data_length)) < 0)
| +		goto read_rbu_data_exit;
| +
| +	if ((pos + count) > imagesize) {
| +		rbu_data.packet_read_count = 0;
| +		/* this was the last copy */
| +		retval = bytes_left;
| +	} else
| +		retval = count;
| +
| +      read_rbu_data_exit:
Darn, almost missed that label.  Please move it to column 1 (or 0).
It's hidden as is.

| +	return retval;
| +}
| +
| +static ssize_t
| +read_rbu_mono_data(char *buffer, loff_t pos, size_t count)
| +{
| +	unsigned char *ptemp = NULL;
| +	size_t bytes_left = 0;
| +	size_t data_length = 0;
| +	ssize_t ret_count = 0;
| +
| +	/* check to see if we have something to return */
| +	if ((rbu_data.image_update_buffer == NULL) ||
| +		(rbu_data.bios_image_size == 0)) {
| +		pr_debug("read_rbu_data_mono: image_update_buffer %p ,"
| +			"bios_image_size %lu\n",
| +			rbu_data.image_update_buffer,
| +			rbu_data.bios_image_size);
| +		ret_count = -ENOMEM;
| +		goto read_rbu_data_exit;
| +	}
| +
| +	if (pos > rbu_data.bios_image_size) {
| +		ret_count = 0;
| +		goto read_rbu_data_exit;
| +	}
| +
| +	bytes_left = rbu_data.bios_image_size - pos;
| +	data_length = min(bytes_left, count);
| +
| +	ptemp = rbu_data.image_update_buffer;
| +	memcpy(buffer, (ptemp + pos), data_length);
| +
| +	if ((pos + count) > rbu_data.bios_image_size)
| +		/* this was the last copy */
| +		ret_count = bytes_left;
| +	else
| +		ret_count = count;
| +      read_rbu_data_exit:

Label in column 1.

| +	return ret_count;
| +}
| +
| +static ssize_t
| +read_rbu_data(struct kobject *kobj, char *buffer, loff_t pos, size_t count)
| +{
| +	ssize_t ret_count = 0;
| +
| +	spin_lock(&rbu_data.lock);
| +
| +	if (!strcmp(image_type, "mono"))
| +		ret_count = read_rbu_mono_data(buffer, pos, count);
| +	else if (!strcmp(image_type, "packet"))
| +		ret_count = read_packet_data(buffer, pos, count);
| +	else
| +		pr_debug("read_rbu_data: invalid image type specified\n");

Why is this pr_debug() and not a printk(KERN_WARNING...)?

| +
| +	spin_unlock(&rbu_data.lock);
| +	return ret_count;
| +}
| +
| +static ssize_t
| +rbu_show_image_type(struct platform_device *rbu_dev, char *buf)
| +{
| +	unsigned int size = 0;

Don't need to init size to 0 and then to the return value of
sprintf().

| +	size = sprintf(buf, "%s\n", image_type);
| +	return size;
| +}
| +
| +static ssize_t
| +rbu_store_image_type(struct platform_device *rbu_dev,
| +	const char *buf, size_t count)
| +{
| +	int rc = count;

Blank line between data and code, please.

| +	spin_lock(&rbu_data.lock);
| +
| +	if (strlen(buf) < MAX_IMAGE_LENGTH)
| +		sscanf(buf, "%s", image_type);
| +	else
| +		printk(KERN_WARNING "dell_rbu: image_type is invalid"
| +			"max chars = %d\n", MAX_IMAGE_LENGTH);
| +
| +	/* we must free all previous allocations */
| +	packet_empty_list();
| +	img_update_free();
| +
| +	spin_unlock(&rbu_data.lock);
| +	return rc;
| +}
| +
| +struct rbu_attribute {
| +	struct attribute attr;
| +	 ssize_t(*show) (struct platform_device * rbu_dev, char *buf);
| +	 ssize_t(*store) (struct platform_device * rbu_dev,
| +		const char *buf, size_t count);
| +};
| +
| +#define RBU_DEVICE_ATTR(_name,_mode,_show,_store ) \
| +struct rbu_attribute rbu_attr_##_name = {       \
| +	.attr ={.name= __stringify(_name), .mode= _mode, .owner= THIS_MODULE},\

Use some spaces around '=', as in " = ".  And like below.

| +	.show = _show,                                \
| +	.store = _store,                                \
| +};
| +
| +static RBU_DEVICE_ATTR(image_type, 0644, rbu_show_image_type,
| +	rbu_store_image_type);
| +
| +static struct bin_attribute rbu_data_attr = {
| +	.attr = {.name = "data",.owner = THIS_MODULE,.mode = 0444},

Spaces around '-' and after ','.

| +	.read = read_rbu_data,
| +};
| +
| +void
| +callbackfn_rbu(const struct firmware *fw, void *context)
| +{
| +	int rc = 0;
| +
| +	if (!fw || !fw->size)
| +		return;
| +
| +	spin_lock(&rbu_data.lock);
| +	if (!strcmp(image_type, "mono")) {
| +		if (!img_update_realloc(fw->size))
| +			memcpy(rbu_data.image_update_buffer,
| +				fw->data, fw->size);
| +	} else if (!strcmp(image_type, "packet")) {
| +		if (!rbu_data.packetsize)
| +			rbu_data.packetsize = fw->size;
| +		else if (rbu_data.packetsize != fw->size) {
| +			packet_empty_list();
| +			rbu_data.packetsize = fw->size;
| +		}
| +		packetize_data(fw->data, fw->size);
| +	} else
| +		pr_debug("invalid image type specified.\n");

Wny pr_debug() instead of printk() ?

| +	spin_unlock(&rbu_data.lock);
| +
| +	rc = request_firmware_nowait_nohotplug(THIS_MODULE,
| +		"dell_rbu", &rbu_device->dev, &context, callbackfn_rbu);
| +	if (rc)
| +		printk(KERN_ERR
| +			"dell_rbu:%s request_firmware_nowait failed"
| +			" %d\n", __FUNCTION__, rc);
| +}
| +
| +static int __init
| +dcdrbu_init(void)
| +{
| +	int rc = 0;

Blank line here, please.

| +	spin_lock_init(&rbu_data.lock);
| +
| +	init_packet_head();
| +
| +	rbu_device =
| +		platform_device_register_simple("dell_rbu", -1, NULL, 0);
| +	if (!rbu_device) {
| +		printk(KERN_ERR
| +			"dell_rbu:%s:platform_device_register_simple "
| +			"failed\n", __FUNCTION__);
| +		return -EIO;
| +	}
| +
| +	sysfs_create_file(&rbu_device->dev.kobj,
| +		&rbu_attr_image_type.attr);
| +
| +	sysfs_create_bin_file(&rbu_device->dev.kobj, &rbu_data_attr);
| +
| +	rc = request_firmware_nowait_nohotplug(THIS_MODULE,
| +		"dell_rbu", &rbu_device->dev, &context, callbackfn_rbu);
| +	if (rc)
| +		printk(KERN_ERR "dell_rbu:%s:request_firmware_nowait"
| +			" failed %d\n", __FUNCTION__, rc);
| +
| +	return rc;
| +
| +}
| +
| +static __exit void
| +dcdrbu_exit(void)
| +{
| +	spin_lock(&rbu_data.lock);
| +	packet_empty_list();
| +	img_update_free();
| +	spin_unlock(&rbu_data.lock);
| +	platform_device_unregister(rbu_device);
| +}
| +
| +module_exit(dcdrbu_exit);
| +module_init(dcdrbu_init);
| diff -uprN linux-2.6.11.11.orig/drivers/firmware/Kconfig linux-2.6.11.11.new/drivers/firmware/Kconfig
| --- linux-2.6.11.11.orig/drivers/firmware/Kconfig	2005-06-14 21:00:10.000000000 -0500
| +++ linux-2.6.11.11.new/drivers/firmware/Kconfig	2005-06-30 15:37:43.000000000 -0500
| @@ -58,4 +58,15 @@ config EFI_PCDP
|  
|  	  See <http://www.dig64.org/specifications/DIG64_HCDPv20_042804.pdf>
|  
| +config DELL_RBU
| +	tristate "BIOS update support for DELL systems via sysfs"
| +	default y

Why does the rest of the world want to build this driver?

| +	select FW_LOADER
| +	help
| +	 Say Y if you want to have the option of updating the BIOS for your
| +	 DELL system. Note you need a supporting application to comunicate
| +	 with the BIOS regardin the new image for the image update to
| +	 take effect.

When & where is this mystery app going to be available?

| +	 See <file:Documentation/dell_rbu.txt> for more details on the driver.
| +
|  endmenu


---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/