[patch] sparc64: enable IRQs on error paths

[patch] sparc64: enable IRQs on error paths

[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 <dan.carpenter@oracle.com>
---
Not tested.

diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
index 06981cc7..274df7a 100644
--- a/arch/sparc/kernel/pci_sun4v.c
+++ b/arch/sparc/kernel/pci_sun4v.c
@@ -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);
 

Re: [patch] sparc64: enable IRQs on error paths

[Sowmini Varadhan]

On (11/25/16 14:12), Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>

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

Re: [patch] sparc64: enable IRQs on error paths

[Dan Carpenter]

On Fri, Nov 25, 2016 at 07:32:28AM -0500, Sowmini Varadhan wrote:
> On (11/25/16 14:12), Dan Carpenter wrote:
> > 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 <dan.carpenter@oracle.com>
> 
> 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

Re: [patch] sparc64: enable IRQs on error paths

[Sowmini Varadhan]

On (11/25/16 16:01), Dan Carpenter wrote:
> 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);
> +

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

--Sowmini

Re: [patch] sparc64: enable IRQs on error paths

[Dan Carpenter]

On Fri, Nov 25, 2016 at 08:11:44AM -0500, Sowmini Varadhan wrote:
> On (11/25/16 16:01), Dan Carpenter wrote:
> > 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);
> > +
> 
> 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

Re: [patch] sparc64: enable IRQs on error paths

[Sowmini Varadhan]

On (11/25/16 16:46), Dan Carpenter wrote:
> > 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

Re: [patch] sparc64: enable IRQs on error paths

[Sam Ravnborg]

Hi Dan.

On Fri, Nov 25, 2016 at 02:12:51PM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>

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

> ---
> Not tested.
> 
> diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
> index 06981cc7..274df7a 100644
> --- a/arch/sparc/kernel/pci_sun4v.c
> +++ b/arch/sparc/kernel/pci_sun4v.c
> @@ -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

Re: [patch] sparc64: enable IRQs on error paths

[tndave]

On 11/25/2016 12:45 PM, Sam Ravnborg wrote:
> Hi Dan.
>
> On Fri, Nov 25, 2016 at 02:12:51PM +0300, Dan Carpenter wrote:
>> 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 <dan.carpenter@oracle.com>
>
> Please use a more descriptive subject such as:
> sparc64: restore irq in error paths in iommu
>
>> ---
>> Not tested.
>>
>> diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
>> index 06981cc7..274df7a 100644
>> --- a/arch/sparc/kernel/pci_sun4v.c
>> +++ b/arch/sparc/kernel/pci_sun4v.c
>> @@ -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.
+1

-Tushar
>
> 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
>

Re: [patch] sparc64: enable IRQs on error paths

[Dan Carpenter]

On Fri, Nov 25, 2016 at 09:45:24PM +0100, Sam Ravnborg wrote:
> Hi Dan.
> 
> On Fri, Nov 25, 2016 at 02:12:51PM +0300, Dan Carpenter wrote:
> > 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 <dan.carpenter@oracle.com>
> 
> Please use a more descriptive subject such as:
> sparc64: restore irq in error paths in iommu
>

Is this really "in iommu"?

> > ---
> > Not tested.
> > 
> > diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
> > index 06981cc7..274df7a 100644
> > --- a/arch/sparc/kernel/pci_sun4v.c
> > +++ b/arch/sparc/kernel/pci_sun4v.c
> > @@ -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.

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