Hi Robin, Thanks for the patch.
Robin Murphy <[email protected]> writes: > We wouldn't normally expect ops->attach_dev() to fail, but on IOMMUs > with limited hardware resources, or generally misconfigured systems, > it is certainly possible. We report failure correctly from the external > iommu_attach_device() interface, but do not do so in iommu_group_add() > when attaching to the default domain. The result of failure there is > that the device, group and domain all get left in a broken, > part-configured state which leads to weird errors and misbehaviour down > the line when IOMMU API calls sort-of-but-don't-quite work. > > Check the return value of __iommu_attach_device() on the default domain, > and refactor the error handling paths to cope with its failure and clean > up correctly in such cases. > Booting with the patch, the boot time warnings I was seeing go away. I also now see a bunch of messages when adding devices, such as - [ 4.805466] iommu: Failed to add device 0000:06:10.2 to group 10: -28 > Fixes: e39cb8a3aa98 ("iommu: Make sure a device is always attached to a > domain") > Reported-by: Punit Agrawal <[email protected]> > Signed-off-by: Robin Murphy <[email protected]> FWIW, Tested-by: Punit Agrawal <[email protected]> > --- > drivers/iommu/iommu.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index dbe7f653bb7c..aed906a3e3db 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -383,36 +383,30 @@ int iommu_group_add_device(struct iommu_group *group, > struct device *dev) > device->dev = dev; > > ret = sysfs_create_link(&dev->kobj, &group->kobj, "iommu_group"); > - if (ret) { > - kfree(device); > - return ret; > - } > + if (ret) > + goto err_free_device; > > device->name = kasprintf(GFP_KERNEL, "%s", kobject_name(&dev->kobj)); > rename: > if (!device->name) { > - sysfs_remove_link(&dev->kobj, "iommu_group"); > - kfree(device); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err_remove_link; > } > > ret = sysfs_create_link_nowarn(group->devices_kobj, > &dev->kobj, device->name); > if (ret) { > - kfree(device->name); > if (ret == -EEXIST && i >= 0) { > /* > * Account for the slim chance of collision > * and append an instance to the name. > */ > + kfree(device->name); > device->name = kasprintf(GFP_KERNEL, "%s.%d", > kobject_name(&dev->kobj), i++); > goto rename; > } > - > - sysfs_remove_link(&dev->kobj, "iommu_group"); > - kfree(device); > - return ret; > + goto err_free_name; > } > > kobject_get(group->devices_kobj); > @@ -424,8 +418,10 @@ int iommu_group_add_device(struct iommu_group *group, > struct device *dev) > mutex_lock(&group->mutex); > list_add_tail(&device->list, &group->devices); > if (group->domain) > - __iommu_attach_device(group->domain, dev); > + ret = __iommu_attach_device(group->domain, dev); > mutex_unlock(&group->mutex); > + if (ret) > + goto err_put_group; > > /* Notify any listeners about change to group. */ > blocking_notifier_call_chain(&group->notifier, > @@ -436,6 +432,21 @@ int iommu_group_add_device(struct iommu_group *group, > struct device *dev) > pr_info("Adding device %s to group %d\n", dev_name(dev), group->id); > > return 0; > + > +err_put_group: > + mutex_lock(&group->mutex); > + list_del(&device->list); > + mutex_unlock(&group->mutex); > + dev->iommu_group = NULL; > + kobject_put(group->devices_kobj); > +err_free_name: > + kfree(device->name); > +err_remove_link: > + sysfs_remove_link(&dev->kobj, "iommu_group"); > +err_free_device: > + kfree(device); > + pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), > group->id, ret); > + return ret; > } > EXPORT_SYMBOL_GPL(iommu_group_add_device); _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
