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 <[EMAIL PROTECTED]>
| 
| 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 <[EMAIL PROTECTED]>
| + *
| + * 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 <[EMAIL PROTECTED]>");
| +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 [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to