On Fri, 8 Jan 2021 15:52:13 +0100
Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx> wrote:
> The IOPF (I/O Page Fault) feature is now enabled independently from the
> SVA feature, because some IOPF implementations are device-specific and
> do not require IOMMU support for PCIe PRI or Arm SMMU stall.
>
> Enable IOPF unconditionally when enabling SVA for now. In the future, if
> a device driver implementing a uacce interface doesn't need IOPF
> support, it will need to tell the uacce module, for example with a new
> flag.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
Hi Jean-Philippe,
A minor suggestion inline but I'm not that bothered so either way
looks good to me.
> ---
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
> Cc: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> ---
> drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index d07af4edfcac..41ef1eb62a14 100644
> --- a/drivers/misc/uacce/uacce.c
> +++ b/drivers/misc/uacce/uacce.c
> @@ -385,6 +385,24 @@ static void uacce_release(struct device *dev)
> kfree(uacce);
> }
>
> +static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
> +{
> + if (!(flags & UACCE_DEV_SVA))
> + return flags;
> +
> + flags &= ~UACCE_DEV_SVA;
> +
> + if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
> + return flags;
> +
> + if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
> + iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
> + return flags;
> + }
> +
> + return flags | UACCE_DEV_SVA;
> +}
I'm a great fan of paired enable / disable functions.
Whilst it would be trivial, maybe it is worth introducing
uacce_disable_sva()?
Also make that do the flags check internally to make it match
up with the enable path.
> +
> /**
> * uacce_alloc() - alloc an accelerator
> * @parent: pointer of uacce parent device
> @@ -404,11 +422,7 @@ struct uacce_device *uacce_alloc(struct device *parent,
> if (!uacce)
> return ERR_PTR(-ENOMEM);
>
> - if (flags & UACCE_DEV_SVA) {
> - ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
> - if (ret)
> - flags &= ~UACCE_DEV_SVA;
> - }
> + flags = uacce_enable_sva(parent, flags);
>
> uacce->parent = parent;
> uacce->flags = flags;
> @@ -432,8 +446,10 @@ struct uacce_device *uacce_alloc(struct device *parent,
> return uacce;
>
> err_with_uacce:
> - if (flags & UACCE_DEV_SVA)
> + if (flags & UACCE_DEV_SVA) {
> iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
> + iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
> + }
> kfree(uacce);
> return ERR_PTR(ret);
> }
> @@ -487,8 +503,10 @@ void uacce_remove(struct uacce_device *uacce)
> mutex_unlock(&uacce->queues_lock);
>
> /* disable sva now since no opened queues */
> - if (uacce->flags & UACCE_DEV_SVA)
> + if (uacce->flags & UACCE_DEV_SVA) {
> iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
> + iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
> + }
>
> if (uacce->cdev)
> cdev_device_del(uacce->cdev, &uacce->dev);
