On Thu, 15 Jun 2017 15:11:51 +0200 Joerg Roedel <j...@8bytes.org> wrote:
> From: Joerg Roedel <jroe...@suse.de> > > The iommu_group_get_for_dev() function also attaches the > device to its group, so this code doesn't need to be in the > iommu driver. > > Further by using this function the driver can make use of > default domains in the future. > > Signed-off-by: Joerg Roedel <jroe...@suse.de> Seems pretty straightforward, so Reviewed-by: Gerald Schaefer <gerald.schae...@de.ibm.com> However, looking at iommu_group_get_for_dev(), I wonder if the generic_device_group() always returns the right thing, but that would be independent from this patch. With generic_device_group() returning NULL in case the allocation failed, this part of iommu_group_get_for_dev() would then happily dereference the NULL pointer, because IS_ERR(group) would be false: if (ops && ops->device_group) group = ops->device_group(dev); if (IS_ERR(group)) return group; /* * Try to allocate a default domain - needs support from the * IOMMU driver. */ if (!group->default_domain) { The same is true for pci_device_group(), which also returns NULL in case of allocation failure. I guess both functions should just return the group pointer from iommu_group_alloc() directly, which already would contain an appropriate ERR_PTR, but never NULL. What do you think? > --- > drivers/iommu/s390-iommu.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c > index 179e636..8788640 100644 > --- a/drivers/iommu/s390-iommu.c > +++ b/drivers/iommu/s390-iommu.c > @@ -165,20 +165,14 @@ static void s390_iommu_detach_device(struct > iommu_domain *domain, > > static int s390_iommu_add_device(struct device *dev) > { > - struct iommu_group *group; > - int rc; > + struct iommu_group *group = iommu_group_get_for_dev(dev); > > - group = iommu_group_get(dev); > - if (!group) { > - group = iommu_group_alloc(); > - if (IS_ERR(group)) > - return PTR_ERR(group); > - } > + if (IS_ERR(group)) > + return PTR_ERR(group); > > - rc = iommu_group_add_device(group, dev); > iommu_group_put(group); > > - return rc; > + return 0; > } > > static void s390_iommu_remove_device(struct device *dev) > @@ -344,6 +338,7 @@ static struct iommu_ops s390_iommu_ops = { > .iova_to_phys = s390_iommu_iova_to_phys, > .add_device = s390_iommu_add_device, > .remove_device = s390_iommu_remove_device, > + .device_group = generic_device_group, > .pgsize_bitmap = S390_IOMMU_PGSIZES, > }; > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu