Hi Joerg
On 07.04.2020 20:37, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@xxxxxxx>
>
> All drivers are converted to use the probe/release_device()
> call-backs, so the add_device/remove_device() pointers are unused and
> the code using them can be removed.
>
> Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
> ---
> drivers/iommu/iommu.c | 145 ++++++++----------------------------------
> include/linux/iommu.h | 4 --
> 2 files changed, 27 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cf25c1e48830..d9032f9d597c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -220,7 +220,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> return ret;
> }
>
> -static int __iommu_probe_device_helper(struct device *dev)
> +int iommu_probe_device(struct device *dev)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> struct iommu_group *group;
> @@ -264,70 +264,17 @@ static int __iommu_probe_device_helper(struct device *dev)
>
> }
>
> -int iommu_probe_device(struct device *dev)
> +void iommu_release_device(struct device *dev)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> - struct iommu_group *group;
> - int ret;
> -
> - WARN_ON(dev->iommu_group);
> -
> - if (!ops)
> - return -EINVAL;
> -
> - if (!dev_iommu_get(dev))
> - return -ENOMEM;
> -
> - if (!try_module_get(ops->owner)) {
> - ret = -EINVAL;
> - goto err_free_dev_param;
> - }
> -
> - if (ops->probe_device)
> - return __iommu_probe_device_helper(dev);
> -
> - ret = ops->add_device(dev);
> - if (ret)
> - goto err_module_put;
>
> - group = iommu_group_get(dev);
> - iommu_create_device_direct_mappings(group, dev);
> - iommu_group_put(group);
> -
> - if (ops->probe_finalize)
> - ops->probe_finalize(dev);
> -
> - return 0;
> -
> -err_module_put:
> - module_put(ops->owner);
> -err_free_dev_param:
> - dev_iommu_free(dev);
> - return ret;
> -}
> -
> -static void __iommu_release_device(struct device *dev)
> -{
> - const struct iommu_ops *ops = dev->bus->iommu_ops;
> + if (!dev->iommu)
> + return;
>
> iommu_device_unlink(dev->iommu->iommu_dev, dev);
> -
> iommu_group_remove_device(dev);
>
> ops->release_device(dev);
> -}
> -
> -void iommu_release_device(struct device *dev)
> -{
> - const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> - if (!dev->iommu)
> - return;
> -
> - if (ops->release_device)
> - __iommu_release_device(dev);
> - else if (dev->iommu_group)
> - ops->remove_device(dev);
>
> module_put(ops->owner);
> dev_iommu_free(dev);
> @@ -1560,23 +1507,6 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
> if (ret)
> goto out_put_group;
>
> - /*
> - * Try to allocate a default domain - needs support from the
> - * IOMMU driver. There are still some drivers which don't support
> - * default domains, so the return value is not yet checked. Only
> - * allocate the domain here when the driver still has the
> - * add_device/remove_device call-backs implemented.
> - */
> - if (!ops->probe_device) {
> - iommu_alloc_default_domain(dev);
> -
> - if (group->default_domain)
> - ret = __iommu_attach_device(group->default_domain, dev);
> -
> - if (ret)
> - goto out_put_group;
> - }
> -
> return group;
>
> out_put_group:
> @@ -1591,21 +1521,6 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
> return group->default_domain;
> }
>
> -static int add_iommu_group(struct device *dev, void *data)
> -{
> - int ret = iommu_probe_device(dev);
> -
> - /*
> - * We ignore -ENODEV errors for now, as they just mean that the
> - * device is not translated by an IOMMU. We still care about
> - * other errors and fail to initialize when they happen.
> - */
> - if (ret == -ENODEV)
> - ret = 0;
> -
> - return ret;
> -}
> -
> static int probe_iommu_group(struct device *dev, void *data)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> @@ -1789,45 +1704,39 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group)
>
> int bus_iommu_probe(struct bus_type *bus)
> {
> - const struct iommu_ops *ops = bus->iommu_ops;
> + struct iommu_group *group, *next;
> + LIST_HEAD(group_list);
> int ret;
>
> - if (ops->probe_device) {
> - struct iommu_group *group, *next;
> - LIST_HEAD(group_list);
> -
> - /*
> - * This code-path does not allocate the default domain when
> - * creating the iommu group, so do it after the groups are
> - * created.
> - */
> - ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
> - if (ret)
> - return ret;
> + /*
> + * This code-path does not allocate the default domain when
> + * creating the iommu group, so do it after the groups are
> + * created.
> + */
> + ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
> + if (ret)
> + return ret;
>
> - list_for_each_entry_safe(group, next, &group_list, entry) {
> - /* Remove item from the list */
> - list_del_init(&group->entry);
> + list_for_each_entry_safe(group, next, &group_list, entry) {
> + /* Remove item from the list */
> + list_del_init(&group->entry);
>
> - mutex_lock(&group->mutex);
> + mutex_lock(&group->mutex);
>
> - /* Try to allocate default domain */
> - probe_alloc_default_domain(bus, group);
> + /* Try to allocate default domain */
> + probe_alloc_default_domain(bus, group);
>
> - if (!group->default_domain)
> - continue;
> + if (!group->default_domain)
> + continue;
It doesn't look straight from the above diff, but this continue leaks
group->lock taken.
>
> - iommu_group_create_direct_mappings(group);
> + iommu_group_create_direct_mappings(group);
>
> - ret = __iommu_group_dma_attach(group);
> + ret = __iommu_group_dma_attach(group);
>
> - mutex_unlock(&group->mutex);
> + mutex_unlock(&group->mutex);
>
> - if (ret)
> - break;
> - }
> - } else {
> - ret = bus_for_each_dev(bus, NULL, NULL, add_iommu_group);
> + if (ret)
> + break;
> }
>
> return ret;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fea1622408ad..dd076366383f 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -223,8 +223,6 @@ struct iommu_iotlb_gather {
> * @iotlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
> * queue
> * @iova_to_phys: translate iova to physical address
> - * @add_device: add device to iommu grouping
> - * @remove_device: remove device from iommu grouping
> * @probe_device: Add device to iommu driver handling
> * @release_device: Remove device from iommu driver handling
> * @probe_finalize: Do final setup work after the device is added to an IOMMU
> @@ -277,8 +275,6 @@ struct iommu_ops {
> void (*iotlb_sync)(struct iommu_domain *domain,
> struct iommu_iotlb_gather *iotlb_gather);
> phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova);
> - int (*add_device)(struct device *dev);
> - void (*remove_device)(struct device *dev);
> struct iommu_device *(*probe_device)(struct device *dev);
> void (*release_device)(struct device *dev);
> void (*probe_finalize)(struct device *dev);
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
