* [PATCH] iommu: fix return error code in iommu_probe_device()
@ 2020-11-17 2:52 Yang Yingliang
2020-11-17 9:40 ` Lu Baolu
0 siblings, 1 reply; 8+ messages in thread
From: Yang Yingliang @ 2020-11-17 2:52 UTC (permalink / raw)
To: iommu, linux-kernel; +Cc: yangyingliang
If iommu_group_get() failed, it need return error code
in iommu_probe_device().
Fixes: cf193888bfbd ("iommu: Move new probe_device path...")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/iommu/iommu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b53446bb8c6b..6f4a32df90f6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev)
goto err_out;
group = iommu_group_get(dev);
- if (!group)
+ if (!group) {
+ ret = -ENODEV;
goto err_release;
+ }
/*
* Try to allocate a default domain - needs support from the
--
2.25.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-17 2:52 [PATCH] iommu: fix return error code in iommu_probe_device() Yang Yingliang @ 2020-11-17 9:40 ` Lu Baolu 2020-11-17 11:11 ` Yang Yingliang 0 siblings, 1 reply; 8+ messages in thread From: Lu Baolu @ 2020-11-17 9:40 UTC (permalink / raw) To: Yang Yingliang, iommu, linux-kernel Hi Yingliang, On 2020/11/17 10:52, Yang Yingliang wrote: > If iommu_group_get() failed, it need return error code > in iommu_probe_device(). > > Fixes: cf193888bfbd ("iommu: Move new probe_device path...") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/iommu/iommu.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index b53446bb8c6b..6f4a32df90f6 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) > goto err_out; > > group = iommu_group_get(dev); > - if (!group) > + if (!group) { > + ret = -ENODEV; Can you please explain why you use -ENODEV here? Best regards, baolu > goto err_release; > + } > > /* > * Try to allocate a default domain - needs support from the > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-17 9:40 ` Lu Baolu @ 2020-11-17 11:11 ` Yang Yingliang 2020-11-17 22:41 ` Will Deacon 0 siblings, 1 reply; 8+ messages in thread From: Yang Yingliang @ 2020-11-17 11:11 UTC (permalink / raw) To: Lu Baolu, iommu, linux-kernel On 2020/11/17 17:40, Lu Baolu wrote: > Hi Yingliang, > > On 2020/11/17 10:52, Yang Yingliang wrote: >> If iommu_group_get() failed, it need return error code >> in iommu_probe_device(). >> >> Fixes: cf193888bfbd ("iommu: Move new probe_device path...") >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/iommu/iommu.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index b53446bb8c6b..6f4a32df90f6 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) >> goto err_out; >> group = iommu_group_get(dev); >> - if (!group) >> + if (!group) { >> + ret = -ENODEV; > > Can you please explain why you use -ENODEV here? Before 79659190ee97 ("iommu: Don't take group reference in iommu_alloc_default_domain()"), in iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/iommu.c?h=v5.10-rc4&id=70b8170e55d3ca9503a53211967faee6b5f18b19 > > Best regards, > baolu > >> goto err_release; >> + } >> /* >> * Try to allocate a default domain - needs support from the >> > . _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-17 11:11 ` Yang Yingliang @ 2020-11-17 22:41 ` Will Deacon 2020-11-25 1:54 ` Yang Yingliang 0 siblings, 1 reply; 8+ messages in thread From: Will Deacon @ 2020-11-17 22:41 UTC (permalink / raw) To: Yang Yingliang; +Cc: iommu, linux-kernel On Tue, Nov 17, 2020 at 07:11:28PM +0800, Yang Yingliang wrote: > On 2020/11/17 17:40, Lu Baolu wrote: > > On 2020/11/17 10:52, Yang Yingliang wrote: > > > If iommu_group_get() failed, it need return error code > > > in iommu_probe_device(). > > > > > > Fixes: cf193888bfbd ("iommu: Move new probe_device path...") > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > --- > > > drivers/iommu/iommu.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > index b53446bb8c6b..6f4a32df90f6 100644 > > > --- a/drivers/iommu/iommu.c > > > +++ b/drivers/iommu/iommu.c > > > @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) > > > goto err_out; > > > group = iommu_group_get(dev); > > > - if (!group) > > > + if (!group) { > > > + ret = -ENODEV; > > > > Can you please explain why you use -ENODEV here? > > Before 79659190ee97 ("iommu: Don't take group reference in > iommu_alloc_default_domain()"), in > > iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV. Hmm. While I think the patch is ok, I'm not sure it qualifies as a fix. Has iommu_probe_device() ever propagated this error? The commit you identify in the 'Fixes:' tag doesn't seem to change this afaict. Will _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-17 22:41 ` Will Deacon @ 2020-11-25 1:54 ` Yang Yingliang 2020-11-25 11:35 ` Will Deacon 0 siblings, 1 reply; 8+ messages in thread From: Yang Yingliang @ 2020-11-25 1:54 UTC (permalink / raw) To: Will Deacon; +Cc: iommu, linux-kernel On 2020/11/18 6:41, Will Deacon wrote: > On Tue, Nov 17, 2020 at 07:11:28PM +0800, Yang Yingliang wrote: >> On 2020/11/17 17:40, Lu Baolu wrote: >>> On 2020/11/17 10:52, Yang Yingliang wrote: >>>> If iommu_group_get() failed, it need return error code >>>> in iommu_probe_device(). >>>> >>>> Fixes: cf193888bfbd ("iommu: Move new probe_device path...") >>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>> --- >>>> drivers/iommu/iommu.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>>> index b53446bb8c6b..6f4a32df90f6 100644 >>>> --- a/drivers/iommu/iommu.c >>>> +++ b/drivers/iommu/iommu.c >>>> @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) >>>> goto err_out; >>>> group = iommu_group_get(dev); >>>> - if (!group) >>>> + if (!group) { >>>> + ret = -ENODEV; >>> Can you please explain why you use -ENODEV here? >> Before 79659190ee97 ("iommu: Don't take group reference in >> iommu_alloc_default_domain()"), in >> >> iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV. > Hmm. While I think the patch is ok, I'm not sure it qualifies as a fix. > Has iommu_probe_device() ever propagated this error? The commit you > identify in the 'Fixes:' tag doesn't seem to change this afaict. I think after this commit 439945e74a4b ("iommu: Move default domain allocation to iommu_probe_device()"), iommu_probe_device() won't return error code if group is NULL. I can add this fix tag in v2. > > Will > . _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-25 1:54 ` Yang Yingliang @ 2020-11-25 11:35 ` Will Deacon 2020-11-26 2:29 ` Yang Yingliang 0 siblings, 1 reply; 8+ messages in thread From: Will Deacon @ 2020-11-25 11:35 UTC (permalink / raw) To: Yang Yingliang; +Cc: iommu, linux-kernel On Wed, Nov 25, 2020 at 09:54:34AM +0800, Yang Yingliang wrote: > > On 2020/11/18 6:41, Will Deacon wrote: > > On Tue, Nov 17, 2020 at 07:11:28PM +0800, Yang Yingliang wrote: > > > On 2020/11/17 17:40, Lu Baolu wrote: > > > > On 2020/11/17 10:52, Yang Yingliang wrote: > > > > > If iommu_group_get() failed, it need return error code > > > > > in iommu_probe_device(). > > > > > > > > > > Fixes: cf193888bfbd ("iommu: Move new probe_device path...") > > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > > > --- > > > > > drivers/iommu/iommu.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > > > index b53446bb8c6b..6f4a32df90f6 100644 > > > > > --- a/drivers/iommu/iommu.c > > > > > +++ b/drivers/iommu/iommu.c > > > > > @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) > > > > > goto err_out; > > > > > group = iommu_group_get(dev); > > > > > - if (!group) > > > > > + if (!group) { > > > > > + ret = -ENODEV; > > > > Can you please explain why you use -ENODEV here? > > > Before 79659190ee97 ("iommu: Don't take group reference in > > > iommu_alloc_default_domain()"), in > > > > > > iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV. > > Hmm. While I think the patch is ok, I'm not sure it qualifies as a fix. > > Has iommu_probe_device() ever propagated this error? The commit you > > identify in the 'Fixes:' tag doesn't seem to change this afaict. > > I think after this commit 439945e74a4b ("iommu: Move default domain > allocation to iommu_probe_device()"), That SHA doesn't exist in my tree (maybe you mean 6e1aa2049154?). But even then, I'm not sure 6e1aa2049154 is actually broken if you look at the interaction with group creation in __iommu_probe_device(). In fact, isn't that the case in mainline too? If __iommu_probe_device() returns 0, then we _know_ a group exists and so iommu_group_get() will succeed. I'm still happy with the patch in case this changes in future, but it doesn't appear to be fixing anything. Do you agree? Will _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-25 11:35 ` Will Deacon @ 2020-11-26 2:29 ` Yang Yingliang 2020-11-26 8:59 ` Will Deacon 0 siblings, 1 reply; 8+ messages in thread From: Yang Yingliang @ 2020-11-26 2:29 UTC (permalink / raw) To: Will Deacon; +Cc: iommu, linux-kernel On 2020/11/25 19:35, Will Deacon wrote: > On Wed, Nov 25, 2020 at 09:54:34AM +0800, Yang Yingliang wrote: >> On 2020/11/18 6:41, Will Deacon wrote: >>> On Tue, Nov 17, 2020 at 07:11:28PM +0800, Yang Yingliang wrote: >>>> On 2020/11/17 17:40, Lu Baolu wrote: >>>>> On 2020/11/17 10:52, Yang Yingliang wrote: >>>>>> If iommu_group_get() failed, it need return error code >>>>>> in iommu_probe_device(). >>>>>> >>>>>> Fixes: cf193888bfbd ("iommu: Move new probe_device path...") >>>>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>>>> --- >>>>>> drivers/iommu/iommu.c | 4 +++- >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>>>>> index b53446bb8c6b..6f4a32df90f6 100644 >>>>>> --- a/drivers/iommu/iommu.c >>>>>> +++ b/drivers/iommu/iommu.c >>>>>> @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) >>>>>> goto err_out; >>>>>> group = iommu_group_get(dev); >>>>>> - if (!group) >>>>>> + if (!group) { >>>>>> + ret = -ENODEV; >>>>> Can you please explain why you use -ENODEV here? >>>> Before 79659190ee97 ("iommu: Don't take group reference in >>>> iommu_alloc_default_domain()"), in >>>> >>>> iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV. >>> Hmm. While I think the patch is ok, I'm not sure it qualifies as a fix. >>> Has iommu_probe_device() ever propagated this error? The commit you >>> identify in the 'Fixes:' tag doesn't seem to change this afaict. >> I think after this commit 439945e74a4b ("iommu: Move default domain >> allocation to iommu_probe_device()"), > That SHA doesn't exist in my tree (maybe you mean 6e1aa2049154?). But even > then, I'm not sure 6e1aa2049154 is actually broken if you look at the > interaction with group creation in __iommu_probe_device(). > > In fact, isn't that the case in mainline too? If __iommu_probe_device() > returns 0, then we _know_ a group exists and so iommu_group_get() will > succeed. I'm still happy with the patch in case this changes in future, > but it doesn't appear to be fixing anything. Do you agree? Yes, I look into the __iommu_probe_device(), if it can't get group, it will return error first. Do I need send a v2 without the fix tag ? > > Will > . _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu: fix return error code in iommu_probe_device() 2020-11-26 2:29 ` Yang Yingliang @ 2020-11-26 8:59 ` Will Deacon 0 siblings, 0 replies; 8+ messages in thread From: Will Deacon @ 2020-11-26 8:59 UTC (permalink / raw) To: Yang Yingliang; +Cc: iommu, linux-kernel On Thu, Nov 26, 2020 at 10:29:57AM +0800, Yang Yingliang wrote: > > On 2020/11/25 19:35, Will Deacon wrote: > > On Wed, Nov 25, 2020 at 09:54:34AM +0800, Yang Yingliang wrote: > > > On 2020/11/18 6:41, Will Deacon wrote: > > > > On Tue, Nov 17, 2020 at 07:11:28PM +0800, Yang Yingliang wrote: > > > > > On 2020/11/17 17:40, Lu Baolu wrote: > > > > > > On 2020/11/17 10:52, Yang Yingliang wrote: > > > > > > > If iommu_group_get() failed, it need return error code > > > > > > > in iommu_probe_device(). > > > > > > > > > > > > > > Fixes: cf193888bfbd ("iommu: Move new probe_device path...") > > > > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > > > > > --- > > > > > > > drivers/iommu/iommu.c | 4 +++- > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > > > > > index b53446bb8c6b..6f4a32df90f6 100644 > > > > > > > --- a/drivers/iommu/iommu.c > > > > > > > +++ b/drivers/iommu/iommu.c > > > > > > > @@ -253,8 +253,10 @@ int iommu_probe_device(struct device *dev) > > > > > > > goto err_out; > > > > > > > group = iommu_group_get(dev); > > > > > > > - if (!group) > > > > > > > + if (!group) { > > > > > > > + ret = -ENODEV; > > > > > > Can you please explain why you use -ENODEV here? > > > > > Before 79659190ee97 ("iommu: Don't take group reference in > > > > > iommu_alloc_default_domain()"), in > > > > > > > > > > iommu_alloc_default_domain(), if group is NULL, it will return -ENODEV. > > > > Hmm. While I think the patch is ok, I'm not sure it qualifies as a fix. > > > > Has iommu_probe_device() ever propagated this error? The commit you > > > > identify in the 'Fixes:' tag doesn't seem to change this afaict. > > > I think after this commit 439945e74a4b ("iommu: Move default domain > > > allocation to iommu_probe_device()"), > > That SHA doesn't exist in my tree (maybe you mean 6e1aa2049154?). But even > > then, I'm not sure 6e1aa2049154 is actually broken if you look at the > > interaction with group creation in __iommu_probe_device(). > > > > In fact, isn't that the case in mainline too? If __iommu_probe_device() > > returns 0, then we _know_ a group exists and so iommu_group_get() will > > succeed. I'm still happy with the patch in case this changes in future, > > but it doesn't appear to be fixing anything. Do you agree? > > Yes, I look into the __iommu_probe_device(), if it can't get group, it will > return error > > first. Do I need send a v2 without the fix tag ? Yes, I think that would be fine, and perhaps update the commit message to make it clear that this is purely cosmetic. Will _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-26 8:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-17 2:52 [PATCH] iommu: fix return error code in iommu_probe_device() Yang Yingliang 2020-11-17 9:40 ` Lu Baolu 2020-11-17 11:11 ` Yang Yingliang 2020-11-17 22:41 ` Will Deacon 2020-11-25 1:54 ` Yang Yingliang 2020-11-25 11:35 ` Will Deacon 2020-11-26 2:29 ` Yang Yingliang 2020-11-26 8:59 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).