Skip to contentSkip to conversation

sparc64: enable IRQs on error paths

PATCH In review Dan Carpenter 4 participants 9 messages Nov 25, 2016

Subsystems (2)
Kernel janitors SPARC + UltraSPARC (sparc/sparc64)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 sparc64: enable IRQs on error paths

There are several error paths where we should enable IRQs but we don't.

Fixes bb620c3d3925 ("sparc: Make sparc64 use scalable lib/iommu-common.c functions")
Signed-off-by Dan Carpenter <[email protected]>

---
Not tested.

arch/sparc/kernel/pci_sun4v.c +12 -4
index 06981cc7..274df7a 100644
@@ -230,12 +230,16 @@ static void *dma_4v_alloc_coherent(struct device *dev, size_t size,
 
 	for (n = 0; n < npages; n++) {
 		long err = iommu_batch_add(first_page + (n * PAGE_SIZE), mask);
-		if (unlikely(err < 0L))
+		if (unlikely(err < 0L)) {
+			local_irq_restore(flags);
 			goto iommu_map_fail;
+		}
 	}
 
-	if (unlikely(iommu_batch_end(mask) < 0L))
+	if (unlikely(iommu_batch_end(mask) < 0L)) {
+		local_irq_restore(flags);
 		goto iommu_map_fail;
+	}
 
 	local_irq_restore(flags);
 

@@ -398,11 +402,15 @@ static dma_addr_t dma_4v_map_page(struct device *dev, struct page *page,
 
 	for (i = 0; i < npages; i++, base_paddr += IO_PAGE_SIZE) {
 		long err = iommu_batch_add(base_paddr, mask);
-		if (unlikely(err < 0L))
+		if (unlikely(err < 0L)) {
+			local_irq_restore(flags);
 			goto iommu_map_fail;
+		}
 	}
-	if (unlikely(iommu_batch_end(mask) < 0L))
+	if (unlikely(iommu_batch_end(mask) < 0L)) {
+		local_irq_restore(flags);
 		goto iommu_map_fail;
+	}
 
 	local_irq_restore(flags);
#1 lore raw
Quoted 4 lines · Dan Carpenter

There are several error paths where we should enable IRQs but we don't.

Fixes bb620c3d3925 ("sparc: Make sparc64 use scalable lib/iommu-common.c functions")
Signed-off-by Dan Carpenter <[email protected]>

I dont have any problem with the fix, but the commit id flagged in "Fixes" is inaccurate- this problem pre-existed before bb620c3d3925.

I think 6a32fd4d is the commit that this Fixes.

--Sowmini

author
#2 lore raw
In reply to Dan Carpenter
Quoted 10 lines · Sowmini Varadhan
Quoted 4 lines · Dan Carpenter

There are several error paths where we should enable IRQs but we don't.

Fixes bb620c3d3925 ("sparc: Make sparc64 use scalable lib/iommu-common.c functions")
Signed-off-by Dan Carpenter <[email protected]>

I dont have any problem with the fix, but the commit id flagged in "Fixes" is inaccurate- this problem pre-existed before bb620c3d3925.

I think 6a32fd4d is the commit that this Fixes.

I'm pretty sure the Fixes tag is correct. Originally it did:

+iommu_map_fail:

+       /* Interrupts are disabled.  */
+       spin_lock(&iommu->lock);
+       pci_arena_free(&iommu->arena, entry, npages);
+       spin_unlock_irqrestore(&iommu->lock, flags);

+

So it enabled the Interrupts. The other patch removed the _irqrestore().

regards,
dan carpenter

#3 lore raw
In reply to Dan Carpenter

Review

Quoted 8 lines · Dan Carpenter

I'm pretty sure the Fixes tag is correct. Originally it did:

(quoted diff) +6 -0
+iommu_map_fail:
+       /* Interrupts are disabled.  */
+       spin_lock(&iommu->lock);
+       pci_arena_free(&iommu->arena, entry, npages);
+       spin_unlock_irqrestore(&iommu->lock, flags);
+

In that case why dont you just do the local_irq_restore once at the iommu_map_fail label?

--Sowmini

author
#4 lore raw
In reply to Dan Carpenter
Quoted 12 lines · Sowmini Varadhan

Review

Quoted 8 lines · Dan Carpenter

I'm pretty sure the Fixes tag is correct. Originally it did:

(quoted diff) +6 -0
+iommu_map_fail:
+       /* Interrupts are disabled.  */
+       spin_lock(&iommu->lock);
+       pci_arena_free(&iommu->arena, entry, npages);
+       spin_unlock_irqrestore(&iommu->lock, flags);
+

In that case why dont you just do the local_irq_restore once at the iommu_map_fail label?

I thought this way was easier to read. It doesn't really matter?

regards,
dan carpenter

#5 lore raw
In reply to Dan Carpenter
Quoted 4 lines · Dan Carpenter
Quoted 2 lines

In that case why dont you just do the local_irq_restore once at the iommu_map_fail label?

I thought this way was easier to read. It doesn't really matter?

I suppose.

--Sowmini

#6 lore raw
In reply to Dan Carpenter

Hi Dan.

Quoted 4 lines · Dan Carpenter

There are several error paths where we should enable IRQs but we don't.

Fixes bb620c3d3925 ("sparc: Make sparc64 use scalable lib/iommu-common.c functions")
Signed-off-by Dan Carpenter <[email protected]>

Please use a more descriptive subject such as:
sparc64: restore irq in error paths in iommu

Quoted 8 lines

---
Not tested.

arch/sparc/kernel/pci_sun4v.c +0 -0
index 06981cc7..274df7a 100644

arch/sparc/kernel/pci_sun4v.c static void *dma_4v_alloc_coherent(struct device *dev, size_t size, · line 237

Quoted 9 lines
arch/sparc/kernel/pci_sun4v.c +3 -1
@@ -230,12 +230,16 @@ static void *dma_4v_alloc_coherent(struct device *dev, size_t size,
 
 	for (n = 0; n < npages; n++) {
 		long err = iommu_batch_add(first_page + (n * PAGE_SIZE), mask);
-		if (unlikely(err < 0L))
+		if (unlikely(err < 0L)) {
+			local_irq_restore(flags);
 			goto iommu_map_fail;
+		}
 	}

Locate all cleanup after the iommu_map_fail label.

As it is now the irq_restore is on the error site
and the free() is at the error label.

It is very confusing that half of the recovery is in one place and the other in another place.

Sam

#7 lore raw
In reply to Dan Carpenter
Quoted 20 lines · Sam Ravnborg

Hi Dan.

Quoted 4 lines · Dan Carpenter

There are several error paths where we should enable IRQs but we don't.

Fixes bb620c3d3925 ("sparc: Make sparc64 use scalable lib/iommu-common.c functions")
Signed-off-by Dan Carpenter <[email protected]>

Please use a more descriptive subject such as:
sparc64: restore irq in error paths in iommu

Quoted 9 lines

---
Not tested.

arch/sparc/kernel/pci_sun4v.c +0 -0
index 06981cc7..274df7a 100644
@@ -230,12 +230,16 @@ static void *dma_4v_alloc_coherent(struct device *dev, size_t size,
Quoted 10 lines

arch/sparc/kernel/pci_sun4v.c static void *dma_4v_alloc_coherent(struct device *dev, size_t size, · line 237

Quoted 8 lines
arch/sparc/kernel/pci_sun4v.c +3 -1
 	for (n = 0; n < npages; n++) {
 		long err = iommu_batch_add(first_page + (n * PAGE_SIZE), mask);
-		if (unlikely(err < 0L))
+		if (unlikely(err < 0L)) {
+			local_irq_restore(flags);
 			goto iommu_map_fail;
+		}
 	}

Locate all cleanup after the iommu_map_fail label.

+1

-Tushar

Quoted 9 lines

As it is now the irq_restore is on the error site
and the free() is at the error label.

It is very confusing that half of the recovery is in one place and the other in another place.

Sam

author
#8 lore raw
In reply to Dan Carpenter
Quoted 11 lines · Sam Ravnborg

Hi Dan.

Quoted 4 lines · Dan Carpenter

There are several error paths where we should enable IRQs but we don't.

Fixes bb620c3d3925 ("sparc: Make sparc64 use scalable lib/iommu-common.c functions")
Signed-off-by Dan Carpenter <[email protected]>

Please use a more descriptive subject such as:
sparc64: restore irq in error paths in iommu

Is this really "in iommu"?

Quoted 20 lines
Quoted 8 lines

---
Not tested.

arch/sparc/kernel/pci_sun4v.c +0 -0
index 06981cc7..274df7a 100644

arch/sparc/kernel/pci_sun4v.c static void *dma_4v_alloc_coherent(struct device *dev, size_t size, · line 237

Quoted 9 lines
arch/sparc/kernel/pci_sun4v.c +3 -1
@@ -230,12 +230,16 @@ static void *dma_4v_alloc_coherent(struct device *dev, size_t size,
 
 	for (n = 0; n < npages; n++) {
 		long err = iommu_batch_add(first_page + (n * PAGE_SIZE), mask);
-		if (unlikely(err < 0L))
+		if (unlikely(err < 0L)) {
+			local_irq_restore(flags);
 			goto iommu_map_fail;
+		}
 	}

Locate all cleanup after the iommu_map_fail label.

Quoted 5 lines

As it is now the irq_restore is on the error site
and the free() is at the error label.

It is very confusing that half of the recovery is in one place and the other in another place.

My way is the correct, more elegant way. When you're allocating things you do:

	a = alloc();
	b = alloc();
	c = alloc();

	...

	return 0;

free_c:

	free(c);
free_b:
	free(b);
free_a:
	free(a);

	return ret;

Those things build on each other. We allocate and then free symetrically. The locks don't build on each other, we take them and then release them as needed. It's just chance that we are holding the locks for both "goto iommu_map_fail;" statements. If we added to this function we would have to move the unlocks back out of the release path.

The original code was written the way you suggest and it was too confusing so we introduced a bug. It was *even commented* but it was still too confusing. Best to keep all the locking together.

To be honest, I don't care. I'll send a v2.

regards,
dan carpenter