> On 2020/2/24 16:12, Lu Baolu wrote:
> > On 2020/2/24 15:57, Prakhya, Sai Praneeth wrote:
> >>> On 2020/2/24 15:03, Prakhya, Sai Praneeth wrote:
> >>>>> On 2020/2/24 11:20, Prakhya, Sai Praneeth wrote:
> >>>>>>>> + list_for_each_entry_safe(grp_dev, temp, &group->devices,
> >>>>>>>> list) {
> >>>>>>>> + struct device *dev;
> >>>>>>>> +
> >>>>>>>> + dev = grp_dev->dev;
> >>>>>>>> + iommu_release_device(dev);
> >>>>>>>> +
> >>>>>>>> + ret = iommu_group_add_device(group, dev);
> >>>>>>>> + if (ret)
> >>>>>>>> + dev_err(dev, "Failed to add to iommu group %d:
> >>>>>>>> +%d\n",
> >>>>>>>> + group->id, ret);
> >>>>>>> Need to handle this error case.
> >>>>>> I wasn't sure on how to handle the error ☹
> >>>>> Just roll back to the state before calling this function and
> >>>>> return an appropriate error value.
> >>>>>
> >>>>> The likely behavior is detaching the new domains from all devices
> >>>>> (if it has already attached), attaching the old domains to all
> >>>>> devices in the group,
> >>>> And while handling this error case, there is a possibility that
> >>>> attaching to old
> >>> domain could fail.. so, we might need to handle that error case as
> >>> well. If we plan to handle error case, since we have removed devices
> >>> from group above, adding them back could fail too.. that would lead
> >>> into handling error case for an error case.
> >>>
> >>> We can assume that the old domain should always be attached back.
> >>> Otherwise, there must be some bugs in the vendor iommu driver.
> >>>
> >>> It must be able to role back, otherwise users have to reboot the
> >>> system in order to use the devices in the group. This is not
> >>> acceptable in the production kernel.
> >> I agree that we should be able to roll back but I am afraid that the
> >> error handling code could become complex than the usual code that
> >> gets to run. For example, assume there are 'n' devices in the group,
> >> 'k' of them are successfully processed (i.e. default domain type has
> >> been
> >> changed) and if k+1 fails while changing default domain type, to roll
> >> back state of k devices, we need to maintain a list of processed
> >> devices so that we now roll back state for devices in this list. The
> >> present code does not have any list because it's processing
> >> sequentially, it takes a device from the group, changes it domain and
> >> moves to other device and hence does not require a list.
> >>
> >> All said, I could give this a try and see how complex the code could
> >> turn out to be. I hope I am wrong (i.e. turns out implementing error
> >> handling is simple).
> >>
> >
> > I think something like below should work.
> >
> > static int iommu_group_do_attach_device(struct device *dev, void
> > *data) {
> > struct iommu_domain *domain = data;
> >
> > return __iommu_attach_device(domain, dev); }
> >
> > static int __iommu_attach_group(struct iommu_domain *domain,
> > struct iommu_group *group) {
> > int ret;
> >
> > if (group->default_domain && group->domain !=
> > group->default_domain)
> > return -EBUSY;
> >
> > ret = __iommu_group_for_each_dev(group, domain,
> >
> > iommu_group_do_attach_device);
> > if (ret == 0)
> > group->domain = domain;
> >
> > return ret;
> > }
> >
> > The vendor iommu driver should always deprecate the old domain if it
> > exists. Add a comment there.
> >
>
> By the way, this is the expected behavior. Please check
> __iommu_detach_group().
Ok.. I will look into it.
Regards,
Sai
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
