Re: [patch 2.6.12-rc3] dell_rbu: Resubmitting patch for new Dell BIOS update driver

Alexey Dobriyan
2005-05-13T21:14:00Z

On Friday 13 May 2005 23:00, Abhay Salunke wrote:
--- linux-2.6.11.8.ORIG/Documentation/DELL_RBU.txt
+++ linux-2.6.11.8/Documentation/DELL_RBU.txt

> +physical memory of packetdatasize and the data is written to that meomry.
^^^^^^
> +Afte updating the BIOS image the appplication needs to communicate with the BIOS
^^^^
> +reboot the system imideately or not reboot the system and leave up to the user
^^^^^^^^^^

--- linux-2.6.11.8.ORIG/drivers/firmware/dell_rbu.c
+++ linux-2.6.11.8/drivers/firmware/dell_rbu.c

> + * Changelog:
> + *
> + * 13 May 2005 Abhay Salunke <Abhay_Salunke@dell.com>
> + * Modified code with suggestions from Andrew Morton;

Maintaining ChangeLogs is the job of SCM. At least don't put my name here. ;-)

> +void init_packet_head(void)

static?

> +static int fill_last_packet(void *data, size_t length)

> + memcpy(((char *)ppacket->data + rbu_data.packet_write_count),

No need for cast and brackets.

> + data, length);

> + /* adjust the total packet length */

Useless comment.

> + rbu_data.packet_write_count += length;

> +static void *get_free_pages_limited(unsigned long size,

> + /* The incoming size is very large */

Useless comment.

> + pr_debug("get_free_pages_limited: Incoming size is very large\n");

> + pbuf =(unsigned char *)__get_free_pages(GFP_KERNEL, *ordernum);

pbuf is void *.

> + img_buf_phys_addr = (unsigned long)virt_to_phys((void *)pbuf);

pbuf is void *.

> + pbuf =(unsigned char *)__get_free_pages(GFP_DMA, *ordernum);

pbuf is void *.

> +static int create_packet(size_t length)

> + 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->data = get_free_pages_limited(rbu_data.packetsize,&ordernum, 0);
> + pr_debug("create_packet: newpacket %p\n", newpacket->data);
> +
> + if(newpacket->data == NULL) {
> + printk(KERN_WARNING"create_packet: failed to allocate new packet\n");
> + return -ENOMEM;

You leak newpacket.

> + /* initialize the newly created packet headers */

Useless comment.

> + INIT_LIST_HEAD(&newpacket->list);
> + /* add this packet to the link list */

Useless comment.

> + list_add_tail(&newpacket->list, &packet_data_head.list);
> + /*
> + * packets are of fixed sizes so initialize
> + * the length to rbu_data.packetsize
> + */

"Packets have fixed size" is enough.

> + newpacket->length = rbu_data.packetsize;

> +static int packetize_data(void *data, size_t length)

> + /* create a new packet */

Useless comment.

> + if ((rc = create_packet(length)) != 0 )
> + return rc;

> +static int packet_read_list(char *data, size_t *pread_length)

> + /* get the current read count */

Useless comment.

> + bytes_read = rbu_data.packet_read_count;

> + ptemp_list = (&packet_data_head.list)->next;
> + while(!list_empty(ptemp_list)) {

> + /* adjust the remaining bytes */

Useless comment.

> + remaining_bytes -= bytes_copied;
> + bytes_read += bytes_copied;
> + /* adjust the data pointer */

Useless comment.

> + pdest += bytes_copied;

> + /* point to the next packet in the list */

Useless comment.

> + ptemp_list = ptemp_list->next;

> +static void packet_empty_list(void)

> + ptemp_list = (&packet_data_head.list)->next;
> + while(!list_empty(ptemp_list)) {
> + newpacket = list_entry(ptemp_list, struct packet_data, list);
> + /* get the next list ptr before we delete this entry */

Useless comment.

> + pnext_list = ptemp_list->next;
> + /* remove the list entry */

Useless comment.

> + list_del(ptemp_list);
> + /* set the list to next */

Useless comment.

> + ptemp_list = pnext_list;
> + /* zero out the RBU packet memory before freeing */
> + memset(newpacket->data, 0, rbu_data.packetsize);

For what? Useless comment, anyway.

> + /* free the memory pointed by this packet */

Useless comment.

> + free_pages((unsigned long)newpacket->data, newpacket->ordernum);
> + /* now free the packet*/

Useless comment.

> + kfree(newpacket);

> +static void img_update_free( void)
^
> + /* zero out this buffer before freeing it */
> + memset(rbu_data.image_update_buffer, 0, rbu_data.image_update_buffer_size);

For what?

> + free_pages((unsigned long)rbu_data.image_update_buffer,
> + rbu_data.image_update_order_number);

> +static int img_update_realloc(unsigned long size)

> + if (image_update_buffer != NULL) {
> + /* store address for the new buffer */

Useless comment.

> + rbu_data.image_update_buffer = image_update_buffer;
> + /* adjust allocated size */

Useless comment.

> + rbu_data.image_update_buffer_size = PAGE_SIZE << ordernum;
> + /* save the current order number */

Useless comment.

> + rbu_data.image_update_order_number = ordernum;
> + /* initialize the new buffer data to 0 */

Useless comment.

> + memset(rbu_data.image_update_buffer,0, rbu_data.image_update_buffer_size);
> + pr_debug("img_update_realloc: success\n");
> + /* success */

Useless comment.

> + rc = 0;

> +static int __init dcdrbu_init(void)

> + printk(KERN_WARNING"dcdrbu_init: firmware_register failed\n");

People usually leave space after KERN_*.

> +static __exit void dcdrbu_exit( void)
^
> + 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 );

Almost trailing whitespace.

--- linux-2.6.11.8.ORIG/drivers/firmware/Kconfig
+++ linux-2.6.11.8/drivers/firmware/Kconfig
> + DELL system. Note you need a supporting application to comunicate
^^^^^^^^^^
> + with the BIOS regardign the new image for the image update to
^^^^^^^^^

Also, try to fit both comments and code in 80 characters, run comments through
spellchecker, use consistent style for comments (
/* foo /* foo
bar 1 */ or * bar 1 */
) is OK.