Messages in this thread | | | Date | Fri, 6 Jan 2023 14:07:32 +0800 | Subject | Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops | From | Baolu Lu <> |
| |
On 1/5/23 9:15 PM, Jason Gunthorpe wrote: > On Thu, Jan 05, 2023 at 01:58:42PM +0800, Baolu Lu wrote: >> Hi Jason, >> >> On 2023/1/4 21:17, Jason Gunthorpe wrote: >>> On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote: >>> >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>>> index de91dd88705b..4e35a9f94873 100644 >>>> --- a/drivers/iommu/iommu.c >>>> +++ b/drivers/iommu/iommu.c >>>> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data) >>>> return 0; >>>> } >>>> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data) >>>> +{ >>>> + const struct iommu_ops *ops = dev_iommu_ops(dev); >>>> + >>>> + if (!ops->set_platform_dma_ops) >>>> + return -EINVAL; >>>> + >>>> + ops->set_platform_dma_ops(dev); >>>> + return 0; >>>> +} >>>> + >>>> static int __iommu_group_set_domain(struct iommu_group *group, >>>> struct iommu_domain *new_domain) >>>> { >>>> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group, >>>> * platform specific behavior. >>>> */ >>>> if (!new_domain) { >>>> - if (WARN_ON(!group->domain->ops->detach_dev)) >>>> - return -EINVAL; >>> This should still have the WARN_ON.. >>> >>> if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops) >> This has been implicitly included in the code. >> >> iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver >> doesn't support set_platform_dma_ops (otherwise always return success). >> Then, the domain->ops->detach_dev is required and a WARN_ON was there. >> >> if (!new_domain) { >> ret = __iommu_group_for_each_dev(group, NULL, >> iommu_group_do_set_platform_dma); >> if (ret) { >> if (WARN_ON(!group->domain->ops->detach_dev)) >> return -EINVAL; >> __iommu_group_for_each_dev(group, group->domain, >> iommu_group_do_detach_device); >> } >> group->domain = NULL; >> return 0; >> } >> >> Perhaps I should add a comment to explain this? > But you delete this later when you remove this. > > I think testing the op directly is much clearer, get rid of the whole > ret and EINVAL thinig: > > if (dev_iommu_ops(dev)->set_platform_dma_ops) > __iommu_group_for_each_dev(group, NULL, > iommu_group_do_set_platform_dma); // Can't fail! > else if (group->domain->ops->detach_dev) > __iommu_group_for_each_dev(group, group->domain, > iommu_group_do_detach_device); > else > WARN(true)
Above looks good to me. Thanks! I have updated this part of code like below:
@@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct iommu_group *group, * platform specific behavior. */ if (!new_domain) { - if (WARN_ON(!group->domain->ops->detach_dev)) - return -EINVAL; - __iommu_group_for_each_dev(group, group->domain, - iommu_group_do_detach_device); + struct group_device *grp_dev; + + grp_dev = list_first_entry(&group->devices, + struct group_device, list); + + if (dev_iommu_ops(grp_dev->dev)->set_platform_dma_ops) + __iommu_group_for_each_dev(group, NULL, + iommu_group_do_set_platform_dma); + else if (group->domain->ops->detach_dev) + __iommu_group_for_each_dev(group, group->domain, + iommu_group_do_detach_device); + else + WARN_ON_ONCE(1); + group->domain = NULL; return 0; }
-- Best regards, baolu
| |