Re: [discuss] Re: 32-bit dma allocations on 64-bit platforms

[Takashi Iwai <tiwai@suse.de>]

At Fri, 25 Jun 2004 19:30:46 +0200,
Andrea Arcangeli wrote:
> 
> On Fri, Jun 25, 2004 at 05:50:04PM +0200, Takashi Iwai wrote:
> > --- linux-2.6.7/arch/i386/kernel/pci-dma.c-dist	2004-06-24 15:56:46.017473544 +0200
> > +++ linux-2.6.7/arch/i386/kernel/pci-dma.c	2004-06-25 17:43:42.509366917 +0200
> > @@ -23,11 +23,22 @@ void *dma_alloc_coherent(struct device *
> >  	if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
> >  		gfp |= GFP_DMA;
> >  
> > + again:
> >  	ret = (void *)__get_free_pages(gfp, get_order(size));
> >  
> > -	if (ret != NULL) {
> > +	if (ret == NULL) {
> > +		if (dev && (gfp & GFP_DMA)) {
> > +			gfp &= ~GFP_DMA;
> 
> I would find cleaner to use __GFP_DMA in the whole file, this is not
> about your changes, previous code used GFP_DMA too. The issue is that if
> we change GFP_DMA to add a __GFP_HIGH or similar, the above will clear
> the other bitflags too.

Indeed.

> 
> > +		    (((unsigned long)*dma_handle + size - 1) & ~(unsigned long)dev->coherent_dma_mask)) {
> > +			free_pages((unsigned long)ret, get_order(size));
> > +			return NULL;
> > +		}
> 
> I would do the memset and setting of dma_handle after the above check.

Yep.  The below is the corrected version.


Thanks!

Takashi


--- linux-2.6.7/arch/i386/kernel/pci-dma.c-dist	2004-06-24 15:56:46.017473544 +0200
+++ linux-2.6.7/arch/i386/kernel/pci-dma.c	2004-06-25 19:38:26.334210809 +0200
@@ -21,13 +21,24 @@ void *dma_alloc_coherent(struct device *
 	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM);
 
 	if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
-		gfp |= GFP_DMA;
+		gfp |= __GFP_DMA;
 
+ again:
 	ret = (void *)__get_free_pages(gfp, get_order(size));
 
-	if (ret != NULL) {
-		memset(ret, 0, size);
+	if (ret == NULL) {
+		if (dev && (gfp & __GFP_DMA)) {
+			gfp &= ~__GFP_DMA;
+			goto again;
+		}
+	} else {
 		*dma_handle = virt_to_phys(ret);
+		if (!(gfp & __GFP_DMA) &&
+		    (((unsigned long)*dma_handle + size - 1) & ~(unsigned long)dev->coherent_dma_mask)) {
+			free_pages((unsigned long)ret, get_order(size));
+			return NULL;
+		}
+		memset(ret, 0, size);
 	}
 	return ret;
 }