* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
@ 2016-11-25 12:32 ` Sowmini Varadhan
2016-11-25 13:01 ` Dan Carpenter
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sowmini Varadhan @ 2016-11-25 12:32 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
2016-11-25 12:32 ` Sowmini Varadhan
@ 2016-11-25 13:01 ` Dan Carpenter
2016-11-25 13:11 ` Sowmini Varadhan
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2016-11-25 13:01 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
2016-11-25 12:32 ` Sowmini Varadhan
2016-11-25 13:01 ` Dan Carpenter
@ 2016-11-25 13:11 ` Sowmini Varadhan
2016-11-25 13:46 ` Dan Carpenter
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sowmini Varadhan @ 2016-11-25 13:11 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
` (2 preceding siblings ...)
2016-11-25 13:11 ` Sowmini Varadhan
@ 2016-11-25 13:46 ` Dan Carpenter
2016-11-25 13:57 ` Sowmini Varadhan
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2016-11-25 13:46 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
` (3 preceding siblings ...)
2016-11-25 13:46 ` Dan Carpenter
@ 2016-11-25 13:57 ` Sowmini Varadhan
2016-11-25 20:45 ` Sam Ravnborg
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Sowmini Varadhan @ 2016-11-25 13:57 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
` (4 preceding siblings ...)
2016-11-25 13:57 ` Sowmini Varadhan
@ 2016-11-25 20:45 ` Sam Ravnborg
2016-11-25 21:06 ` tndave
2016-11-25 21:11 ` Dan Carpenter
7 siblings, 0 replies; 9+ messages in thread
From: Sam Ravnborg @ 2016-11-25 20:45 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
` (5 preceding siblings ...)
2016-11-25 20:45 ` Sam Ravnborg
@ 2016-11-25 21:06 ` tndave
2016-11-25 21:11 ` Dan Carpenter
7 siblings, 0 replies; 9+ messages in thread
From: tndave @ 2016-11-25 21:06 UTC (permalink / raw)
To: kernel-janitors
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] sparc64: enable IRQs on error paths
2016-11-25 11:12 [patch] sparc64: enable IRQs on error paths Dan Carpenter
` (6 preceding siblings ...)
2016-11-25 21:06 ` tndave
@ 2016-11-25 21:11 ` Dan Carpenter
7 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2016-11-25 21:11 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread