amd/iommu: add missing unlock in iommu_read_log

[Xen-devel] [PATCH] amd/iommu: add missing unlock in iommu_read_log

Posted by Roger Pau Monne 5 years, 11 months ago

Reported-by: Coverity
CID: 1458632
Fixes: 709d3ddea2d5e ('AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/amd/iommu_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 4c86848c52..e93a090830 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -338,6 +338,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
             {
                 AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
                                 log == &iommu->event_log ? "Event" : "PPR");
+                spin_unlock(&log->lock);
                 return 0;
             }
             udelay(1);
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] amd/iommu: add missing unlock in iommu_read_log

Posted by Andrew Cooper 5 years, 11 months ago

On 19/02/2020 11:19, Roger Pau Monne wrote:
> Reported-by: Coverity
> CID: 1458632

We tend to use just Coverity-ID: 1458632

> Fixes: 709d3ddea2d5e ('AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/drivers/passthrough/amd/iommu_init.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index 4c86848c52..e93a090830 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -338,6 +338,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
>              {
>                  AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
>                                  log == &iommu->event_log ? "Event" : "PPR");
> +                spin_unlock(&log->lock);

A goto out would be cleaner.  Can fix up on commit if you're happy?

~Andrew

>                  return 0;
>              }
>              udelay(1);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] amd/iommu: add missing unlock in iommu_read_log

Posted by Roger Pau Monné 5 years, 11 months ago

On Wed, Feb 19, 2020 at 11:23:40AM +0000, Andrew Cooper wrote:
> On 19/02/2020 11:19, Roger Pau Monne wrote:
> > Reported-by: Coverity
> > CID: 1458632
> 
> We tend to use just Coverity-ID: 1458632

Oh, I got confused with FreeBSD usage of CID.

> 
> > Fixes: 709d3ddea2d5e ('AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log()')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/drivers/passthrough/amd/iommu_init.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> > index 4c86848c52..e93a090830 100644
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -338,6 +338,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
> >              {
> >                  AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
> >                                  log == &iommu->event_log ? "Event" : "PPR");
> > +                spin_unlock(&log->lock);
> 
> A goto out would be cleaner.  Can fix up on commit if you're happy?

That's fine, I don't have a preference. In such cases where a simple
unlock is required I tend to avoid the label as I know Jan prefers
avoiding them.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] amd/iommu: add missing unlock in iommu_read_log

Posted by Jan Beulich 5 years, 11 months ago

On 19.02.2020 12:32, Roger Pau Monné wrote:
> On Wed, Feb 19, 2020 at 11:23:40AM +0000, Andrew Cooper wrote:
>> On 19/02/2020 11:19, Roger Pau Monne wrote:
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -338,6 +338,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
>>>              {
>>>                  AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
>>>                                  log == &iommu->event_log ? "Event" : "PPR");
>>> +                spin_unlock(&log->lock);
>>
>> A goto out would be cleaner.  Can fix up on commit if you're happy?
> 
> That's fine, I don't have a preference. In such cases where a simple
> unlock is required I tend to avoid the label as I know Jan prefers
> avoiding them.

I appreciate this, yet I accept others thinking differently. The
only case where I outright object to "goto" is if the target
simply does "return" or some such. Centralizing unlocking in a
single place is acceptable to me, albeit perhaps not preferred.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel