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.
Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
