[PATCH] x86_84 pci_map_sg fix for 2.6.0-test7

[PATCH] x86_84 pci_map_sg fix for 2.6.0-test7

[Badari Pulavarty]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

Hi,

Here is the patch to fix few minor issues with pci_map_sg() and pci_map_cont()
for x86_64. I ran into these asserts while testing with qlogic fc driver.

The patch fixes following:

1) pci_map_sg() coalsces "sg" entries without modifying command's
"use_sg" value. It sets the "sg" entries length to "0" to indicate that
these entires are coalsced. If the command gets retried, the pci_map_sg() 
code trips on the assert that all entries length should be > 0.

2) __pci_map_cont() incorrectly assumes that "start" is always 0, so it
trips on few asserts.

Thanks,
Badari




[-- Attachment #2: pci-gart.patch --]
[-- Type: text/x-diff, Size: 1054 bytes --]

--- linux-2.6.0-test7/arch/x86_64/kernel/pci-gart.c	2003-10-10 11:48:40.324040400 -0700
+++ linux-2.6.0-test7.new/arch/x86_64/kernel/pci-gart.c	2003-10-10 11:49:12.281182176 -0700
@@ -395,7 +395,7 @@ static int __pci_map_cont(struct scatter
 	for (i = start; i < stopat; i++) {
 		struct scatterlist *s = &sg[i];
 		unsigned long start_addr = s->dma_address;
-		BUG_ON(i > 0 && s->offset);
+		BUG_ON(i > start && s->offset);
 		if (i == start) {
 			*sout = *s; 
 			sout->dma_address = iommu_bus_base;
@@ -409,8 +409,8 @@ static int __pci_map_cont(struct scatter
 			SET_LEAK(iommu_page);
 			addr += PAGE_SIZE;
 			iommu_page++;
-	} 
-		BUG_ON(i > 0 && addr % PAGE_SIZE); 
+		} 
+		BUG_ON(i > start && addr % PAGE_SIZE); 
 	} 
 	BUG_ON(iommu_page - iommu_start != pages);	
 	return 0;
@@ -451,7 +451,8 @@ int pci_map_sg(struct pci_dev *dev, stru
 		struct scatterlist *s = &sg[i];
 		dma_addr_t addr = page_to_phys(s->page) + s->offset;
 		s->dma_address = addr;
-		BUG_ON(s->length == 0); 
+		if (s->length == 0)
+			break;
 
 		size += s->length; 
 

Re: [PATCH] x86_84 pci_map_sg fix for 2.6.0-test7

[Andi Kleen]

On Fri, Oct 10, 2003 at 02:26:33PM -0700, Badari Pulavarty wrote:
> Hi,
> 
> Here is the patch to fix few minor issues with pci_map_sg() and pci_map_cont()
> for x86_64. I ran into these asserts while testing with qlogic fc driver.

Sounds like the driver is buggy.

> 
> The patch fixes following:
> 
> 1) pci_map_sg() coalsces "sg" entries without modifying command's
> "use_sg" value. It sets the "sg" entries length to "0" to indicate that
> these entires are coalsced. If the command gets retried, the pci_map_sg() 
> code trips on the assert that all entries length should be > 0.

As I explained for your last patch this change is wrong. Remapping
an already mapped sg is not possible - it leaks IOMMU space and 
cause eventually system failure when the aperture fills up.

You have to somehow handle this in the caller, the pci-dma
layer cannot do it. Eeither free the modified sg and retry with the original 
one or better just don't remap it and use the already mapped one) 

I will add an BUG for passing sgs with dma_address != NULL to 
pci_map_sg(). This should catch such abuses early.

> 
> 2) __pci_map_cont() incorrectly assumes that "start" is always 0, so it
> trips on few asserts.

That's the same high level bug I think.  

-Andi

Re: [PATCH] x86_84 pci_map_sg fix for 2.6.0-test7

[Badari Pulavarty]

On Friday 10 October 2003 04:11 pm, Andi Kleen wrote:
> On Fri, Oct 10, 2003 at 02:26:33PM -0700, Badari Pulavarty wrote:
> > Hi,
> >
> > Here is the patch to fix few minor issues with pci_map_sg() and
> > pci_map_cont() for x86_64. I ran into these asserts while testing with
> > qlogic fc driver.
>
> Sounds like the driver is buggy.

Hmm. Possible. I am not an expert in qlogic driver. Last time I looked
commands are getting retried due to hardware error returns.
(something like underruns). 

>
> > The patch fixes following:
> >
> > 1) pci_map_sg() coalsces "sg" entries without modifying command's
> > "use_sg" value. It sets the "sg" entries length to "0" to indicate that
> > these entires are coalsced. If the command gets retried, the pci_map_sg()
> > code trips on the assert that all entries length should be > 0.
>
> As I explained for your last patch this change is wrong. Remapping
> an already mapped sg is not possible - it leaks IOMMU space and
> cause eventually system failure when the aperture fills up.
>
> You have to somehow handle this in the caller, the pci-dma
> layer cannot do it. Eeither free the modified sg and retry with the
> original one or better just don't remap it and use the already mapped one)
>
> I will add an BUG for passing sgs with dma_address != NULL to
> pci_map_sg(). This should catch such abuses early.

Okay !! I will take a closer look at it. 

> > 2) __pci_map_cont() incorrectly assumes that "start" is always 0, so it
> > trips on few asserts.
>
> That's the same high level bug I think.

pci_map_sg() can call pci_map_cont() with different start values.

Thanks,
Badari