Re: [PATCH v5 16/19] iommu/vt-d: Misc macro clean up for SVM

2019-08-15 Thread Jacob Pan
On Fri, 16 Aug 2019 00:17:44 +0300
Andy Shevchenko  wrote:

> On Thu, Aug 15, 2019 at 11:52 PM Jacob Pan
>  wrote:
> >
> > Use combined macros for_each_svm_dev() to simplify SVM device
> > iteration and error checking.
> >
> > Suggested-by: Andy Shevchenko 
> > Signed-off-by: Jacob Pan 
> > Reviewed-by: Eric Auger 
> > ---
> >  drivers/iommu/intel-svm.c | 85
> > +++ 1 file changed, 41
> > insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index 5a688a5..ea6f2e2 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -218,6 +218,10 @@ static const struct mmu_notifier_ops
> > intel_mmuops = { static DEFINE_MUTEX(pasid_mutex);
> >  static LIST_HEAD(global_svm_list);
> >
> > +#define for_each_svm_dev(svm, dev) \
> > +   list_for_each_entry(sdev, >devs, list) \  
> 
> > +   if (dev == sdev->dev)   \  
> 
> This should be
>   if (dev != sdev->dev) {} else
> and no trailing \ is neeeded.
> 
> The rationale of above form to avoid
> for_each_foo() {
> } else {
>   ...WTF?!..
> }
> 
I understand, but until we have the else {} case we don't have anything
to avoid. The current code only has a simple positive logic.

> > +
> >  int intel_svm_bind_mm(struct device *dev, int *pasid, int flags,
> > struct svm_dev_ops *ops) {
> > struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> > @@ -263,15 +267,13 @@ int intel_svm_bind_mm(struct device *dev, int
> > *pasid, int flags, struct svm_dev_ goto out;
> > }
> >
> > -   list_for_each_entry(sdev, >devs, list)
> > {
> > -   if (dev == sdev->dev) {
> > -   if (sdev->ops != ops) {
> > -   ret = -EBUSY;
> > -   goto out;
> > -   }
> > -   sdev->users++;
> > -   goto success;
> > +   for_each_svm_dev(svm, dev) {
> > +   if (sdev->ops != ops) {
> > +   ret = -EBUSY;
> > +   goto out;
> > }
> > +   sdev->users++;
> > +   goto success;
> > }
> >
> > break;
> > @@ -408,48 +410,43 @@ int intel_svm_unbind_mm(struct device *dev,
> > int pasid) goto out;
> >
> > svm = ioasid_find(NULL, pasid, NULL);
> > -   if (IS_ERR(svm)) {
> > +   if (IS_ERR_OR_NULL(svm)) {
> > ret = PTR_ERR(svm);
> > goto out;
> > }
> >
> > -   if (!svm)
> > -   goto out;
> > +   for_each_svm_dev(svm, dev) {
> > +   ret = 0;
> > +   sdev->users--;
> > +   if (!sdev->users) {
> > +   list_del_rcu(>list);
> > +   /* Flush the PASID cache and IOTLB for this
> > device.
> > +* Note that we do depend on the hardware
> > *not* using
> > +* the PASID any more. Just as we depend on
> > other
> > +* devices never using PASIDs that they
> > have no right
> > +* to use. We have a *shared* PASID table,
> > because it's
> > +* large and has to be physically
> > contiguous. So it's
> > +* hard to be as defensive as we might
> > like. */
> > +   intel_pasid_tear_down_entry(iommu, dev,
> > svm->pasid);
> > +   intel_flush_svm_range_dev(svm, sdev, 0, -1,
> > 0, !svm->mm);
> > +   kfree_rcu(sdev, rcu);
> > +
> > +   if (list_empty(>devs)) {
> > +   ioasid_free(svm->pasid);
> > +   if (svm->mm)
> > +
> > mmu_notifier_unregister(>notifier, svm->mm);
> >
> > -   list_for_each_entry(sdev, >devs, list) {
> > -   if (dev == sdev->dev) {
> > -   ret = 0;
> > -   sdev->users--;
> > -   if (!sdev->users) {
> > -   list_del_rcu(>list);
> > -   /* Flush the PASID cache and IOTLB
> > for this device.
> > -* Note that we do depend on the
> > hardware *not* using
> > -* the PASID any more. Just as we
> > depend on other
> > -* devices never using PASIDs that
> > they have no right
> > -* to use. We have a *shared* PASID
> > table, because it's
> > -* large and has to be physically
> > 

Re: [PATCH v5 16/19] iommu/vt-d: Misc macro clean up for SVM

2019-08-15 Thread Andy Shevchenko
On Thu, Aug 15, 2019 at 11:52 PM Jacob Pan
 wrote:
>
> Use combined macros for_each_svm_dev() to simplify SVM device iteration
> and error checking.
>
> Suggested-by: Andy Shevchenko 
> Signed-off-by: Jacob Pan 
> Reviewed-by: Eric Auger 
> ---
>  drivers/iommu/intel-svm.c | 85 
> +++
>  1 file changed, 41 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 5a688a5..ea6f2e2 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -218,6 +218,10 @@ static const struct mmu_notifier_ops intel_mmuops = {
>  static DEFINE_MUTEX(pasid_mutex);
>  static LIST_HEAD(global_svm_list);
>
> +#define for_each_svm_dev(svm, dev) \
> +   list_for_each_entry(sdev, >devs, list) \

> +   if (dev == sdev->dev)   \

This should be
  if (dev != sdev->dev) {} else
and no trailing \ is neeeded.

The rationale of above form to avoid
for_each_foo() {
} else {
  ...WTF?!..
}

> +
>  int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct 
> svm_dev_ops *ops)
>  {
> struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> @@ -263,15 +267,13 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, 
> int flags, struct svm_dev_
> goto out;
> }
>
> -   list_for_each_entry(sdev, >devs, list) {
> -   if (dev == sdev->dev) {
> -   if (sdev->ops != ops) {
> -   ret = -EBUSY;
> -   goto out;
> -   }
> -   sdev->users++;
> -   goto success;
> +   for_each_svm_dev(svm, dev) {
> +   if (sdev->ops != ops) {
> +   ret = -EBUSY;
> +   goto out;
> }
> +   sdev->users++;
> +   goto success;
> }
>
> break;
> @@ -408,48 +410,43 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
> goto out;
>
> svm = ioasid_find(NULL, pasid, NULL);
> -   if (IS_ERR(svm)) {
> +   if (IS_ERR_OR_NULL(svm)) {
> ret = PTR_ERR(svm);
> goto out;
> }
>
> -   if (!svm)
> -   goto out;
> +   for_each_svm_dev(svm, dev) {
> +   ret = 0;
> +   sdev->users--;
> +   if (!sdev->users) {
> +   list_del_rcu(>list);
> +   /* Flush the PASID cache and IOTLB for this device.
> +* Note that we do depend on the hardware *not* using
> +* the PASID any more. Just as we depend on other
> +* devices never using PASIDs that they have no right
> +* to use. We have a *shared* PASID table, because 
> it's
> +* large and has to be physically contiguous. So it's
> +* hard to be as defensive as we might like. */
> +   intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> +   intel_flush_svm_range_dev(svm, sdev, 0, -1, 0, 
> !svm->mm);
> +   kfree_rcu(sdev, rcu);
> +
> +   if (list_empty(>devs)) {
> +   ioasid_free(svm->pasid);
> +   if (svm->mm)
> +   
> mmu_notifier_unregister(>notifier, svm->mm);
>
> -   list_for_each_entry(sdev, >devs, list) {
> -   if (dev == sdev->dev) {
> -   ret = 0;
> -   sdev->users--;
> -   if (!sdev->users) {
> -   list_del_rcu(>list);
> -   /* Flush the PASID cache and IOTLB for this 
> device.
> -* Note that we do depend on the hardware 
> *not* using
> -* the PASID any more. Just as we depend on 
> other
> -* devices never using PASIDs that they have 
> no right
> -* to use. We have a *shared* PASID table, 
> because it's
> -* large and has to be physically contiguous. 
> So it's
> -* hard to be as defensive as we might like. 
> */
> -   intel_pasid_tear_down_entry(iommu, dev, 
> svm->pasid);
> -   intel_flush_svm_range_dev(svm, sdev, 0, -1, 
> 0, !svm->mm);
> -   kfree_rcu(sdev, rcu);
> -
> -