x86_84 pci_map_sg fix for 2.6.0-test7

PATCH In review Badari Pulavarty 2 participants 3 messages Oct 10, 2003

lore

Share & tools

lore

Badari Pulavarty

author

2003-10-10 21:27 UTC #0 lore raw

PATCH 1/1 x86_84 pci_map_sg fix for 2.6.0-test7

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

arch/x86_64/kernel/pci-gart.c +5 -4

@@ -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;

Andi Kleen

2003-10-10 23:12 UTC #1 lore raw

Quoted 4 lines · Badari Pulavarty

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.

Quoted 7 lines

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.

Quoted 3 lines

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

Badari Pulavarty

author

2003-10-10 23:38 UTC #2 lore raw

Quoted 8 lines · Andi Kleen

Quoted 5 lines · Badari Pulavarty
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).

Quoted 12 lines

Quoted 7 lines
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.

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines
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