[1/7] iommu/dma: fix trival coding style mistake

Message ID 1527752569-18020-2-git-send-email-thunder.leizhen@huawei.com
State New
Headers show
Series
  • [1/7] iommu/dma: fix trival coding style mistake
Related show

Commit Message

Zhen Lei May 31, 2018, 7:42 a.m.
The static function iova_reserve_iommu_regions is only called by function
iommu_dma_init_domain, and the 'if (!dev)' check in iommu_dma_init_domain
affect it only, so we can safely move the check into it. I think it looks
more natural.

In addition, the local variable 'ret' is only assigned in the branch of
'if (region->type == IOMMU_RESV_MSI)', so the 'if (ret)' should also only
take effect in the branch, add a brace to enclose it.

No functional changes.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

---
 drivers/iommu/dma-iommu.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
1.8.3


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Robin Murphy May 31, 2018, 1:03 p.m. | #1
On 31/05/18 08:42, Zhen Lei wrote:
> The static function iova_reserve_iommu_regions is only called by function

> iommu_dma_init_domain, and the 'if (!dev)' check in iommu_dma_init_domain

> affect it only, so we can safely move the check into it. I think it looks

> more natural.


As before, I disagree - the logic of iommu_dma_init_domain() is "we 
expect to have a valid device, but stop here if we don't", and moving 
the check just needlessly obfuscates that. It is not a coincidence that 
the arguments of both functions are in effective order of importance.

> In addition, the local variable 'ret' is only assigned in the branch of

> 'if (region->type == IOMMU_RESV_MSI)', so the 'if (ret)' should also only

> take effect in the branch, add a brace to enclose it.


'ret' is clearly also assigned at its declaration, to cover the (very 
likely) case where we don't enter the loop at all. Thus testing it in 
the loop is harmless, and cluttering that up with extra tabs and braces 
is just noise.

Robin.

> No functional changes.

> 

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

> ---

>   drivers/iommu/dma-iommu.c | 12 +++++++-----

>   1 file changed, 7 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

> index ddcbbdb..4e885f7 100644

> --- a/drivers/iommu/dma-iommu.c

> +++ b/drivers/iommu/dma-iommu.c

> @@ -231,6 +231,9 @@ static int iova_reserve_iommu_regions(struct device *dev,

>   	LIST_HEAD(resv_regions);

>   	int ret = 0;

>   

> +	if (!dev)

> +		return 0;

> +

>   	if (dev_is_pci(dev))

>   		iova_reserve_pci_windows(to_pci_dev(dev), iovad);

>   

> @@ -246,11 +249,12 @@ static int iova_reserve_iommu_regions(struct device *dev,

>   		hi = iova_pfn(iovad, region->start + region->length - 1);

>   		reserve_iova(iovad, lo, hi);

>   

> -		if (region->type == IOMMU_RESV_MSI)

> +		if (region->type == IOMMU_RESV_MSI) {

>   			ret = cookie_init_hw_msi_region(cookie, region->start,

>   					region->start + region->length);

> -		if (ret)

> -			break;

> +			if (ret)

> +				break;

> +		}

>   	}

>   	iommu_put_resv_regions(dev, &resv_regions);

>   

> @@ -308,8 +312,6 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,

>   	}

>   

>   	init_iova_domain(iovad, 1UL << order, base_pfn);

> -	if (!dev)

> -		return 0;

>   

>   	return iova_reserve_iommu_regions(dev, domain);

>   }

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhen Lei June 4, 2018, 11:05 a.m. | #2
On 2018/5/31 21:03, Robin Murphy wrote:
> On 31/05/18 08:42, Zhen Lei wrote:

>> The static function iova_reserve_iommu_regions is only called by function

>> iommu_dma_init_domain, and the 'if (!dev)' check in iommu_dma_init_domain

>> affect it only, so we can safely move the check into it. I think it looks

>> more natural.

> 

> As before, I disagree - the logic of iommu_dma_init_domain() is "we expect to have a valid device, but stop here if we don't", and moving the check just needlessly obfuscates that. It is not a coincidence that the arguments of both functions are in effective order of importance.

OK

> 

>> In addition, the local variable 'ret' is only assigned in the branch of

>> 'if (region->type == IOMMU_RESV_MSI)', so the 'if (ret)' should also only

>> take effect in the branch, add a brace to enclose it.

> 

> 'ret' is clearly also assigned at its declaration, to cover the (very likely) case where we don't enter the loop at all. Thus testing it in the loop is harmless, and cluttering that up with extra tabs and braces is just noise.

OK, I will drop this patch in v2

> 

> Robin.

> 

>> No functional changes.

>>

>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

>> ---

>>   drivers/iommu/dma-iommu.c | 12 +++++++-----

>>   1 file changed, 7 insertions(+), 5 deletions(-)

>>

>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

>> index ddcbbdb..4e885f7 100644

>> --- a/drivers/iommu/dma-iommu.c

>> +++ b/drivers/iommu/dma-iommu.c

>> @@ -231,6 +231,9 @@ static int iova_reserve_iommu_regions(struct device *dev,

>>       LIST_HEAD(resv_regions);

>>       int ret = 0;

>>   +    if (!dev)

>> +        return 0;

>> +

>>       if (dev_is_pci(dev))

>>           iova_reserve_pci_windows(to_pci_dev(dev), iovad);

>>   @@ -246,11 +249,12 @@ static int iova_reserve_iommu_regions(struct device *dev,

>>           hi = iova_pfn(iovad, region->start + region->length - 1);

>>           reserve_iova(iovad, lo, hi);

>>   -        if (region->type == IOMMU_RESV_MSI)

>> +        if (region->type == IOMMU_RESV_MSI) {

>>               ret = cookie_init_hw_msi_region(cookie, region->start,

>>                       region->start + region->length);

>> -        if (ret)

>> -            break;

>> +            if (ret)

>> +                break;

>> +        }

>>       }

>>       iommu_put_resv_regions(dev, &resv_regions);

>>   @@ -308,8 +312,6 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,

>>       }

>>         init_iova_domain(iovad, 1UL << order, base_pfn);

>> -    if (!dev)

>> -        return 0;

>>         return iova_reserve_iommu_regions(dev, domain);

>>   }

>>

> 

> .

> 


-- 
Thanks!
BestRegards

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index ddcbbdb..4e885f7 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -231,6 +231,9 @@  static int iova_reserve_iommu_regions(struct device *dev,
 	LIST_HEAD(resv_regions);
 	int ret = 0;
 
+	if (!dev)
+		return 0;
+
 	if (dev_is_pci(dev))
 		iova_reserve_pci_windows(to_pci_dev(dev), iovad);
 
@@ -246,11 +249,12 @@  static int iova_reserve_iommu_regions(struct device *dev,
 		hi = iova_pfn(iovad, region->start + region->length - 1);
 		reserve_iova(iovad, lo, hi);
 
-		if (region->type == IOMMU_RESV_MSI)
+		if (region->type == IOMMU_RESV_MSI) {
 			ret = cookie_init_hw_msi_region(cookie, region->start,
 					region->start + region->length);
-		if (ret)
-			break;
+			if (ret)
+				break;
+		}
 	}
 	iommu_put_resv_regions(dev, &resv_regions);
 
@@ -308,8 +312,6 @@  int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	}
 
 	init_iova_domain(iovad, 1UL << order, base_pfn);
-	if (!dev)
-		return 0;
 
 	return iova_reserve_iommu_regions(dev, domain);
 }