[PATCH 1/2] iommu/vt-d: break the for loop if an empty ir_ioapic entry found

[PATCH 1/2] iommu/vt-d: break the for loop if an empty ir_ioapic entry found

[Baoquan He]

No need to continue the loop after it has been found.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/intel_irq_remapping.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 9ec4e0d..37b93f5 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -841,8 +841,10 @@ static int ir_parse_one_ioapic_scope(struct acpi_dmar_device_scope *scope,
 		if (ir_ioapic[count].iommu == iommu &&
 		    ir_ioapic[count].id == scope->enumeration_id)
 			return 0;
-		else if (ir_ioapic[count].iommu == NULL && free == -1)
+		else if (ir_ioapic[count].iommu == NULL && free == -1) {
 			free = count;
+			break;
+		}
 	}
 	if (free == -1) {
 		pr_warn("Exceeded Max IO APICS\n");
-- 
2.1.0

[PATCH 2/2] iommu/vt-d: Adjsut the return value of the parse_ioapics_under_ir

[Baoquan He]

Adjust the return value of parse_ioapics_under_ir as "-1" representing
failure and "0" representing succcess. Just make it consistent with other
function implementation, and we can judge if calling is successfull by
if (!parse_ioapics_under_ir()) style.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/intel_irq_remapping.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 37b93f5..0f441b7 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -672,7 +672,7 @@ static int __init intel_prepare_irq_remapping(void)
 	if (!dmar_ir_support())
 		return -ENODEV;
 
-	if (parse_ioapics_under_ir() != 1) {
+	if (!parse_ioapics_under_ir()) {
 		pr_info("Not enabling interrupt remapping\n");
 		goto error;
 	}
@@ -918,7 +918,7 @@ static int __init parse_ioapics_under_ir(void)
 		}
 
 	if (!ir_supported)
-		return 0;
+		return -1;
 
 	for (ioapic_idx = 0; ioapic_idx < nr_ioapics; ioapic_idx++) {
 		int ioapic_id = mpc_ioapic_id(ioapic_idx);
@@ -930,7 +930,7 @@ static int __init parse_ioapics_under_ir(void)
 		}
 	}
 
-	return 1;
+	return 0;
 }
 
 static int __init ir_dev_scope_init(void)
-- 
2.1.0

Re: [PATCH 1/2] iommu/vt-d: break the for loop if an empty ir_ioapic entry found

[Joerg Roedel]

On Tue, Sep 29, 2015 at 03:26:08PM +0800, Baoquan He wrote:
> No need to continue the loop after it has been found.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  drivers/iommu/intel_irq_remapping.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> index 9ec4e0d..37b93f5 100644
> --- a/drivers/iommu/intel_irq_remapping.c
> +++ b/drivers/iommu/intel_irq_remapping.c
> @@ -841,8 +841,10 @@ static int ir_parse_one_ioapic_scope(struct acpi_dmar_device_scope *scope,
>  		if (ir_ioapic[count].iommu == iommu &&
>  		    ir_ioapic[count].id == scope->enumeration_id)
>  			return 0;
> -		else if (ir_ioapic[count].iommu == NULL && free == -1)
> +		else if (ir_ioapic[count].iommu == NULL && free == -1) {
>  			free = count;
> +			break;
> +		}
>  	}

The purpose of the loop is also to avoid duplicate entries, so we need
to search it till the end, no?



	Joerg

Re: [PATCH 2/2] iommu/vt-d: Adjsut the return value of the parse_ioapics_under_ir

[Joerg Roedel]

On Tue, Sep 29, 2015 at 03:26:09PM +0800, Baoquan He wrote:
> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> index 37b93f5..0f441b7 100644
> --- a/drivers/iommu/intel_irq_remapping.c
> +++ b/drivers/iommu/intel_irq_remapping.c
> @@ -672,7 +672,7 @@ static int __init intel_prepare_irq_remapping(void)
>  	if (!dmar_ir_support())
>  		return -ENODEV;
>  
> -	if (parse_ioapics_under_ir() != 1) {
> +	if (!parse_ioapics_under_ir()) {
>  		pr_info("Not enabling interrupt remapping\n");
>  		goto error;
>  	}
> @@ -918,7 +918,7 @@ static int __init parse_ioapics_under_ir(void)
>  		}
>  
>  	if (!ir_supported)
> -		return 0;
> +		return -1;
>  
>  	for (ioapic_idx = 0; ioapic_idx < nr_ioapics; ioapic_idx++) {
>  		int ioapic_id = mpc_ioapic_id(ioapic_idx);
> @@ -930,7 +930,7 @@ static int __init parse_ioapics_under_ir(void)
>  		}
>  	}
>  
> -	return 1;
> +	return 0;

Looks good, but -ENODEV is probably a better return value than -1.


	Joerg

Re: [PATCH 1/2] iommu/vt-d: break the for loop if an empty ir_ioapic entry found

[Baoquan He]

On 09/29/15 at 03:50pm, Joerg Roedel wrote:
> On Tue, Sep 29, 2015 at 03:26:08PM +0800, Baoquan He wrote:
> > No need to continue the loop after it has been found.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  drivers/iommu/intel_irq_remapping.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> > index 9ec4e0d..37b93f5 100644
> > --- a/drivers/iommu/intel_irq_remapping.c
> > +++ b/drivers/iommu/intel_irq_remapping.c
> > @@ -841,8 +841,10 @@ static int ir_parse_one_ioapic_scope(struct acpi_dmar_device_scope *scope, > >  		if (ir_ioapic[count].iommu == iommu &&
> >  		    ir_ioapic[count].id == scope->enumeration_id)
> >  			return 0;
> > -		else if (ir_ioapic[count].iommu == NULL && free == -1)
> > +		else if (ir_ioapic[count].iommu == NULL && free == -1) {
> >  			free = count;
> > +			break;
> > +		}
> >  	}
> 
> The purpose of the loop is also to avoid duplicate entries, so we need
> to search it till the end, no?

Ah, yes, you are right, it's on purpose. So I will drop this one
and update the 2/2 only with your suggestion.