Messages in this thread | | | Subject | RE: [patch 2.6.12-rc3] dell_rbu: New Dell BIOS update driver | Date | Wed, 11 May 2005 13:21:19 -0500 | From | <> |
| |
> > + > > + /* 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |