On Mon, Sep 28, 2020 at 02:38:30PM -0700, Jacob Pan wrote:
> IOASID private data can be cleared by ioasid_attach_data() with a NULL
> data pointer. A common use case is for a caller to free the data
> afterward. ioasid_attach_data() calls synchronize_rcu() before return
> such that free data can be sure without outstanding readers.
> However, since synchronize_rcu() may sleep, ioasid_attach_data() cannot
> be used under spinlocks.
>
> This patch adds ioasid_detach_data() as a separate API where
> synchronize_rcu() is called only in this case. ioasid_attach_data() can
> then be used under spinlocks. In addition, this change makes the API
> symmetrical.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
A typo below, but
Reviewed-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> ---
> drivers/iommu/intel/svm.c | 4 ++--
> drivers/iommu/ioasid.c | 54 ++++++++++++++++++++++++++++++++++++++---------
> include/linux/ioasid.h | 5 ++++-
> 3 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 2c5645f0737a..06a16bee7b65 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -398,7 +398,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> list_add_rcu(&sdev->list, &svm->devs);
> out:
> if (!IS_ERR_OR_NULL(svm) && list_empty(&svm->devs)) {
> - ioasid_attach_data(data->hpasid, NULL);
> + ioasid_detach_data(data->hpasid);
> kfree(svm);
> }
>
> @@ -441,7 +441,7 @@ int intel_svm_unbind_gpasid(struct device *dev, int pasid)
> * the unbind, IOMMU driver will get notified
> * and perform cleanup.
> */
> - ioasid_attach_data(pasid, NULL);
> + ioasid_detach_data(pasid);
> kfree(svm);
> }
> }
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index 5f63af07acd5..6cfbdfb492e0 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -272,24 +272,58 @@ int ioasid_attach_data(ioasid_t ioasid, void *data)
>
> spin_lock(&ioasid_allocator_lock);
> ioasid_data = xa_load(&active_allocator->xa, ioasid);
> - if (ioasid_data)
> - rcu_assign_pointer(ioasid_data->private, data);
> - else
> +
> + if (!ioasid_data) {
> ret = -ENOENT;
> - spin_unlock(&ioasid_allocator_lock);
> + goto done_unlock;
> + }
>
> - /*
> - * Wait for readers to stop accessing the old private data, so the
> - * caller can free it.
> - */
> - if (!ret)
> - synchronize_rcu();
> + if (ioasid_data->private) {
> + ret = -EBUSY;
> + goto done_unlock;
> + }
> + rcu_assign_pointer(ioasid_data->private, data);
> +
> +done_unlock:
> + spin_unlock(&ioasid_allocator_lock);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(ioasid_attach_data);
>
> /**
> + * ioasid_detach_data - Clear the private data of an ioasid
> + *
> + * @ioasid: the IOASIDD to clear private data
IOASID
> + */
> +void ioasid_detach_data(ioasid_t ioasid)
> +{
> + struct ioasid_data *ioasid_data;
> +
> + spin_lock(&ioasid_allocator_lock);
> + ioasid_data = xa_load(&active_allocator->xa, ioasid);
> +
> + if (!ioasid_data) {
> + pr_warn("IOASID %u not found to detach data from\n", ioasid);
> + goto done_unlock;
> + }
> +
> + if (ioasid_data->private) {
> + rcu_assign_pointer(ioasid_data->private, NULL);
> + goto done_unlock;
> + }
> +
> +done_unlock:
> + spin_unlock(&ioasid_allocator_lock);
> + /*
> + * Wait for readers to stop accessing the old private data,
> + * so the caller can free it.
> + */
> + synchronize_rcu();
> +}
> +EXPORT_SYMBOL_GPL(ioasid_detach_data);
> +
> +/**
> * ioasid_alloc - Allocate an IOASID
> * @set: the IOASID set
> * @min: the minimum ID (inclusive)
> diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
> index 9c44947a68c8..c7f649fa970a 100644
> --- a/include/linux/ioasid.h
> +++ b/include/linux/ioasid.h
> @@ -40,7 +40,7 @@ void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
> int ioasid_register_allocator(struct ioasid_allocator_ops *allocator);
> void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator);
> int ioasid_attach_data(ioasid_t ioasid, void *data);
> -
> +void ioasid_detach_data(ioasid_t ioasid);
> #else /* !CONFIG_IOASID */
> static inline ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
> ioasid_t max, void *private)
> @@ -72,5 +72,8 @@ static inline int ioasid_attach_data(ioasid_t ioasid, void *data)
> return -ENOTSUPP;
> }
>
> +static inline void ioasid_detach_data(ioasid_t ioasid)
> +{
> +}
> #endif /* CONFIG_IOASID */
> #endif /* __LINUX_IOASID_H */
> --
> 2.7.4
>
