The dell_rbu driver is required for updating BIOS on DELL servers and client
systems. The driver lets a user application download the BIOS image in to
contiguous physical memory pages; the driver exposes the memory via sysfs
filesystem. The update mechanism basically has two approcahes; one by
allocating contiguous physical memory and the second approach is by allocating
small chunks of contiguous physical memory.
The patch file is attached to this mail.
By making a contribution to this project, I certify that:
The contribution was created in whole or in part by me and I have the right to
submit it under the open source license indicated in the file.
Signed-off-by: Abhay Salunke <[email protected]>
Thanks,
Abhay Salunke
Software Engineer.
DELL Inc
diff -uprN linux-2.6.11.8.ORIG/Documentation/DELL_RBU.txt linux-2.6.11.8/Documentation/DELL_RBU.txt
--- linux-2.6.11.8.ORIG/Documentation/DELL_RBU.txt 1969-12-31 18:00:00.000000000 -0600
+++ linux-2.6.11.8/Documentation/DELL_RBU.txt 2005-05-09 16:34:47.477703152 -0500
@@ -0,0 +1,92 @@
+Purpose:
+Demonstrate the usage of the DELL_RBU (DELL Remote BIOS Update) driver
+for updating BIOS images on Dell hardware.
+
+Scope:
+This document discusses the functionality of the DELL_RBU driver.
+This driver is required by BIOS update applications shipped by DELL for updating
+BIOS on DELL servers and client systems.
+
+Overview:
+The rbu driver is designed to be running on 2.6 kernel.
+This driver is one single dell_rbu.c file (approx 800 lines total).
+The BIOS update is done by writing the new BIOS image in to contiguous physical
+memory addressable by the BIOS. The user application indicates the BIOS regarding
+the update of a fresh image. The BIOS then scans the memory to find the image and
+it will then update itself. There are basically two different mechanisms for
+writing the BIOS image in to contiguous memory
+1> By writing the image to one single shunk of contiguous physical memory.
+2> By writing image in to smaller chunks of contiguous physical memory.
+The update mechanism is determined by the update application based on the
+particular system type.
+
+Update mechanism using single physical chunk of memory:
+The rbu driver on its load time created the following entries in sysfs
+/sys/firmware/rbu/rbudatasize
+/sys/firmware/rbu/rbudata
+
+Steps to update the BIOS image:
+
+1> Set the incoming BIOS image size in the /sys/firmware/rbudatasize file.
+
+ e.g. echo XXXXXX > /sys/firmware/rbudatasize
+NOTE: the size specified is always in decimal.
+
+you can also read back the image size by doing
+cat /sys/firmware/rbudatasize
+
+2> Download the BIOS image by copying the image file to /sys/firmware/rbudata
+file.
+e.g. echo image.hdr > /sys/firmware/rbudata
+
+you can also read back the image using
+cat /sys/firmware/rbu/rbudata
+This is usually helpful in verifying the image downloaded.
+
+Step#1 results in the driver allocating contiguous physical memory of the size
+echoed in to rbudatasize. The subsequent writes to rbudata as described in
+step #2 results in the image getting written to the allocated contiguous physical
+pages. Repeating step #2 will overwrite the previous data in rbudata file.
+
+On a driver unload the allocated memory is freed and the rbudatasize file reads 0.
+The user should not unload the driver after downloading the new BIOS image for
+if it wants to update BIOS with that image.
+
+The user can overwrite the rbudata file with a new image. The user has to make
+sure that the new image size is less than or equal to the image size copied to
+the rbudatasize file.
+If the new image is grater than the allocated size then only the allocated size
+gets copied the rest will not.
+
+The user can also free the previous BIOS image as follows
+echo 0 > /sys/firmware/rbu/rbudatasize
+
+If the user tries to set the BIOS image size there is a possiblity that the
+system may not have enough contiguous physical memory for upadtes, thus the
+image allocation will fail. The user the needs to verify this by reading back
+the rbudatasize which will be set to 0.
+
+Update using smaller chunks (packets) of contiguous memory:
+The disadvantage of contiguous allocation is that it may not be always possible
+to get that size of contiuguous chunk of avaliable physical pages as in most
+Linux systems the memory gets fragmented immideately after a reboot.
+The update using smaller chunks fixes this issue; it also requires the BIOS on
+the system to support this feature; the update application needs to query this
+with the BIOS on the system before using this technique.
+
+The appplication breaks the BIOS image in to small packets; before starting the
+update using this technique, the application sets the packetdatasize as follows
+
+echo XXXXXX > /sys/firmware/packetdatasize
+Any writes to /sys/firmware/packetdata results in allocation of contiguous
+physical memory of packetdatasize and the data is written to that meomry.
+Writing 0 to packetdatasize results in freeing of all packets.
+Unloading the driver will also result in freeing up of the allocated packets.
+
+NOTE:
+Afte updating the BIOS image the appplication needs to communicate with the BIOS
+for enabling the update on the next reboot. The application can then choose to
+reboot the system imideately or not reboot the system and leave up to the user
+to do a reboot.
+
+
diff -uprN linux-2.6.11.8.ORIG/drivers/firmware/dell_rbu.c linux-2.6.11.8/drivers/firmware/dell_rbu.c
--- linux-2.6.11.8.ORIG/drivers/firmware/dell_rbu.c 1969-12-31 18:00:00.000000000 -0600
+++ linux-2.6.11.8/drivers/firmware/dell_rbu.c 2005-05-09 15:14:30.008069856 -0500
@@ -0,0 +1,862 @@
+/*
+ * dell_rbu.c
+ * Bios Update driver for Dell systems
+ * Author: Dell Inc
+ * Abhay Salunke <[email protected]>
+ *
+ * Copyright (C) 2004 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 contiguous and packetized.
+ * Both these methods still require to have some application to set the
+ * CMOS bit indicating the BIOS to update itself after a reboot.
+ *
+ * Contiguous method:
+ * This driver tries to allocates contiguos physical pages large enough
+ * to accomodate the BIOS image size specified by the user. The user
+ * supplied BIOS image is then copied in to the allocated contiguous pages.
+ *
+ * Packetized method:
+ * In case of packetized the driver provides entries in the /sys file systems
+ * as packetdatasize and packetdata. This driver requires an application to
+ * break the BIOS image in to fixed sized packet chunks and each packet is written
+ * to the packetdata entry. The packetdatasize needs to be set once and is fixed
+ * for all the packets.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#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/firmware.h>
+#include <linux/spinlock.h>
+#include <linux/moduleparam.h>
+
+#define BIT(x) (1UL << ((x)%BITS_PER_LONG))
+#define BIOS_SCAN_LIMIT 0xffffffff
+MODULE_AUTHOR("Abhay Salunke <[email protected]>");
+MODULE_DESCRIPTION("Driver for updating BIOS image on DELL systems");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("0.5");
+
+static struct {
+ void *image_update_buffer;
+ unsigned long image_update_buffer_size;
+ unsigned long bios_image_size;
+ unsigned long image_update_order_number;
+ spinlock_t lock;
+ unsigned long packet_read_count;
+ unsigned long packet_write_count;
+ unsigned long num_packets;
+ unsigned long packetsize;
+}rbu_data ;
+
+struct packet_data{
+ struct list_head list;
+ size_t length;
+ void *rbu_data;
+ int ordernum;
+};
+
+
+static struct packet_data packet_data_head;
+
+/* no default attributes yet. */
+static struct attribute * def_attrs[] = { NULL, };
+
+/* don't use show and store attribute functions */
+static struct sysfs_ops rbu_attr_ops = { };
+
+static struct kobj_type ktype_rbu = {
+ .sysfs_ops = &rbu_attr_ops,
+ .default_attrs = def_attrs,
+};
+
+static decl_subsys(rbu,&ktype_rbu,NULL);
+
+static void init_rbu_lock(void *plock)
+{
+ spin_lock_init((spinlock_t *)plock);
+}
+
+static void lock_rbu_access(void *plock)
+{
+ spin_lock((spinlock_t *)plock);
+}
+
+static void unlock_rbu_access(void *plock)
+{
+ spin_unlock((spinlock_t *)plock);
+}
+
+void init_packet_head(void)
+{
+ INIT_LIST_HEAD((struct list_head *)&packet_data_head);
+ rbu_data.packet_write_count = 0;
+ rbu_data.packet_read_count = 0;
+ rbu_data.num_packets = 0;
+ rbu_data.packetsize = 0;
+}
+
+static int fill_last_packet(void *data, size_t length)
+{
+ struct list_head *ptemp_list;
+ struct packet_data *ppacket = NULL;
+ int packet_count = 0;
+
+ pr_debug("fill_last_packet: entry \n");
+
+ /* check if we have any packets */
+ if (0 == rbu_data.num_packets){
+ pr_debug("fill_last_packet: num_packets=0\n");
+ return -ENOMEM;
+ }
+
+ packet_count = rbu_data.num_packets;
+
+ ptemp_list = ((struct list_head *)&packet_data_head)->next;
+
+ while(--packet_count)
+ {
+ ptemp_list = ptemp_list->next;
+ }
+
+ ppacket = list_entry(ptemp_list,struct packet_data, list);
+
+ if ((rbu_data.packet_write_count + length) > rbu_data.packetsize) {
+ printk(KERN_WARNING"fill_last_packet: packet size data overrun\n");
+ return -ENOMEM;
+ }
+
+ pr_debug("fill_last_packet : buffer = %p\n", ppacket->rbu_data);
+
+ /* copy the incoming data in to the new buffer */
+ memcpy(((char *)ppacket->rbu_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 {
+ /* adjust the total packet length */
+ rbu_data.packet_write_count += length;
+ }
+ pr_debug("fill_last_packet: exit \n");
+ return 0;
+}
+
+/*
+ get_free_pages_limited:
+ This is a helper function which allocates free pages based on an upper limit
+ It first tries to allocate memory normally using the GFP_KERNEL argument
+ if the incoming limit is non-zero and if the returned physical memory address
+ exceeds the upper limit, the allocated pages are freed and the memory is
+ reallocated using the GFP_DMA argument.
+*/
+static void *get_free_pages_limited(unsigned long size,
+ int *ordernum,
+ unsigned long limit)
+{
+ unsigned long ImgBufPhysAddr;
+ void *pbuf = NULL;
+
+ *ordernum = get_order(size);
+ /* check if we are not getting a very large file
+ This can happen as a user error in entering the file size */
+ if (*ordernum == BITS_PER_LONG){
+ /* The incoming size is very large */
+ pr_debug("get_free_pages_limited: Incoming size is very large\n");
+ return NULL;
+ }
+
+ /* try allocating a new buffer to fit the request */
+ pbuf =(unsigned char *)__get_free_pages(GFP_KERNEL, *ordernum);
+
+ if (pbuf != NULL) {
+ /* check if the image is with in limits */
+ ImgBufPhysAddr = (unsigned long)virt_to_phys((void *)pbuf);
+
+ if ((limit != 0) &&
+ ((ImgBufPhysAddr + size) > limit)) {
+
+ pr_debug("Got memory above 4GB range, free this and try with DMA memory\n");
+
+ /* free this memory as we need it with in 4GB range */
+ free_pages ((unsigned long)pbuf, *ordernum);
+
+ /* try allocating a new buffer from the GFP_DMA range
+ as it is with in 16MB range.*/
+ pbuf =(unsigned char *)__get_free_pages(GFP_DMA, *ordernum);
+
+ if (pbuf == NULL)
+ pr_debug("Failed to get memory of size %ld using GFP_DMA\n", size);
+ }
+ }
+ return pbuf;
+}
+
+static int create_packet(size_t length)
+{
+ struct packet_data *newpacket;
+ int ordernum =0;
+
+ pr_debug("create_packet: entry \n");
+
+ if (rbu_data.packetsize == 0 ) {
+ pr_debug("create_packet: packetsize not specified\n");
+ return -EINVAL;
+ }
+
+ newpacket = kmalloc(sizeof(struct packet_data) , GFP_KERNEL);
+ if(newpacket == NULL){
+ printk(KERN_WARNING"create_packet: failed to allocate new packet\n");
+ return -ENOMEM;
+ }
+
+ /* there is no upper limit on memory address for packetized mechanism */
+ newpacket->rbu_data = get_free_pages_limited(rbu_data.packetsize,
+ &ordernum,
+ 0);
+ pr_debug("create_packet: newpacket %p\n", newpacket->rbu_data);
+
+ if(newpacket->rbu_data == NULL){
+ printk(KERN_WARNING"create_packet: failed to allocate new packet\n");
+ return -ENOMEM;
+ }
+
+ newpacket->ordernum = ordernum;
+
+ ++rbu_data.num_packets;
+ /* initialize the newly created packet headers */
+ INIT_LIST_HEAD(&newpacket->list);
+ /* add this packet to the link list */
+ list_add_tail(&newpacket->list, (struct list_head *)&packet_data_head);
+ /* packets are of fixed sizes so initialize the length to rbu_data.packetsize*/
+ 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;
+
+ pr_debug("packetize_data : entry\n");
+ if (rbu_data.packet_write_count == 0){
+ /* create a new packet */
+ if ((rc = create_packet(length)) != 0 )
+ return rc;
+ }
+ /* fill data in to the packet */
+ if ((rc = fill_last_packet(data, length)) != 0)
+ return rc;
+
+ /* set the total image size here */
+ rbu_data.bios_image_size += length;
+ pr_debug("packetize_data : exit\n");
+ return rc;
+}
+
+
+/*
+ do_packet_read :
+ This is a helper function which reads the packet data of the
+ current list.
+ data: is the incoming buffer
+ ptemp_list: points to the incoming list item
+ length: is the length of the free space in the buffer.
+ bytes_read: is the total number of bytes read already from
+ the packet list
+ list_read_count: is the counter to keep track of the number
+ of bytes read out of each packet.
+*/
+int do_packet_read(char * data, struct list_head *ptemp_list, int length,
+ int bytes_read, int *list_read_count)
+{
+ void *ptemp_buf;
+ struct packet_data *newpacket = NULL;
+ int bytes_copied = 0;
+ void *pDest = data;
+ int j = 0;
+
+ newpacket = list_entry(ptemp_list,struct packet_data, list);
+ *list_read_count += newpacket->length;
+
+ if (*list_read_count > bytes_read) {
+ /* point to the start of unread data */
+ j = newpacket->length - (*list_read_count - bytes_read);
+ /* point to the offset in the packet buffer*/
+ ptemp_buf = (u8 *)newpacket->rbu_data + j;
+ /* check if there is enough room in the
+ incoming buffer*/
+ if (length > (*list_read_count - bytes_read))
+ /* copy what ever is there in this packet and move on*/
+ bytes_copied = (*list_read_count - bytes_read);
+ else
+ /* copy the remaining */
+ bytes_copied = length;
+ memcpy(pDest, ptemp_buf, bytes_copied);
+ }
+ return bytes_copied;
+}
+
+/*
+ packet_read_list:
+ This function reads the data out of the packet link list.
+ It will read data from multiple packets depending upon the
+ size of the incoming buffer.
+ data: is the incoming buffer pointer
+ *pread_length: is the length of the incoming buffer. At return
+ this value is adjusted to the actual size of the data read.
+*/
+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)
+ return -ENOMEM;
+
+ remaining_bytes = *pread_length;
+ /* get the current read count */
+ bytes_read = rbu_data.packet_read_count;
+
+ ptemp_list = ((struct list_head *)&packet_data_head)->next;
+ while(!list_empty(ptemp_list))
+ {
+ /* read data */
+ bytes_copied = do_packet_read(pDest, ptemp_list, remaining_bytes,
+ bytes_read, &temp_count);
+ /* adjust the remaining bytes */
+ remaining_bytes -= bytes_copied;
+ bytes_read += bytes_copied;
+ /* adjust the data pointer */
+ pDest = (u8 *)pDest + bytes_copied;
+
+ if (remaining_bytes == 0){
+ /* we reached end of buffer before
+ reaching the last packet*/
+ break;
+ }
+ /* point to the next packet in the list */
+ ptemp_list = ptemp_list->next;
+ }
+ /*finally set the bytes read */
+ *pread_length = bytes_read - rbu_data.packet_read_count;
+ rbu_data.packet_read_count = bytes_read;
+ return 0;
+}
+
+static void packet_empty_list(void)
+{
+ struct list_head *ptemp_list;
+ struct list_head *pnext_list;
+ struct packet_data *newpacket;
+
+ ptemp_list = ((struct list_head *)&packet_data_head)->next;
+ while(!list_empty(ptemp_list))
+ {
+ newpacket = list_entry(ptemp_list, struct packet_data, list);
+ /* adjust the total packet length */
+ rbu_data.bios_image_size -= newpacket->length;
+ /* get the next list ptr before we delete this entry */
+ pnext_list = ptemp_list->next;
+ /* remove the list entry */
+ list_del(ptemp_list);
+ /* set the list to next */
+ ptemp_list = pnext_list;
+ /* zero out the RBU packet memory before freeing */
+ memset(newpacket->rbu_data, 0, rbu_data.packetsize);
+ /* free the memory pointed by this packet */
+ free_pages((unsigned long)newpacket->rbu_data, newpacket->ordernum);
+ /* now free the packet*/
+ kfree(newpacket);
+ }
+
+ rbu_data.packet_write_count = 0;
+ rbu_data.packet_read_count = 0;
+ rbu_data.num_packets = 0;
+}
+
+
+/*
+ img_update_free:
+ Frees the buffer allocated for storing BIOS image
+ Always called with lock held
+*/
+static void img_update_free( void)
+{
+ if (rbu_data.image_update_buffer == NULL)
+ return;
+
+ /* zero out this buffer before freeing it */
+ memset(rbu_data.image_update_buffer, 0, rbu_data.image_update_buffer_size);
+ /* unlock as free_pages might sleep */
+ unlock_rbu_access(&rbu_data.lock);
+ free_pages((unsigned long)rbu_data.image_update_buffer,
+ rbu_data.image_update_order_number);
+ lock_rbu_access(&rbu_data.lock);
+ rbu_data.image_update_buffer = NULL;
+ rbu_data.image_update_buffer_size = 0;
+ rbu_data.bios_image_size = 0;
+}
+
+/*
+ img_update_realloc:
+ This function allocates the contiguous pages to accomodate the requested
+ size of data. The memory address and size values are stored globally and
+ on every call to this function the new size is checked to see if more
+ data is required than the existing size. If true the previous memory is freed
+ and new allocation is done to accomodate the new size.
+ If the incoming size is less then than the already allocated size, then that
+ memory is reused.
+*/
+static int img_update_realloc(unsigned long size)
+{
+ unsigned char *image_update_buffer = NULL;
+ unsigned long rc;
+ int ordernum =0;
+
+
+ /* check if the buffer of sufficient size has been already allocated */
+ if (rbu_data.image_update_buffer_size >= size) {
+ /* check for corruption */
+ if ((size != 0) && (rbu_data.image_update_buffer == NULL)) {
+ pr_debug("img_update_realloc: corruption check failed\n");
+ return -ENOMEM;
+ }
+ /* we have a valid pre-allocated buffer with sufficient size */
+ return 0;
+ }
+
+ /* free any previously allocated buffer */
+ img_update_free();
+
+ /* This has already been called as locked so we can now unlock
+ and proceed to calling get_free_pages_limited as this function
+ can sleep*/
+ unlock_rbu_access(&rbu_data.lock);
+
+ image_update_buffer = (unsigned char *)get_free_pages_limited(size,
+ &ordernum,
+ BIOS_SCAN_LIMIT);
+
+ /* acquire the semaphore again */
+ lock_rbu_access(&rbu_data.lock);
+
+ if (image_update_buffer != NULL) {
+ /* store address for the new buffer */
+ rbu_data.image_update_buffer = image_update_buffer;
+ /* adjust allocated size */
+ rbu_data.image_update_buffer_size = BIT(ordernum) * PAGE_SIZE;
+ /* save the current order number */
+ rbu_data.image_update_order_number = ordernum;
+ /* initialize the new buffer data to 0 */
+ memset(rbu_data.image_update_buffer,0, rbu_data.image_update_buffer_size);
+ pr_debug("img_update_realloc: success\n");
+ /* success */
+ rc = 0;
+ } else {
+ pr_debug("Not enough memory for image update:order number = %d"
+ ",size = %ld\n",ordernum, size);
+ rc = -ENOMEM;
+ }
+
+ return rc;
+} /* img_update_realloc */
+
+
+/*
+ read_packet_data_size:
+ Returns the size of an RBU packet; if no packets present returns 0
+*/
+static ssize_t read_packet_data_size(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ unsigned int size = 0;
+ if (pos == 0) {
+ lock_rbu_access(&rbu_data.lock);
+ size = sprintf(buffer, "%lu\n", rbu_data.packetsize);
+ unlock_rbu_access(&rbu_data.lock);
+ }
+ return size;
+}
+
+/*
+ write_packet_data_size:
+ Writes the RBU data size supplied by the user, if the
+ data size supplied is non zero number, this function
+ records the packet size for any packet allocations.
+ If a byte size of zero is supplied this function will free
+ the previously allocated packets.
+*/
+static ssize_t write_packet_data_size(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ int retVal = count;
+
+ lock_rbu_access(&rbu_data.lock);
+ /* extract the image size */
+ sscanf(buffer, "%lu",&rbu_data.packetsize);
+ /* free the previous packet lists */
+ packet_empty_list();
+
+ unlock_rbu_access(&rbu_data.lock);
+ return retVal;
+}
+
+/*
+ read_rbu_data_size:
+ Returns the size of an RBU image previously downloaded
+ if no image is downloaded the size returned is 0
+*/
+static ssize_t read_rbu_data_size(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ unsigned int size = 0;
+ if (pos == 0) {
+ lock_rbu_access(&rbu_data.lock);
+ size = sprintf(buffer, "%lu\n", rbu_data.bios_image_size);
+ unlock_rbu_access(&rbu_data.lock);
+ }
+ return size;
+}
+
+/*
+ write_rbu_data_size:
+ Writes the RBU data size supplied by the user, if the
+ data size supplied is non zero number, this function
+ allocates the contiguous physical memory pages for the
+ supplied size , if it fails this function returns error.
+ If a byte size of zero is supplied this function will free
+ the previously allocated contiguous pages.
+*/
+static ssize_t write_rbu_data_size(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ int retVal = count;
+
+ lock_rbu_access(&rbu_data.lock);
+ /* extract the image size */
+ sscanf(buffer, "%lu",&rbu_data.bios_image_size);
+
+ if (rbu_data.bios_image_size !=0 ) {
+ if (img_update_realloc(rbu_data.bios_image_size) < 0) {
+ pr_debug("write_rbu_data_size: failed to allocate mem size %lu\n",
+ (unsigned long)rbu_data.bios_image_size);
+ rbu_data.bios_image_size = 0;
+ retVal = -ENOMEM;
+ }
+ } else {
+ pr_debug(" freeing %ld size memory \n", rbu_data.bios_image_size);
+ /* free any allocated RBU memory */
+ img_update_free();
+ }
+
+ pr_debug("write_rbu_data_size: = %lu\n",
+ (unsigned long)rbu_data.bios_image_size);
+
+ unlock_rbu_access(&rbu_data.lock);
+ return retVal;
+} /* write_rbu_data_size*/
+
+/*
+ read_rbu_data:
+ Reads the BIOS image file from previously allocated contiguous physical
+ pages in to the buffer supplied in this call.
+ The reading is done in chunks of bytes supplied in the count argument.
+ The reading stops when the total number of bytes read equals the image
+ size given previously.
+ If image size is not specified or if the image size is zero this function
+ returns failure.
+ Parameters:
+ kobj is the kernel object
+ buffer is the pointer to the incoming data buffer.
+ count is the value of the incoming buffer size,
+ pos is the amount of bytes already read.
+*/
+static ssize_t read_rbu_data(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ unsigned char *ptemp = NULL;
+ int retVal =0;
+ int bytes_left = 0;
+ int data_length = 0;
+
+ lock_rbu_access(&rbu_data.lock);
+
+ /* 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: image_update_buffer %p ,"
+ "bios_image_size %lu\n",
+ rbu_data.image_update_buffer,
+ rbu_data.bios_image_size);
+ retVal = -ENOMEM;
+ goto read_rbu_data_exit;
+ }
+
+ if ( pos > rbu_data.bios_image_size ) {
+ retVal = 0;
+ goto read_rbu_data_exit;
+ }
+
+ bytes_left = rbu_data.bios_image_size - pos;
+ data_length = ((bytes_left > count) ? count : bytes_left);
+
+ ptemp = (char *)rbu_data.image_update_buffer;
+ memcpy(buffer, (char *)(ptemp + pos), data_length);
+
+ if ((pos + count) > rbu_data.bios_image_size) {
+ /* this was the last copy */
+ retVal = bytes_left;
+ }
+ else
+ retVal = count;
+
+read_rbu_data_exit:
+ unlock_rbu_access(&rbu_data.lock);
+ return retVal;
+}
+
+/*
+ write_rbu_data
+ Writes from the incoming BIOS image file to the pre-allocated
+ contiguous physical memory pages.
+ The writes occur in chunks of memory supplied by the count. The writes
+ stops when the total memory supplied equals the image size given previously.
+ If no memory size is previously specified or if the previously specified size
+ is zero the write returns error.
+*/
+static ssize_t write_rbu_data(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ unsigned char *pDest = NULL;
+ unsigned char *ptemp = NULL;
+ int retVal = 0;
+
+ lock_rbu_access(&rbu_data.lock);
+
+ /* check if the image size is given */
+ if (0 == rbu_data.bios_image_size){
+ printk(KERN_WARNING"write_rbu_data: BIOS image size not set\n");
+ retVal = -ENOMEM;
+ goto error_exit;
+ }
+
+ if ((pos + count) > rbu_data.bios_image_size) {
+ pr_debug("write_rbu_data: data_over_run, file pos %lu "
+ "bios_image_size %lu\n",
+ (unsigned long)pos,
+ rbu_data.bios_image_size);
+ retVal = -ENOMEM;
+ goto error_exit;
+ }
+
+ pDest = (unsigned char*)rbu_data.image_update_buffer;
+ ptemp = pDest + pos;
+ memcpy(ptemp, buffer, count);
+ retVal = count;
+ pr_debug("write_rbu_data : retVal = %d\n", retVal);
+error_exit:
+ unlock_rbu_access(&rbu_data.lock);
+ return retVal;
+}
+
+
+/*
+ read_rbu_packet_data:
+ Reads the BIOS image file from previously allocated contiguous physical
+ pages in to the buffer supplied in this call.
+ The reading is done in chunks of bytes supplied in the count argument.
+ The reading stops when the total number of bytes read equals the image
+ size given previously.
+ If image size is not specified or if the image size is zero this function
+ returns failure.
+ Parameters:
+ kobj is the kernel object
+ buffer is the pointer to the incoming data buffer.
+ count is the value of the incoming buffer size,
+ pos is the amount of bytes already read.
+*/
+static ssize_t read_rbu_packet_data(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ int retVal =0;
+ int bytes_left = 0;
+ size_t data_length = 0;
+ char *ptempBuf = buffer;
+ unsigned long imagesize = 0;
+
+ lock_rbu_access(&rbu_data.lock);
+
+ /* check to see if we have something to return */
+ if (rbu_data.num_packets == 0) {
+ pr_debug("read_rbu_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"read_rbu_packet_data: data underrun\n");
+ goto read_rbu_data_exit;
+ }
+
+ bytes_left = imagesize - pos;
+ data_length = ((bytes_left > count) ? count : bytes_left);
+
+ 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:
+ unlock_rbu_access(&rbu_data.lock);
+ return retVal;
+}
+
+/*
+ write_rbu_packet_data
+ Writes from the incoming BIOS image file to the pre-allocated
+ contiguous physical memory pages.
+ The writes occur in chunks of memory supplied by the count. The writes
+ stops when the total memory supplied equals the image size given previously.
+ If no memory size is previously specified or if the previously specified size
+ is zero the write returns error.
+*/
+static ssize_t write_rbu_packet_data(struct kobject *kobj, char *buffer,
+ loff_t pos, size_t count)
+{
+ int retVal = 0;
+
+ lock_rbu_access(&rbu_data.lock);
+
+ /* check if the packet size is given */
+ if (0 == rbu_data.packetsize){
+ printk(KERN_WARNING"write_rbu_packet_data: packetsize not set\n");
+ retVal = -ENOMEM;
+ goto error_exit;
+ }
+
+ if ((pos + count) > rbu_data.packetsize) {
+ pr_debug("write_rbu_packet_data: data_over_run, file pos %lu,"
+ "packetsize %lu\n",
+ (unsigned long)pos,
+ rbu_data.packetsize);
+ retVal = -ENOMEM;
+
+ /* We have a write data overrun, obviously this is
+ not the corret file, so free the previous data*/
+ pr_debug("data overrun freeing all the previous packets\n");
+ packet_empty_list();
+ goto error_exit;
+ }
+
+ if ((retVal = packetize_data(buffer, count)) < 0 ) {
+ pr_debug(KERN_WARNING"write_rbu_packet_data: packetize_data failed with status %d\n", retVal);
+ retVal = -EIO;
+ goto error_exit;
+ }
+
+ retVal = count;
+
+ pr_debug("write_rbu_packet_data : retVal = %d\n", retVal);
+
+error_exit:
+ unlock_rbu_access(&rbu_data.lock);
+ return retVal;
+}
+
+
+static struct bin_attribute rbudata_attr = {
+ .attr = {.name = "rbudata", .owner = THIS_MODULE, .mode = 0644},
+ .read = read_rbu_data,
+ .write = write_rbu_data,
+};
+
+static struct bin_attribute rbudatasize_attr = {
+ .attr = { .name = "rbudatasize", .owner = THIS_MODULE, .mode = 0644 },
+ .read = read_rbu_data_size,
+ .write= write_rbu_data_size,
+};
+
+static struct bin_attribute packetdatasize_attr = {
+ .attr = { .name = "packetdatasize", .owner = THIS_MODULE, .mode = 0644 },
+ .read = read_packet_data_size,
+ .write= write_packet_data_size,
+};
+
+static struct bin_attribute packetdata_attr = {
+ .attr = { .name = "packetdata", .owner = THIS_MODULE, .mode = 0644 },
+ .read = read_rbu_packet_data,
+ .write= write_rbu_packet_data,
+};
+
+static int __init dcdrbu_init(void)
+{
+ int rc;
+
+ init_rbu_lock(&rbu_data.lock);
+
+ init_packet_head();
+
+ rc = firmware_register(&rbu_subsys);
+ if (rc < 0) {
+ printk(KERN_WARNING"dcdrbu_init: firmware_register failed\n");
+ return rc;
+ }
+
+ sysfs_create_bin_file(&rbu_subsys.kset.kobj, &rbudata_attr );
+ sysfs_create_bin_file(&rbu_subsys.kset.kobj, &rbudatasize_attr );
+ sysfs_create_bin_file(&rbu_subsys.kset.kobj, &packetdatasize_attr );
+ sysfs_create_bin_file(&rbu_subsys.kset.kobj, &packetdata_attr );
+
+ return rc;
+}
+
+static __exit void dcdrbu_exit( void)
+{
+ lock_rbu_access(&rbu_data.lock);
+ packet_empty_list();
+ img_update_free();
+ unlock_rbu_access(&rbu_data.lock);
+ sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &rbudata_attr );
+ sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &rbudatasize_attr );
+ sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &packetdatasize_attr );
+ sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &packetdata_attr );
+ firmware_unregister(&rbu_subsys);
+}
+
+module_exit(dcdrbu_exit);
+module_init(dcdrbu_init);
+
diff -uprN linux-2.6.11.8.ORIG/drivers/firmware/Kconfig linux-2.6.11.8/drivers/firmware/Kconfig
--- linux-2.6.11.8.ORIG/drivers/firmware/Kconfig 2005-05-09 15:13:06.257801832 -0500
+++ linux-2.6.11.8/drivers/firmware/Kconfig 2005-05-09 16:30:47.746147888 -0500
@@ -58,4 +58,16 @@ config EFI_PCDP
See <http://www.dig64.org/specifications/DIG64_HCDPv20_042804.pdf>
+config DELL_RBU
+ bool "BIOS update support for DELL systems via sysfs"
+ default n
+ 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 regardign the new image for the image update to
+ take effect.
+
+ See <file:Documentation/DELL_RBU.txt> for more details on the driver.
+
+
endmenu
diff -uprN linux-2.6.11.8.ORIG/drivers/firmware/Makefile linux-2.6.11.8/drivers/firmware/Makefile
--- linux-2.6.11.8.ORIG/drivers/firmware/Makefile 2005-05-09 15:13:06.257801832 -0500
+++ linux-2.6.11.8/drivers/firmware/Makefile 2005-05-09 15:15:16.342026024 -0500
@@ -4,3 +4,4 @@
obj-$(CONFIG_EDD) += edd.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_PCDP) += pcdp.o
+obj-$(CONFIG_DELL_RBU) += dell_rbu.o
Abhay Salunke <[email protected]> wrote:
>
> The dell_rbu driver is required for updating BIOS on DELL servers and client
> systems. The driver lets a user application download the BIOS image in to
> contiguous physical memory pages; the driver exposes the memory via sysfs
> filesystem. The update mechanism basically has two approcahes; one by
> allocating contiguous physical memory and the second approach is by allocating
> small chunks of contiguous physical memory.
>
> The patch file is attached to this mail.
Greg, could you please review the sysfs usage?
> --- linux-2.6.11.8.ORIG/Documentation/DELL_RBU.txt 1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.11.8/Documentation/DELL_RBU.txt 2005-05-09 16:34:47.477703152 -0500
2.6.11.8 is very old in kernel terms. It's OK for this patch because it's
pretty standalone. But in general, please raise patches against
contemporary kernels. 2.6.12-rc4 would be appropriate at present.
> +
> +#define BIT(x) (1UL << ((x)%BITS_PER_LONG))
This is used only for:
+ rbu_data.image_update_buffer_size = BIT(ordernum) * PAGE_SIZE;
Please remove BIT() and just do
+ rbu_data.image_update_buffer_size = PAGE_SIZE << ordernum;
> +
> +static struct {
Some gcc's don't like anonymous structs (I think?). Still, it would be
more conventional to give this guy a name.
> + void *image_update_buffer;
> + unsigned long image_update_buffer_size;
> + unsigned long bios_image_size;
> + unsigned long image_update_order_number;
> + spinlock_t lock;
> + unsigned long packet_read_count;
> + unsigned long packet_write_count;
> + unsigned long num_packets;
> + unsigned long packetsize;
> +}rbu_data ;
Make this
} rbu_data;
> +struct packet_data{
struct packet_data {
> + struct list_head list;
> + size_t length;
> + void *rbu_data;
Should that be
struct rbu_data *rbu_data;
?
> +
> +static decl_subsys(rbu,&ktype_rbu,NULL);
> +
static decl_subsys(rbu, &ktype_rbu, NULL);
> +static void init_rbu_lock(void *plock)
> +{
> + spin_lock_init((spinlock_t *)plock);
> +}
> +
> +static void lock_rbu_access(void *plock)
> +{
> + spin_lock((spinlock_t *)plock);
> +}
> +
> +static void unlock_rbu_access(void *plock)
> +{
> + spin_unlock((spinlock_t *)plock);
> +}
Remove these and simply open-code the spinlock functions.
> +void init_packet_head(void)
> +{
> + INIT_LIST_HEAD((struct list_head *)&packet_data_head);
OK, in lots of places the patch typecasts packet_data_head to a list_head,
then does list_head operations on it, relying on the fact that
packet_data.list is the first element. Very weird.
Please replace all those with (for example)
INIT_LIST_HEAD(&packet_data_head.list);
> +static int fill_last_packet(void *data, size_t length)
> +{
> + struct list_head *ptemp_list;
> + struct packet_data *ppacket = NULL;
> + int packet_count = 0;
> +
> + pr_debug("fill_last_packet: entry \n");
> +
> + /* check if we have any packets */
> + if (0 == rbu_data.num_packets){
I know that some people like the `if (constant == variable)' thing to pick
up "= instead of ==" typos. But gcc does warn about that mistake anyway,
and I think most people find the above construct to be artificial, and
harder to read. Would prefer that all these be swapped around please.
Would also prefer a space before the '{'!
> + ptemp_list = ((struct list_head *)&packet_data_head)->next;
Use packet_head_data.list
> +
> + while(--packet_count)
> + {
while (--packet_count) {
> + if ((rbu_data.packet_write_count + length) > rbu_data.packetsize) {
^^
> + printk(KERN_WARNING"fill_last_packet: packet size data overrun\n");
^ space here
> +*/
> +static void *get_free_pages_limited(unsigned long size,
> + int *ordernum,
> + unsigned long limit)
Wild whitespace here?
> +{
> + unsigned long ImgBufPhysAddr;
Please stick with lower case and underscores in identifiers.
> + void *pbuf = NULL;
> +
> + *ordernum = get_order(size);
> + /* check if we are not getting a very large file
> + This can happen as a user error in entering the file size */
The comment layout is a bit odd.
/*
* Check if we are not getting a very large file. This can happen
* as a user error in entering the file size.
*/
> + if (*ordernum == BITS_PER_LONG){
^ space
> + /* The incoming size is very large */
> + pr_debug("get_free_pages_limited: Incoming size is very large\n");
> + return NULL;
> + }
> +
> + /* try allocating a new buffer to fit the request */
> + pbuf =(unsigned char *)__get_free_pages(GFP_KERNEL, *ordernum);
> +
> + if (pbuf != NULL) {
> + /* check if the image is with in limits */
> + ImgBufPhysAddr = (unsigned long)virt_to_phys((void *)pbuf);
> +
> + if ((limit != 0) &&
> + ((ImgBufPhysAddr + size) > limit)) {
> +
> + pr_debug("Got memory above 4GB range, free this and try with DMA memory\n");
The indenting has gone funny here.
> +
> + /* free this memory as we need it with in 4GB range */
> + free_pages ((unsigned long)pbuf, *ordernum);
> +
> + /* try allocating a new buffer from the GFP_DMA range
> + as it is with in 16MB range.*/
> + pbuf =(unsigned char *)__get_free_pages(GFP_DMA, *ordernum);
> +
> + if (pbuf == NULL)
> + pr_debug("Failed to get memory of size %ld using GFP_DMA\n", size);
> + }
> + }
> + return pbuf;
> +}
What architecture is this code designed for? On x86 a GFP_KERNEL
allocation will never return highmem. I guess x86_64?
I assume this code is here because the x86_64 BIOS will only access the
lower 4GB? If so, a comment to that extent would be useful.
Sometime I expect that x86_64 will gain a new zone, GFP_DMA32 which will be
guaranteed to return memory below he 4GB point. When that happens, this
driver should be converted to use it.
> +static int create_packet(size_t length)
> +{
> + struct packet_data *newpacket;
> + int ordernum =0;
^ space
> + newpacket = kmalloc(sizeof(struct packet_data) , GFP_KERNEL);
^
> + if(newpacket == NULL){
^ ^ spaces
> + if(newpacket->rbu_data == NULL){
Ditto. Lots of places.
> + printk(KERN_WARNING"create_packet: failed to allocate new packet\n");
> + return -ENOMEM;
> + }
> +
> + newpacket->ordernum = ordernum;
> +
> + ++rbu_data.num_packets;
> + /* initialize the newly created packet headers */
> + INIT_LIST_HEAD(&newpacket->list);
> + /* add this packet to the link list */
> + list_add_tail(&newpacket->list, (struct list_head *)&packet_data_head);
Does this list not need locking?
Use packet_data_head.list, remove the typecast.
> + /* packets are of fixed sizes so initialize the length to rbu_data.packetsize*/
Try to fit the code into an 80-column xterm, please.
> +/*
> + do_packet_read :
> + This is a helper function which reads the packet data of the
> + current list.
> + data: is the incoming buffer
> + ptemp_list: points to the incoming list item
> + length: is the length of the free space in the buffer.
> + bytes_read: is the total number of bytes read already from
> + the packet list
> + list_read_count: is the counter to keep track of the number
> + of bytes read out of each packet.
> +*/
> +int do_packet_read(char * data, struct list_head *ptemp_list, int length,
It's (a bit) more conventional to not have the space after the `*' when
declaring pointers:
char *data
Here, both styles are used in the same line!
> +{
> + void *ptemp_buf;
> + struct packet_data *newpacket = NULL;
This initialisation is unneeded.
> + int bytes_copied = 0;
> + void *pDest = data;
lowercase and underscore in identifiers
> + int j = 0;
> +
> + newpacket = list_entry(ptemp_list,struct packet_data, list);
> + *list_read_count += newpacket->length;
> +
> + if (*list_read_count > bytes_read) {
> + /* point to the start of unread data */
> + j = newpacket->length - (*list_read_count - bytes_read);
> + /* point to the offset in the packet buffer*/
> + ptemp_buf = (u8 *)newpacket->rbu_data + j;
The cast is actually unneeded - gcc will treat void* as a "pointer to
something which has sizeof==1" when performing such arithmetic. I believe
this is a gcc extension.
> + /* check if there is enough room in the
> + incoming buffer*/
Odd comment layout.
> +/*
> + packet_read_list:
> + This function reads the data out of the packet link list.
> + It will read data from multiple packets depending upon the
> + size of the incoming buffer.
> + data: is the incoming buffer pointer
> + *pread_length: is the length of the incoming buffer. At return
> + this value is adjusted to the actual size of the data read.
> +*/
> +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;
Are all these initialisations actually needed?
> + ptemp_list = ((struct list_head *)&packet_data_head)->next;
packet_data_head.next
> + while(!list_empty(ptemp_list))
> + {
while (!list_empty(ptemp_list)) {
> +static void packet_empty_list(void)
> +{
> + struct list_head *ptemp_list;
> + struct list_head *pnext_list;
> + struct packet_data *newpacket;
> +
> + ptemp_list = ((struct list_head *)&packet_data_head)->next;
ditto
> + while(!list_empty(ptemp_list))
> + {
ditto
I think list_for_each_entry_safe() would work here.
> +/*
> + img_update_free:
> + Frees the buffer allocated for storing BIOS image
> + Always called with lock held
> +*/
> +static void img_update_free( void)
static void img_update_free(void)
> +{
> + if (rbu_data.image_update_buffer == NULL)
> + return;
Can this happen?
> + /* zero out this buffer before freeing it */
> + memset(rbu_data.image_update_buffer, 0, rbu_data.image_update_buffer_size);
> + /* unlock as free_pages might sleep */
> + unlock_rbu_access(&rbu_data.lock);
> + free_pages((unsigned long)rbu_data.image_update_buffer,
> + rbu_data.image_update_order_number);
> + lock_rbu_access(&rbu_data.lock);
free_pages() will never sleep.
> + rbu_data.image_update_buffer = NULL;
> + rbu_data.image_update_buffer_size = 0;
> + rbu_data.bios_image_size = 0;
> +}
Are these assignments needed?
> +static int img_update_realloc(unsigned long size)
> +{
> + unsigned char *image_update_buffer = NULL;
> + unsigned long rc;
> + int ordernum =0;
> +
> +
> + /* check if the buffer of sufficient size has been already allocated */
> + if (rbu_data.image_update_buffer_size >= size) {
> + /* check for corruption */
> + if ((size != 0) && (rbu_data.image_update_buffer == NULL)) {
> + pr_debug("img_update_realloc: corruption check failed\n");
> + return -ENOMEM;
ENOMEM seems to be the wrong error to return here.
> + }
> + /* we have a valid pre-allocated buffer with sufficient size */
> + return 0;
> + }
> +
> + /* free any previously allocated buffer */
> + img_update_free();
> +
> + /* This has already been called as locked so we can now unlock
> + and proceed to calling get_free_pages_limited as this function
> + can sleep*/
> + unlock_rbu_access(&rbu_data.lock);
> +
> + image_update_buffer = (unsigned char *)get_free_pages_limited(size,
get_free_pages_limited() returns void*, so no typecast is needed.
> + &ordernum,
> + BIOS_SCAN_LIMIT);
> +
> + /* acquire the semaphore again */
It's actually a spinlock, not a semaphore.
> + lock_rbu_access(&rbu_data.lock);
> +
> + if (image_update_buffer != NULL) {
> + /* store address for the new buffer */
> + rbu_data.image_update_buffer = image_update_buffer;
> + /* adjust allocated size */
> + rbu_data.image_update_buffer_size = BIT(ordernum) * PAGE_SIZE;
> + /* save the current order number */
> + rbu_data.image_update_order_number = ordernum;
> + /* initialize the new buffer data to 0 */
> + memset(rbu_data.image_update_buffer,0, rbu_data.image_update_buffer_size);
> + pr_debug("img_update_realloc: success\n");
> + /* success */
> + rc = 0;
> + } else {
> + pr_debug("Not enough memory for image update:order number = %d"
> + ",size = %ld\n",ordernum, size);
> + rc = -ENOMEM;
> + }
> +
> + return rc;
> +} /* img_update_realloc */
The comment over img_update_realloc() should mention that it returns with
the lock held.
> +
> +/*
> + read_packet_data_size:
> + Returns the size of an RBU packet; if no packets present returns 0
> +*/
> +static ssize_t read_packet_data_size(struct kobject *kobj, char *buffer,
> + loff_t pos, size_t count)
> +{
> + unsigned int size = 0;
> + if (pos == 0) {
> + lock_rbu_access(&rbu_data.lock);
> + size = sprintf(buffer, "%lu\n", rbu_data.packetsize);
> + unlock_rbu_access(&rbu_data.lock);
The locking here is meaningless. rbu_data.packetsize can change any time
before or after the read.
> + }
> + return size;
> +}
> +
> +/*
> + write_packet_data_size:
> + Writes the RBU data size supplied by the user, if the
> + data size supplied is non zero number, this function
> + records the packet size for any packet allocations.
> + If a byte size of zero is supplied this function will free
> + the previously allocated packets.
> +*/
> +static ssize_t write_packet_data_size(struct kobject *kobj, char *buffer,
> + loff_t pos, size_t count)
80-cols.
> +static ssize_t read_rbu_data_size(struct kobject *kobj, char *buffer,
> + loff_t pos, size_t count)
ditto.
> +{
> + unsigned int size = 0;
> + if (pos == 0) {
> + lock_rbu_access(&rbu_data.lock);
> + size = sprintf(buffer, "%lu\n", rbu_data.bios_image_size);
> + unlock_rbu_access(&rbu_data.lock);
Unneeded or incorrect locking?
> +*/
> +static ssize_t write_rbu_data_size(struct kobject *kobj, char *buffer,
> + loff_t pos, size_t count)
80-cols?
> +static ssize_t read_rbu_data(struct kobject *kobj, char *buffer,
> + loff_t pos, size_t count)
> +{
> + unsigned char *ptemp = NULL;
> + int retVal =0;
> + int bytes_left = 0;
> + int data_length = 0;
Unneeded initialisations. Please review all the code for that.
s/retVal/retval/
> + data_length = ((bytes_left > count) ? count : bytes_left);
max()?
> + ptemp = (char *)rbu_data.image_update_buffer;
unneeded typecast
> + memcpy(buffer, (char *)(ptemp + pos), data_length);
unneeded typecast
> + if ( pos > imagesize ) {
Quite a mix of coding styles in this patch!
> + data_length = ((bytes_left > count) ? count : bytes_left);
max()
> + 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;
> +
} else {
retVal = count;
}
(opinions differ...)
> + sysfs_create_bin_file(&rbu_subsys.kset.kobj, &rbudata_attr );
> + sysfs_create_bin_file(&rbu_subsys.kset.kobj, &rbudatasize_attr );
> + sysfs_create_bin_file(&rbu_subsys.kset.kobj, &packetdatasize_attr );
> + sysfs_create_bin_file(&rbu_subsys.kset.kobj, &packetdata_attr );
> + sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &rbudata_attr );
> + sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &rbudatasize_attr );
> + sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &packetdatasize_attr );
> + sysfs_remove_bin_file(&rbu_subsys.kset.kobj, &packetdata_attr );
whitepsace
>
> +config DELL_RBU
> + bool "BIOS update support for DELL systems via sysfs"
> + default n
> + 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 regardign the new image for the image update to
> + take effect.
> +
> + See <file:Documentation/DELL_RBU.txt> for more details on the driver.
> +
Should this feature be available for all architectures, or only for X86 ||
X86_64? (If it compiles OK for other architectures then I'd leave it
as-is, for coverage).
The patch seems to alternate between using hard tabs and using
eight-spaces. Hard tabs are preferred. Please hunt that down and fix it
up.
On Tue, May 10, 2005 at 05:05:20PM -0500, Abhay Salunke wrote:
> +static struct bin_attribute rbudata_attr = {
> + .attr = {.name = "rbudata", .owner = THIS_MODULE, .mode = 0644},
> + .read = read_rbu_data,
> + .write = write_rbu_data,
> +};
> +
> +static struct bin_attribute rbudatasize_attr = {
> + .attr = { .name = "rbudatasize", .owner = THIS_MODULE, .mode = 0644 },
> + .read = read_rbu_data_size,
> + .write= write_rbu_data_size,
> +};
> +
> +static struct bin_attribute packetdatasize_attr = {
> + .attr = { .name = "packetdatasize", .owner = THIS_MODULE, .mode = 0644 },
> + .read = read_packet_data_size,
> + .write= write_packet_data_size,
> +};
> +
> +static struct bin_attribute packetdata_attr = {
> + .attr = { .name = "packetdata", .owner = THIS_MODULE, .mode = 0644 },
> + .read = read_rbu_packet_data,
> + .write= write_rbu_packet_data,
> +};
I can understand having the data use the sysfs binary attribute, but do
not do this for the size files. Please just use a normal attribute for
them, the binary ones are _only_ for blobs of data that are not
interpreted by the kernel.
thanks,
greg k-h
On Tue, 2005-05-10 at 16:59 -0700, Andrew Morton wrote:
> > +
> > + /* free this memory as we need it with in 4GB range */
> > + free_pages ((unsigned long)pbuf, *ordernum);
> > +
> > + /* try allocating a new buffer from the GFP_DMA range
> > + as it is with in 16MB range.*/
> > + pbuf =(unsigned char *)__get_free_pages(GFP_DMA, *ordernum);
> > +
> > + if (pbuf == NULL)
> > + pr_debug("Failed to get memory of size %ld using GFP_DMA\n", size);
> > + }
> > + }
> > + return pbuf;
> > +}
>
> What architecture is this code designed for? On x86 a GFP_KERNEL
> allocation will never return highmem. I guess x86_64?
>
> I assume this code is here because the x86_64 BIOS will only access the
> lower 4GB? If so, a comment to that extent would be useful.
>
> Sometime I expect that x86_64 will gain a new zone, GFP_DMA32 which will be
> guaranteed to return memory below he 4GB point. When that happens, this
> driver should be converted to use it.
Almost always when this happens the driver actually wanted to use
pci_alloc_consistent memory... (i've not seen the rest of the driver yet
so can't say how appropriate that would be here)
On Tue, 2005-05-10 at 17:05 -0500, Abhay Salunke wrote:
> The dell_rbu driver is required for updating BIOS on DELL servers and client
> systems. The driver lets a user application download the BIOS image in to
> contiguous physical memory pages; the driver exposes the memory via sysfs
> filesystem. The update mechanism basically has two approcahes; one by
> allocating contiguous physical memory and the second approach is by allocating
> small chunks of contiguous physical memory.
>
> The patch file is attached to this mail.
I wonder if this wouldn't be able to use the request_firmware()
interface instead... I mean, it would simplify the sysfs code a lot I
suspect.
Le mardi 10 mai 2005 à 17:05 -0500, Abhay Salunke a écrit :
> +2> Download the BIOS image by copying the image file to /sys/firmware/rbudata
> +file.
> +e.g. echo image.hdr > /sys/firmware/rbudata
Don't you mean 'cat' instead of 'echo' there ?
Xav
> -----Original Message-----
> From: Xavier Bestel [mailto:[email protected]]
> Sent: Wednesday, May 11, 2005 4:57 AM
> To: Salunke, Abhay
> Cc: [email protected]; Andrew Morton; Domsch, Matt
> Subject: Re: [patch 2.6.12-rc3] dell_rbu: New Dell BIOS update driver
>
> Le mardi 10 mai 2005 ? 17:05 -0500, Abhay Salunke a ?crit :
>
> > +2> Download the BIOS image by copying the image file to
> /sys/firmware/rbudata
> > +file.
> > +e.g. echo image.hdr > /sys/firmware/rbudata
>
> Don't you mean 'cat' instead of 'echo' there ?
>
> Xav
>
Yes, thanks for correcting this typo.
Abhay
> > +
> > + /* free this memory as we need it with in 4GB
range */
> > + free_pages ((unsigned long)pbuf, *ordernum);
> > +
> > + /* try allocating a new buffer from the GFP_DMA
range
> > + as it is with in 16MB range.*/
> > + pbuf =(unsigned char *)__get_free_pages(GFP_DMA,
> *ordernum);
> > +
> > + if (pbuf == NULL)
> > + pr_debug("Failed to get memory of size
%ld using
> GFP_DMA\n", size);
> > + }
> > + }
> > + return pbuf;
> > +}
>
> What architecture is this code designed for? On x86 a GFP_KERNEL
> allocation will never return highmem. I guess x86_64?
>
> I assume this code is here because the x86_64 BIOS will only access
the
> lower 4GB? If so, a comment to that extent would be useful.
>
> Sometime I expect that x86_64 will gain a new zone, GFP_DMA32 which
will
> be
> guaranteed to return memory below he 4GB point. When that happens,
this
> driver should be converted to use it.
>
This code is for all architectures but primarily is tested on x32, x64
and x86_64.
> > + newpacket->ordernum = ordernum;
> > +
> > + ++rbu_data.num_packets;
> > + /* initialize the newly created packet headers */
> > + INIT_LIST_HEAD(&newpacket->list);
> > + /* add this packet to the link list */
> > + list_add_tail(&newpacket->list, (struct list_head
> *)&packet_data_head);
>
> Does this list not need locking?
create_packet is called from packetize_data which is called with lock
held.
Will add a comment in create_packet.
> > +/*
> > + img_update_free:
> > + Frees the buffer allocated for storing BIOS image
> > + Always called with lock held
> > +*/
> > +static void img_update_free( void)
>
> static void img_update_free(void)
>
> > +{
> > + if (rbu_data.image_update_buffer == NULL)
> > + return;
>
> Can this happen?
Yes, incase some one did an rmmod immediately after an insmod (without
actually updating any BIOS image)
>
> > + rbu_data.image_update_buffer = NULL;
> > + rbu_data.image_update_buffer_size = 0;
> > + rbu_data.bios_image_size = 0;
> > +}
>
> Are these assignments needed?
Yes, all the variables needs to be re-initialized after calling
free_pages.
>
> > +static int img_update_realloc(unsigned long size)
> > +{
> > + unsigned char *image_update_buffer = NULL;
> > + unsigned long rc;
> > + int ordernum =0;
> > +
> > +
> > + /* check if the buffer of sufficient size has been already
allocated
> */
> > + if (rbu_data.image_update_buffer_size >= size) {
> > + /* check for corruption */
> > + if ((size != 0) && (rbu_data.image_update_buffer ==
NULL)) {
> > + pr_debug("img_update_realloc: corruption check
> failed\n");
> > + return -ENOMEM;
>
> ENOMEM seems to be the wrong error to return here.
Changed to EINVAL.
> Should this feature be available for all architectures, or only for
X86 ||
> X86_64? (If it compiles OK for other architectures then I'd leave it
> as-is, for coverage).
>
It supports all architectures.
All the other recommended changed are being worked out and I will
resubmit the patch with the changes.
Thanks,
Abhay Salunke
Software Engineer
Dell Inc.
On Tue, May 10, 2005 at 05:05:20PM -0500, Abhay Salunke wrote:
> The dell_rbu driver is required for updating BIOS on DELL servers and client
> systems. The driver lets a user application download the BIOS image in to
> contiguous physical memory pages; the driver exposes the memory via sysfs
> filesystem. The update mechanism basically has two approcahes; one by
> allocating contiguous physical memory and the second approach is by allocating
> small chunks of contiguous physical memory.
[snip]
> diff -uprN linux-2.6.11.8.ORIG/drivers/firmware/Kconfig linux-2.6.11.8/drivers/firmware/Kconfig
> --- linux-2.6.11.8.ORIG/drivers/firmware/Kconfig 2005-05-09 15:13:06.257801832 -0500
> +++ linux-2.6.11.8/drivers/firmware/Kconfig 2005-05-09 16:30:47.746147888 -0500
> @@ -58,4 +58,16 @@ config EFI_PCDP
>
> See <http://www.dig64.org/specifications/DIG64_HCDPv20_042804.pdf>
>
> +config DELL_RBU
> + bool "BIOS update support for DELL systems via sysfs"
Trivial: needs to be tristate, not bool, to make a module. Change
help text then accordingly.
Thanks,
Matt
--
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com