> > +
> > + /* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
