RE: [patch 2.6.12-rc3] dell_rbu: New Dell BIOS update driver

[Abhay_Salunke]

> > +
> > +			/* 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/