From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Leizhen (ThunderTown)" Subject: Re: [PATCH 1/7] iommu/dma: fix trival coding style mistake Date: Mon, 4 Jun 2018 19:05:30 +0800 Message-ID: <5B151CFA.10808@huawei.com> References: <1527752569-18020-1-git-send-email-thunder.leizhen@huawei.com> <1527752569-18020-2-git-send-email-thunder.leizhen@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Robin Murphy , Will Deacon , Matthias Brugger , Rob Clark , Joerg Roedel , linux-mediatek , linux-arm-msm , linux-arm-kernel , iommu , linux-kernel Cc: Xinwei Hu , Guozhu Li , Libin , Hanjun Guo List-Id: linux-arm-msm@vger.kernel.org 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 >> --- >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: thunder.leizhen@huawei.com (Leizhen (ThunderTown)) Date: Mon, 4 Jun 2018 19:05:30 +0800 Subject: [PATCH 1/7] iommu/dma: fix trival coding style mistake In-Reply-To: References: <1527752569-18020-1-git-send-email-thunder.leizhen@huawei.com> <1527752569-18020-2-git-send-email-thunder.leizhen@huawei.com> Message-ID: <5B151CFA.10808@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> --- >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752545AbeFDLGP (ORCPT ); Mon, 4 Jun 2018 07:06:15 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:8631 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752062AbeFDLGN (ORCPT ); Mon, 4 Jun 2018 07:06:13 -0400 Subject: Re: [PATCH 1/7] iommu/dma: fix trival coding style mistake To: Robin Murphy , Will Deacon , Matthias Brugger , Rob Clark , Joerg Roedel , linux-mediatek , linux-arm-msm , linux-arm-kernel , iommu , linux-kernel References: <1527752569-18020-1-git-send-email-thunder.leizhen@huawei.com> <1527752569-18020-2-git-send-email-thunder.leizhen@huawei.com> CC: Hanjun Guo , Libin , "Guozhu Li" , Xinwei Hu From: "Leizhen (ThunderTown)" Message-ID: <5B151CFA.10808@huawei.com> Date: Mon, 4 Jun 2018 19:05:30 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.23.164] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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