From: "Leizhen (ThunderTown)" <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
To: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>,
Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>,
Matthias Brugger
<matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>,
linux-mediatek
<linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
linux-arm-msm
<linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
linux-arm-kernel
<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
iommu
<iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>,
linux-kernel
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Xinwei Hu <huxinwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>,
Guozhu Li <liguozhu-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>,
Libin <huawei.libin-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>,
Hanjun Guo <guohanjun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH 1/7] iommu/dma: fix trival coding style mistake
Date: Mon, 4 Jun 2018 19:05:30 +0800 [thread overview]
Message-ID: <5B151CFA.10808@huawei.com> (raw)
In-Reply-To: <b473059d-fee6-1627-4749-192771038741-5wv7dgnIgG8@public.gmane.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 <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com>
To: Robin Murphy <robin.murphy@arm.com>,
Will Deacon <will.deacon@arm.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Rob Clark <robdclark@gmail.com>, Joerg Roedel <joro@8bytes.org>,
linux-mediatek <linux-mediatek@lists.infradead.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
iommu <iommu@lists.linux-foundation.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Cc: Hanjun Guo <guohanjun@huawei.com>,
Libin <huawei.libin@huawei.com>,
"Guozhu Li" <liguozhu@hisilicon.com>,
Xinwei Hu <huxinwei@huawei.com>
Subject: Re: [PATCH 1/7] iommu/dma: fix trival coding style mistake
Date: Mon, 4 Jun 2018 19:05:30 +0800 [thread overview]
Message-ID: <5B151CFA.10808@huawei.com> (raw)
In-Reply-To: <b473059d-fee6-1627-4749-192771038741@arm.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: thunder.leizhen@huawei.com (Leizhen (ThunderTown))
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/7] iommu/dma: fix trival coding style mistake
Date: Mon, 4 Jun 2018 19:05:30 +0800 [thread overview]
Message-ID: <5B151CFA.10808@huawei.com> (raw)
In-Reply-To: <b473059d-fee6-1627-4749-192771038741@arm.com>
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
next prev parent reply other threads:[~2018-06-04 11:05 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-31 7:42 [PATCH 0/7] add non-strict mode support for arm-smmu-v3 Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 7:42 ` [PATCH 1/7] iommu/dma: fix trival coding style mistake Zhen Lei
2018-05-31 7:42 ` Zhen Lei
[not found] ` <1527752569-18020-2-git-send-email-thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2018-05-31 13:03 ` Robin Murphy
2018-05-31 13:03 ` Robin Murphy
2018-05-31 13:03 ` Robin Murphy
[not found] ` <b473059d-fee6-1627-4749-192771038741-5wv7dgnIgG8@public.gmane.org>
2018-06-04 11:05 ` Leizhen (ThunderTown) [this message]
2018-06-04 11:05 ` Leizhen (ThunderTown)
2018-06-04 11:05 ` Leizhen (ThunderTown)
[not found] ` <1527752569-18020-1-git-send-email-thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2018-05-31 7:42 ` [PATCH 2/7] iommu/arm-smmu-v3: fix the implementation of flush_iotlb_all hook Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 11:24 ` [PATCH 0/7] add non-strict mode support for arm-smmu-v3 Robin Murphy
2018-05-31 11:24 ` Robin Murphy
2018-05-31 11:24 ` Robin Murphy
2018-05-31 13:49 ` Hanjun Guo
2018-05-31 13:49 ` Hanjun Guo
2018-05-31 14:25 ` Robin Murphy
2018-05-31 14:25 ` Robin Murphy
[not found] ` <92b240f5-596e-87a9-863a-b18475042cce-5wv7dgnIgG8@public.gmane.org>
2018-06-01 6:50 ` Leizhen (ThunderTown)
2018-06-01 6:50 ` Leizhen (ThunderTown)
2018-06-01 6:50 ` Leizhen (ThunderTown)
2018-06-10 11:13 ` Leizhen (ThunderTown)
2018-06-10 11:13 ` Leizhen (ThunderTown)
2018-06-10 11:13 ` Leizhen (ThunderTown)
2018-05-31 7:42 ` [PATCH 3/7] iommu: prepare for the non-strict mode support Zhen Lei
2018-05-31 7:42 ` Zhen Lei
[not found] ` <1527752569-18020-4-git-send-email-thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2018-05-31 13:04 ` Robin Murphy
2018-05-31 13:04 ` Robin Murphy
2018-05-31 13:04 ` Robin Murphy
2018-06-04 11:27 ` Leizhen (ThunderTown)
2018-06-04 11:27 ` Leizhen (ThunderTown)
2018-05-31 7:42 ` [PATCH 4/7] iommu/amd: make sure TLB to be flushed before IOVA freed Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 13:04 ` Robin Murphy
2018-05-31 13:04 ` Robin Murphy
2018-06-04 11:41 ` Leizhen (ThunderTown)
2018-06-04 11:41 ` Leizhen (ThunderTown)
2018-05-31 7:42 ` [PATCH 5/7] iommu/dma: add support for non-strict mode Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 13:04 ` Robin Murphy
2018-05-31 13:04 ` Robin Murphy
[not found] ` <65cfe2f9-eb23-d81c-270e-ae80e96b6009-5wv7dgnIgG8@public.gmane.org>
2018-06-04 12:04 ` Leizhen (ThunderTown)
2018-06-04 12:04 ` Leizhen (ThunderTown)
2018-06-04 12:04 ` Leizhen (ThunderTown)
[not found] ` <1527752569-18020-6-git-send-email-thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2018-06-01 17:51 ` kbuild test robot
2018-06-01 17:51 ` kbuild test robot
2018-06-01 17:51 ` kbuild test robot
[not found] ` <201806020106.eRUp2Ehc%fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2018-06-04 12:19 ` Leizhen (ThunderTown)
2018-06-04 12:19 ` Leizhen (ThunderTown)
2018-06-04 12:19 ` Leizhen (ThunderTown)
2018-05-31 7:42 ` [PATCH 6/7] iommu/io-pgtable-arm: " Zhen Lei
2018-05-31 7:42 ` Zhen Lei
2018-05-31 7:42 ` [PATCH 7/7] iommu/arm-smmu-v3: " Zhen Lei
2018-05-31 7:42 ` Zhen Lei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5B151CFA.10808@huawei.com \
--to=thunder.leizhen-hv44wf8li93qt0dzr+alfa@public.gmane.org \
--cc=guohanjun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org \
--cc=huawei.libin-hv44wF8Li93QT0dZR+AlfA@public.gmane.org \
--cc=huxinwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org \
--cc=iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \
--cc=joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org \
--cc=liguozhu-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=robin.murphy-5wv7dgnIgG8@public.gmane.org \
--cc=will.deacon-5wv7dgnIgG8@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.