RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameerali Kolothum Thodi]

Hi Robin/Joerg,

> -----Original Message-----
> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com]
> Sent: 01 February 2021 12:41
> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
> linuxarm@openeuler.org
> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
> functions
> 
> The device iommu probe/attach might have failed leaving dev->iommu
> to NULL and device drivers may still invoke these functions resulting
> in a crash in iommu vendor driver code. Hence make sure we check that.
> 
> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
> is successfully associated with an iommu.
> 
> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> v1 --> v2:
>  -Added iommu_ops to struct dev_iommu based on the discussion with Robin.
>  -Rebased against iommu-tree core branch.

A gentle ping on this...

Thanks,
Shameer

> ---
>  drivers/iommu/iommu.c | 19 +++++++------------
>  include/linux/iommu.h |  2 ++
>  2 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index fd76e2f579fe..6023d0b7c542 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev,
> struct list_head *group_list
>  	}
> 
>  	dev->iommu->iommu_dev = iommu_dev;
> +	dev->iommu->ops = iommu_dev->ops;
> 
>  	group = iommu_group_get_for_dev(dev);
>  	if (IS_ERR(group)) {
> @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>   */
>  int iommu_dev_enable_feature(struct device *dev, enum
> iommu_dev_features feat)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> -	if (ops && ops->dev_enable_feat)
> -		return ops->dev_enable_feat(dev, feat);
> +	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
> +		return dev->iommu->ops->dev_enable_feat(dev, feat);
> 
>  	return -ENODEV;
>  }
> @@ -2881,10 +2880,8 @@
> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
>   */
>  int iommu_dev_disable_feature(struct device *dev, enum
> iommu_dev_features feat)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> -	if (ops && ops->dev_disable_feat)
> -		return ops->dev_disable_feat(dev, feat);
> +	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
> +		return dev->iommu->ops->dev_disable_feat(dev, feat);
> 
>  	return -EBUSY;
>  }
> @@ -2892,10 +2889,8 @@
> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
> 
>  bool iommu_dev_feature_enabled(struct device *dev, enum
> iommu_dev_features feat)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> -	if (ops && ops->dev_feat_enabled)
> -		return ops->dev_feat_enabled(dev, feat);
> +	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
> +		return dev->iommu->ops->dev_feat_enabled(dev, feat);
> 
>  	return false;
>  }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 524ffc2ff64f..ff0c76bdfb67 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -354,6 +354,7 @@ struct iommu_fault_param {
>   * @fault_param: IOMMU detected device fault reporting data
>   * @fwspec:	 IOMMU fwspec data
>   * @iommu_dev:	 IOMMU device this device is linked to
> + * @ops:	 iommu-ops for talking to the iommu_dev
>   * @priv:	 IOMMU Driver private data
>   *
>   * TODO: migrate other per device data pointers under iommu_dev_data,
> e.g.
> @@ -364,6 +365,7 @@ struct dev_iommu {
>  	struct iommu_fault_param	*fault_param;
>  	struct iommu_fwspec		*fwspec;
>  	struct iommu_device		*iommu_dev;
> +	const struct iommu_ops		*ops;
>  	void				*priv;
>  };
> 
> --
> 2.17.1
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameerali Kolothum Thodi]

Hi Robin/Joerg,

> -----Original Message-----
> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com]
> Sent: 01 February 2021 12:41
> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
> linuxarm@openeuler.org
> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
> functions
> 
> The device iommu probe/attach might have failed leaving dev->iommu
> to NULL and device drivers may still invoke these functions resulting
> in a crash in iommu vendor driver code. Hence make sure we check that.
> 
> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
> is successfully associated with an iommu.
> 
> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> v1 --> v2:
>  -Added iommu_ops to struct dev_iommu based on the discussion with Robin.
>  -Rebased against iommu-tree core branch.

A gentle ping on this...

Thanks,
Shameer

> ---
>  drivers/iommu/iommu.c | 19 +++++++------------
>  include/linux/iommu.h |  2 ++
>  2 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index fd76e2f579fe..6023d0b7c542 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev,
> struct list_head *group_list
>  	}
> 
>  	dev->iommu->iommu_dev = iommu_dev;
> +	dev->iommu->ops = iommu_dev->ops;
> 
>  	group = iommu_group_get_for_dev(dev);
>  	if (IS_ERR(group)) {
> @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>   */
>  int iommu_dev_enable_feature(struct device *dev, enum
> iommu_dev_features feat)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> -	if (ops && ops->dev_enable_feat)
> -		return ops->dev_enable_feat(dev, feat);
> +	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
> +		return dev->iommu->ops->dev_enable_feat(dev, feat);
> 
>  	return -ENODEV;
>  }
> @@ -2881,10 +2880,8 @@
> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
>   */
>  int iommu_dev_disable_feature(struct device *dev, enum
> iommu_dev_features feat)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> -	if (ops && ops->dev_disable_feat)
> -		return ops->dev_disable_feat(dev, feat);
> +	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
> +		return dev->iommu->ops->dev_disable_feat(dev, feat);
> 
>  	return -EBUSY;
>  }
> @@ -2892,10 +2889,8 @@
> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
> 
>  bool iommu_dev_feature_enabled(struct device *dev, enum
> iommu_dev_features feat)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> -
> -	if (ops && ops->dev_feat_enabled)
> -		return ops->dev_feat_enabled(dev, feat);
> +	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
> +		return dev->iommu->ops->dev_feat_enabled(dev, feat);
> 
>  	return false;
>  }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 524ffc2ff64f..ff0c76bdfb67 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -354,6 +354,7 @@ struct iommu_fault_param {
>   * @fault_param: IOMMU detected device fault reporting data
>   * @fwspec:	 IOMMU fwspec data
>   * @iommu_dev:	 IOMMU device this device is linked to
> + * @ops:	 iommu-ops for talking to the iommu_dev
>   * @priv:	 IOMMU Driver private data
>   *
>   * TODO: migrate other per device data pointers under iommu_dev_data,
> e.g.
> @@ -364,6 +365,7 @@ struct dev_iommu {
>  	struct iommu_fault_param	*fault_param;
>  	struct iommu_fwspec		*fwspec;
>  	struct iommu_device		*iommu_dev;
> +	const struct iommu_ops		*ops;
>  	void				*priv;
>  };
> 
> --
> 2.17.1
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Robin Murphy]

On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
> Hi Robin/Joerg,
> 
>> -----Original Message-----
>> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com]
>> Sent: 01 February 2021 12:41
>> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
>> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
>> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
>> linuxarm@openeuler.org
>> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
>> functions
>>
>> The device iommu probe/attach might have failed leaving dev->iommu
>> to NULL and device drivers may still invoke these functions resulting
>> in a crash in iommu vendor driver code. Hence make sure we check that.
>>
>> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
>> is successfully associated with an iommu.
>>
>> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>> v1 --> v2:
>>   -Added iommu_ops to struct dev_iommu based on the discussion with Robin.
>>   -Rebased against iommu-tree core branch.
> 
> A gentle ping on this...

Is there a convincing justification for maintaining yet another copy of 
the ops pointer rather than simply dereferencing iommu_dev->ops at point 
of use?

Robin.

> Thanks,
> Shameer
> 
>> ---
>>   drivers/iommu/iommu.c | 19 +++++++------------
>>   include/linux/iommu.h |  2 ++
>>   2 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index fd76e2f579fe..6023d0b7c542 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev,
>> struct list_head *group_list
>>   	}
>>
>>   	dev->iommu->iommu_dev = iommu_dev;
>> +	dev->iommu->ops = iommu_dev->ops;
>>
>>   	group = iommu_group_get_for_dev(dev);
>>   	if (IS_ERR(group)) {
>> @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>>    */
>>   int iommu_dev_enable_feature(struct device *dev, enum
>> iommu_dev_features feat)
>>   {
>> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> -
>> -	if (ops && ops->dev_enable_feat)
>> -		return ops->dev_enable_feat(dev, feat);
>> +	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
>> +		return dev->iommu->ops->dev_enable_feat(dev, feat);
>>
>>   	return -ENODEV;
>>   }
>> @@ -2881,10 +2880,8 @@
>> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
>>    */
>>   int iommu_dev_disable_feature(struct device *dev, enum
>> iommu_dev_features feat)
>>   {
>> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> -
>> -	if (ops && ops->dev_disable_feat)
>> -		return ops->dev_disable_feat(dev, feat);
>> +	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
>> +		return dev->iommu->ops->dev_disable_feat(dev, feat);
>>
>>   	return -EBUSY;
>>   }
>> @@ -2892,10 +2889,8 @@
>> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
>>
>>   bool iommu_dev_feature_enabled(struct device *dev, enum
>> iommu_dev_features feat)
>>   {
>> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> -
>> -	if (ops && ops->dev_feat_enabled)
>> -		return ops->dev_feat_enabled(dev, feat);
>> +	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
>> +		return dev->iommu->ops->dev_feat_enabled(dev, feat);
>>
>>   	return false;
>>   }
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 524ffc2ff64f..ff0c76bdfb67 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -354,6 +354,7 @@ struct iommu_fault_param {
>>    * @fault_param: IOMMU detected device fault reporting data
>>    * @fwspec:	 IOMMU fwspec data
>>    * @iommu_dev:	 IOMMU device this device is linked to
>> + * @ops:	 iommu-ops for talking to the iommu_dev
>>    * @priv:	 IOMMU Driver private data
>>    *
>>    * TODO: migrate other per device data pointers under iommu_dev_data,
>> e.g.
>> @@ -364,6 +365,7 @@ struct dev_iommu {
>>   	struct iommu_fault_param	*fault_param;
>>   	struct iommu_fwspec		*fwspec;
>>   	struct iommu_device		*iommu_dev;
>> +	const struct iommu_ops		*ops;
>>   	void				*priv;
>>   };
>>
>> --
>> 2.17.1
>> _______________________________________________
>> Linuxarm mailing list -- linuxarm@openeuler.org
>> To unsubscribe send an email to linuxarm-leave@openeuler.org

Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Robin Murphy]

On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
> Hi Robin/Joerg,
> 
>> -----Original Message-----
>> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com]
>> Sent: 01 February 2021 12:41
>> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
>> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
>> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
>> linuxarm@openeuler.org
>> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
>> functions
>>
>> The device iommu probe/attach might have failed leaving dev->iommu
>> to NULL and device drivers may still invoke these functions resulting
>> in a crash in iommu vendor driver code. Hence make sure we check that.
>>
>> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
>> is successfully associated with an iommu.
>>
>> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>> v1 --> v2:
>>   -Added iommu_ops to struct dev_iommu based on the discussion with Robin.
>>   -Rebased against iommu-tree core branch.
> 
> A gentle ping on this...

Is there a convincing justification for maintaining yet another copy of 
the ops pointer rather than simply dereferencing iommu_dev->ops at point 
of use?

Robin.

> Thanks,
> Shameer
> 
>> ---
>>   drivers/iommu/iommu.c | 19 +++++++------------
>>   include/linux/iommu.h |  2 ++
>>   2 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index fd76e2f579fe..6023d0b7c542 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev,
>> struct list_head *group_list
>>   	}
>>
>>   	dev->iommu->iommu_dev = iommu_dev;
>> +	dev->iommu->ops = iommu_dev->ops;
>>
>>   	group = iommu_group_get_for_dev(dev);
>>   	if (IS_ERR(group)) {
>> @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>>    */
>>   int iommu_dev_enable_feature(struct device *dev, enum
>> iommu_dev_features feat)
>>   {
>> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> -
>> -	if (ops && ops->dev_enable_feat)
>> -		return ops->dev_enable_feat(dev, feat);
>> +	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
>> +		return dev->iommu->ops->dev_enable_feat(dev, feat);
>>
>>   	return -ENODEV;
>>   }
>> @@ -2881,10 +2880,8 @@
>> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
>>    */
>>   int iommu_dev_disable_feature(struct device *dev, enum
>> iommu_dev_features feat)
>>   {
>> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> -
>> -	if (ops && ops->dev_disable_feat)
>> -		return ops->dev_disable_feat(dev, feat);
>> +	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
>> +		return dev->iommu->ops->dev_disable_feat(dev, feat);
>>
>>   	return -EBUSY;
>>   }
>> @@ -2892,10 +2889,8 @@
>> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
>>
>>   bool iommu_dev_feature_enabled(struct device *dev, enum
>> iommu_dev_features feat)
>>   {
>> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> -
>> -	if (ops && ops->dev_feat_enabled)
>> -		return ops->dev_feat_enabled(dev, feat);
>> +	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
>> +		return dev->iommu->ops->dev_feat_enabled(dev, feat);
>>
>>   	return false;
>>   }
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 524ffc2ff64f..ff0c76bdfb67 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -354,6 +354,7 @@ struct iommu_fault_param {
>>    * @fault_param: IOMMU detected device fault reporting data
>>    * @fwspec:	 IOMMU fwspec data
>>    * @iommu_dev:	 IOMMU device this device is linked to
>> + * @ops:	 iommu-ops for talking to the iommu_dev
>>    * @priv:	 IOMMU Driver private data
>>    *
>>    * TODO: migrate other per device data pointers under iommu_dev_data,
>> e.g.
>> @@ -364,6 +365,7 @@ struct dev_iommu {
>>   	struct iommu_fault_param	*fault_param;
>>   	struct iommu_fwspec		*fwspec;
>>   	struct iommu_device		*iommu_dev;
>> +	const struct iommu_ops		*ops;
>>   	void				*priv;
>>   };
>>
>> --
>> 2.17.1
>> _______________________________________________
>> Linuxarm mailing list -- linuxarm@openeuler.org
>> To unsubscribe send an email to linuxarm-leave@openeuler.org
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 12 February 2021 16:39
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
> Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
> 
> On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
> > Hi Robin/Joerg,
> >
> >> -----Original Message-----
> >> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com]
> >> Sent: 01 February 2021 12:41
> >> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> >> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
> >> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
> >> linuxarm@openeuler.org
> >> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in
> iommu_dev_xxx
> >> functions
> >>
> >> The device iommu probe/attach might have failed leaving dev->iommu
> >> to NULL and device drivers may still invoke these functions resulting
> >> in a crash in iommu vendor driver code. Hence make sure we check that.
> >>
> >> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
> >> is successfully associated with an iommu.
> >>
> >> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
> >> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >> v1 --> v2:
> >>   -Added iommu_ops to struct dev_iommu based on the discussion with
> Robin.
> >>   -Rebased against iommu-tree core branch.
> >
> > A gentle ping on this...
> 
> Is there a convincing justification for maintaining yet another copy of
> the ops pointer rather than simply dereferencing iommu_dev->ops at point
> of use?
> 

TBH, nothing I can think of now. That was mainly the way I interpreted your suggestion
from the v1.  Now it looks like you didn’t mean it :). I am Ok to rework it to dereference
it from iommu_dev. Please let me know.

Thanks,
Shameer

> Robin.
> 
> > Thanks,
> > Shameer
> >
> >> ---
> >>   drivers/iommu/iommu.c | 19 +++++++------------
> >>   include/linux/iommu.h |  2 ++
> >>   2 files changed, 9 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index fd76e2f579fe..6023d0b7c542 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device
> *dev,
> >> struct list_head *group_list
> >>   	}
> >>
> >>   	dev->iommu->iommu_dev = iommu_dev;
> >> +	dev->iommu->ops = iommu_dev->ops;
> >>
> >>   	group = iommu_group_get_for_dev(dev);
> >>   	if (IS_ERR(group)) {
> >> @@ -2865,10 +2866,8 @@
> EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
> >>    */
> >>   int iommu_dev_enable_feature(struct device *dev, enum
> >> iommu_dev_features feat)
> >>   {
> >> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> >> -
> >> -	if (ops && ops->dev_enable_feat)
> >> -		return ops->dev_enable_feat(dev, feat);
> >> +	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
> >> +		return dev->iommu->ops->dev_enable_feat(dev, feat);
> >>
> >>   	return -ENODEV;
> >>   }
> >> @@ -2881,10 +2880,8 @@
> >> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
> >>    */
> >>   int iommu_dev_disable_feature(struct device *dev, enum
> >> iommu_dev_features feat)
> >>   {
> >> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> >> -
> >> -	if (ops && ops->dev_disable_feat)
> >> -		return ops->dev_disable_feat(dev, feat);
> >> +	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
> >> +		return dev->iommu->ops->dev_disable_feat(dev, feat);
> >>
> >>   	return -EBUSY;
> >>   }
> >> @@ -2892,10 +2889,8 @@
> >> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
> >>
> >>   bool iommu_dev_feature_enabled(struct device *dev, enum
> >> iommu_dev_features feat)
> >>   {
> >> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> >> -
> >> -	if (ops && ops->dev_feat_enabled)
> >> -		return ops->dev_feat_enabled(dev, feat);
> >> +	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
> >> +		return dev->iommu->ops->dev_feat_enabled(dev, feat);
> >>
> >>   	return false;
> >>   }
> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> >> index 524ffc2ff64f..ff0c76bdfb67 100644
> >> --- a/include/linux/iommu.h
> >> +++ b/include/linux/iommu.h
> >> @@ -354,6 +354,7 @@ struct iommu_fault_param {
> >>    * @fault_param: IOMMU detected device fault reporting data
> >>    * @fwspec:	 IOMMU fwspec data
> >>    * @iommu_dev:	 IOMMU device this device is linked to
> >> + * @ops:	 iommu-ops for talking to the iommu_dev
> >>    * @priv:	 IOMMU Driver private data
> >>    *
> >>    * TODO: migrate other per device data pointers under
> iommu_dev_data,
> >> e.g.
> >> @@ -364,6 +365,7 @@ struct dev_iommu {
> >>   	struct iommu_fault_param	*fault_param;
> >>   	struct iommu_fwspec		*fwspec;
> >>   	struct iommu_device		*iommu_dev;
> >> +	const struct iommu_ops		*ops;
> >>   	void				*priv;
> >>   };
> >>
> >> --
> >> 2.17.1
> >> _______________________________________________
> >> Linuxarm mailing list -- linuxarm@openeuler.org
> >> To unsubscribe send an email to linuxarm-leave@openeuler.org

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 12 February 2021 16:39
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
> Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
> 
> On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
> > Hi Robin/Joerg,
> >
> >> -----Original Message-----
> >> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com]
> >> Sent: 01 February 2021 12:41
> >> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> >> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
> >> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
> >> linuxarm@openeuler.org
> >> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in
> iommu_dev_xxx
> >> functions
> >>
> >> The device iommu probe/attach might have failed leaving dev->iommu
> >> to NULL and device drivers may still invoke these functions resulting
> >> in a crash in iommu vendor driver code. Hence make sure we check that.
> >>
> >> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
> >> is successfully associated with an iommu.
> >>
> >> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
> >> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >> v1 --> v2:
> >>   -Added iommu_ops to struct dev_iommu based on the discussion with
> Robin.
> >>   -Rebased against iommu-tree core branch.
> >
> > A gentle ping on this...
> 
> Is there a convincing justification for maintaining yet another copy of
> the ops pointer rather than simply dereferencing iommu_dev->ops at point
> of use?
> 

TBH, nothing I can think of now. That was mainly the way I interpreted your suggestion
from the v1.  Now it looks like you didn’t mean it :). I am Ok to rework it to dereference
it from iommu_dev. Please let me know.

Thanks,
Shameer

> Robin.
> 
> > Thanks,
> > Shameer
> >
> >> ---
> >>   drivers/iommu/iommu.c | 19 +++++++------------
> >>   include/linux/iommu.h |  2 ++
> >>   2 files changed, 9 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index fd76e2f579fe..6023d0b7c542 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device
> *dev,
> >> struct list_head *group_list
> >>   	}
> >>
> >>   	dev->iommu->iommu_dev = iommu_dev;
> >> +	dev->iommu->ops = iommu_dev->ops;
> >>
> >>   	group = iommu_group_get_for_dev(dev);
> >>   	if (IS_ERR(group)) {
> >> @@ -2865,10 +2866,8 @@
> EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
> >>    */
> >>   int iommu_dev_enable_feature(struct device *dev, enum
> >> iommu_dev_features feat)
> >>   {
> >> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> >> -
> >> -	if (ops && ops->dev_enable_feat)
> >> -		return ops->dev_enable_feat(dev, feat);
> >> +	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
> >> +		return dev->iommu->ops->dev_enable_feat(dev, feat);
> >>
> >>   	return -ENODEV;
> >>   }
> >> @@ -2881,10 +2880,8 @@
> >> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
> >>    */
> >>   int iommu_dev_disable_feature(struct device *dev, enum
> >> iommu_dev_features feat)
> >>   {
> >> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> >> -
> >> -	if (ops && ops->dev_disable_feat)
> >> -		return ops->dev_disable_feat(dev, feat);
> >> +	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
> >> +		return dev->iommu->ops->dev_disable_feat(dev, feat);
> >>
> >>   	return -EBUSY;
> >>   }
> >> @@ -2892,10 +2889,8 @@
> >> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
> >>
> >>   bool iommu_dev_feature_enabled(struct device *dev, enum
> >> iommu_dev_features feat)
> >>   {
> >> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> >> -
> >> -	if (ops && ops->dev_feat_enabled)
> >> -		return ops->dev_feat_enabled(dev, feat);
> >> +	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
> >> +		return dev->iommu->ops->dev_feat_enabled(dev, feat);
> >>
> >>   	return false;
> >>   }
> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> >> index 524ffc2ff64f..ff0c76bdfb67 100644
> >> --- a/include/linux/iommu.h
> >> +++ b/include/linux/iommu.h
> >> @@ -354,6 +354,7 @@ struct iommu_fault_param {
> >>    * @fault_param: IOMMU detected device fault reporting data
> >>    * @fwspec:	 IOMMU fwspec data
> >>    * @iommu_dev:	 IOMMU device this device is linked to
> >> + * @ops:	 iommu-ops for talking to the iommu_dev
> >>    * @priv:	 IOMMU Driver private data
> >>    *
> >>    * TODO: migrate other per device data pointers under
> iommu_dev_data,
> >> e.g.
> >> @@ -364,6 +365,7 @@ struct dev_iommu {
> >>   	struct iommu_fault_param	*fault_param;
> >>   	struct iommu_fwspec		*fwspec;
> >>   	struct iommu_device		*iommu_dev;
> >> +	const struct iommu_ops		*ops;
> >>   	void				*priv;
> >>   };
> >>
> >> --
> >> 2.17.1
> >> _______________________________________________
> >> Linuxarm mailing list -- linuxarm@openeuler.org
> >> To unsubscribe send an email to linuxarm-leave@openeuler.org
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 12 February 2021 16:45
> To: 'Robin Murphy' <robin.murphy@arm.com>; linux-kernel@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
> Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
> 
> 
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: 12 February 2021 16:39
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> > Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
> > <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
> > Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
> functions
> >
> > On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
> > > Hi Robin/Joerg,
> > >
> > >> -----Original Message-----
> > >> From: Shameer Kolothum
> [mailto:shameerali.kolothum.thodi@huawei.com]
> > >> Sent: 01 February 2021 12:41
> > >> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> > >> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
> > >> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
> > >> linuxarm@openeuler.org
> > >> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in
> > iommu_dev_xxx
> > >> functions
> > >>
> > >> The device iommu probe/attach might have failed leaving dev->iommu
> > >> to NULL and device drivers may still invoke these functions resulting
> > >> in a crash in iommu vendor driver code. Hence make sure we check that.
> > >>
> > >> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
> > >> is successfully associated with an iommu.
> > >>
> > >> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per
> device")
> > >> Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > >> ---
> > >> v1 --> v2:
> > >>   -Added iommu_ops to struct dev_iommu based on the discussion with
> > Robin.
> > >>   -Rebased against iommu-tree core branch.
> > >
> > > A gentle ping on this...
> >
> > Is there a convincing justification for maintaining yet another copy of
> > the ops pointer rather than simply dereferencing iommu_dev->ops at point
> > of use?
> >
> 
> TBH, nothing I can think of now. That was mainly the way I interpreted your
> suggestion
> from the v1.  Now it looks like you didn’t mean it :). I am Ok to rework it to
> dereference
> it from iommu_dev. Please let me know.

So we can do something like this,

index fd76e2f579fe..5fd31a3cec18 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2865,10 +2865,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
  */
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-       const struct iommu_ops *ops = dev->bus->iommu_ops;
+       if (dev->iommu && dev->iommu->iommu_dev && dev->iommu->iommu_dev->ops)
+               struct iommu_ops  *ops = dev->iommu->iommu_dev->ops;
 
-       if (ops && ops->dev_enable_feat)
-               return ops->dev_enable_feat(dev, feat);
+               if (ops->dev_enable_feat)
+                       return ops->dev_enable_feat(dev, feat);
+       }
 
        return -ENODEV;
 }

Again, not sure we need to do the checking for iommu->dev and ops here. If the
dev->iommu is set, is it safe to assume that we have a valid iommu->iommu_dev
and ops always? (May be it is safer to do the checking in case something
else breaks this assumption in future). Please let me know your thoughts.

Thanks,
Shameer

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 12 February 2021 16:45
> To: 'Robin Murphy' <robin.murphy@arm.com>; linux-kernel@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
> Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
> 
> 
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: 12 February 2021 16:39
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> > Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
> > <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
> > Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
> functions
> >
> > On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
> > > Hi Robin/Joerg,
> > >
> > >> -----Original Message-----
> > >> From: Shameer Kolothum
> [mailto:shameerali.kolothum.thodi@huawei.com]
> > >> Sent: 01 February 2021 12:41
> > >> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
> > >> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
> > >> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
> > >> linuxarm@openeuler.org
> > >> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in
> > iommu_dev_xxx
> > >> functions
> > >>
> > >> The device iommu probe/attach might have failed leaving dev->iommu
> > >> to NULL and device drivers may still invoke these functions resulting
> > >> in a crash in iommu vendor driver code. Hence make sure we check that.
> > >>
> > >> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
> > >> is successfully associated with an iommu.
> > >>
> > >> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per
> device")
> > >> Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > >> ---
> > >> v1 --> v2:
> > >>   -Added iommu_ops to struct dev_iommu based on the discussion with
> > Robin.
> > >>   -Rebased against iommu-tree core branch.
> > >
> > > A gentle ping on this...
> >
> > Is there a convincing justification for maintaining yet another copy of
> > the ops pointer rather than simply dereferencing iommu_dev->ops at point
> > of use?
> >
> 
> TBH, nothing I can think of now. That was mainly the way I interpreted your
> suggestion
> from the v1.  Now it looks like you didn’t mean it :). I am Ok to rework it to
> dereference
> it from iommu_dev. Please let me know.

So we can do something like this,

index fd76e2f579fe..5fd31a3cec18 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2865,10 +2865,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
  */
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-       const struct iommu_ops *ops = dev->bus->iommu_ops;
+       if (dev->iommu && dev->iommu->iommu_dev && dev->iommu->iommu_dev->ops)
+               struct iommu_ops  *ops = dev->iommu->iommu_dev->ops;
 
-       if (ops && ops->dev_enable_feat)
-               return ops->dev_enable_feat(dev, feat);
+               if (ops->dev_enable_feat)
+                       return ops->dev_enable_feat(dev, feat);
+       }
 
        return -ENODEV;
 }

Again, not sure we need to do the checking for iommu->dev and ops here. If the
dev->iommu is set, is it safe to assume that we have a valid iommu->iommu_dev
and ops always? (May be it is safer to do the checking in case something
else breaks this assumption in future). Please let me know your thoughts.

Thanks,
Shameer


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Robin Murphy]

On 2021-02-12 17:28, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Shameerali Kolothum Thodi
>> Sent: 12 February 2021 16:45
>> To: 'Robin Murphy' <robin.murphy@arm.com>; linux-kernel@vger.kernel.org;
>> iommu@lists.linux-foundation.org
>> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
>> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
>> Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
>>
>>
>>
>>> -----Original Message-----
>>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>>> Sent: 12 February 2021 16:39
>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>>> linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
>>> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
>>> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
>>> Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
>> functions
>>>
>>> On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
>>>> Hi Robin/Joerg,
>>>>
>>>>> -----Original Message-----
>>>>> From: Shameer Kolothum
>> [mailto:shameerali.kolothum.thodi@huawei.com]
>>>>> Sent: 01 February 2021 12:41
>>>>> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
>>>>> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
>>>>> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
>>>>> linuxarm@openeuler.org
>>>>> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in
>>> iommu_dev_xxx
>>>>> functions
>>>>>
>>>>> The device iommu probe/attach might have failed leaving dev->iommu
>>>>> to NULL and device drivers may still invoke these functions resulting
>>>>> in a crash in iommu vendor driver code. Hence make sure we check that.
>>>>>
>>>>> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
>>>>> is successfully associated with an iommu.
>>>>>
>>>>> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per
>> device")
>>>>> Signed-off-by: Shameer Kolothum
>>> <shameerali.kolothum.thodi@huawei.com>
>>>>> ---
>>>>> v1 --> v2:
>>>>>    -Added iommu_ops to struct dev_iommu based on the discussion with
>>> Robin.
>>>>>    -Rebased against iommu-tree core branch.
>>>>
>>>> A gentle ping on this...
>>>
>>> Is there a convincing justification for maintaining yet another copy of
>>> the ops pointer rather than simply dereferencing iommu_dev->ops at point
>>> of use?
>>>
>>
>> TBH, nothing I can think of now. That was mainly the way I interpreted your
>> suggestion
>> from the v1.  Now it looks like you didn’t mean it :). I am Ok to rework it to
>> dereference
>> it from iommu_dev. Please let me know.
> 
> So we can do something like this,
> 
> index fd76e2f579fe..5fd31a3cec18 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2865,10 +2865,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>    */
>   int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
>   {
> -       const struct iommu_ops *ops = dev->bus->iommu_ops;
> +       if (dev->iommu && dev->iommu->iommu_dev && dev->iommu->iommu_dev->ops)
> +               struct iommu_ops  *ops = dev->iommu->iommu_dev->ops;
>   
> -       if (ops && ops->dev_enable_feat)
> -               return ops->dev_enable_feat(dev, feat);
> +               if (ops->dev_enable_feat)
> +                       return ops->dev_enable_feat(dev, feat);
> +       }
>   
>          return -ENODEV;
>   }
> 
> Again, not sure we need to do the checking for iommu->dev and ops here. If the
> dev->iommu is set, is it safe to assume that we have a valid iommu->iommu_dev
> and ops always? (May be it is safer to do the checking in case something
> else breaks this assumption in future). Please let me know your thoughts.

I think it *could* happen that dev->iommu is set by iommu_fwspec_init() 
but iommu_probe_device() later refuses the device for whatever reason, 
so we would still need to check iommu->iommu_dev to be completely safe. 
We can assume iommu_dev->ops is valid, since if the IOMMU driver has 
returned something bogus from .probe_device then it's a major bug in 
that driver and crashing is the best indicator :)

Robin.

> 
> Thanks,
> Shameer
> 
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Robin Murphy]

On 2021-02-12 17:28, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Shameerali Kolothum Thodi
>> Sent: 12 February 2021 16:45
>> To: 'Robin Murphy' <robin.murphy@arm.com>; linux-kernel@vger.kernel.org;
>> iommu@lists.linux-foundation.org
>> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
>> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
>> Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
>>
>>
>>
>>> -----Original Message-----
>>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>>> Sent: 12 February 2021 16:39
>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>>> linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
>>> Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B)
>>> <prime.zeng@hisilicon.com>; linuxarm@openeuler.org
>>> Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx
>> functions
>>>
>>> On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
>>>> Hi Robin/Joerg,
>>>>
>>>>> -----Original Message-----
>>>>> From: Shameer Kolothum
>> [mailto:shameerali.kolothum.thodi@huawei.com]
>>>>> Sent: 01 February 2021 12:41
>>>>> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org
>>>>> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org;
>>>>> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>;
>>>>> linuxarm@openeuler.org
>>>>> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in
>>> iommu_dev_xxx
>>>>> functions
>>>>>
>>>>> The device iommu probe/attach might have failed leaving dev->iommu
>>>>> to NULL and device drivers may still invoke these functions resulting
>>>>> in a crash in iommu vendor driver code. Hence make sure we check that.
>>>>>
>>>>> Also added iommu_ops to the "struct dev_iommu" and set it if the dev
>>>>> is successfully associated with an iommu.
>>>>>
>>>>> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per
>> device")
>>>>> Signed-off-by: Shameer Kolothum
>>> <shameerali.kolothum.thodi@huawei.com>
>>>>> ---
>>>>> v1 --> v2:
>>>>>    -Added iommu_ops to struct dev_iommu based on the discussion with
>>> Robin.
>>>>>    -Rebased against iommu-tree core branch.
>>>>
>>>> A gentle ping on this...
>>>
>>> Is there a convincing justification for maintaining yet another copy of
>>> the ops pointer rather than simply dereferencing iommu_dev->ops at point
>>> of use?
>>>
>>
>> TBH, nothing I can think of now. That was mainly the way I interpreted your
>> suggestion
>> from the v1.  Now it looks like you didn’t mean it :). I am Ok to rework it to
>> dereference
>> it from iommu_dev. Please let me know.
> 
> So we can do something like this,
> 
> index fd76e2f579fe..5fd31a3cec18 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2865,10 +2865,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>    */
>   int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
>   {
> -       const struct iommu_ops *ops = dev->bus->iommu_ops;
> +       if (dev->iommu && dev->iommu->iommu_dev && dev->iommu->iommu_dev->ops)
> +               struct iommu_ops  *ops = dev->iommu->iommu_dev->ops;
>   
> -       if (ops && ops->dev_enable_feat)
> -               return ops->dev_enable_feat(dev, feat);
> +               if (ops->dev_enable_feat)
> +                       return ops->dev_enable_feat(dev, feat);
> +       }
>   
>          return -ENODEV;
>   }
> 
> Again, not sure we need to do the checking for iommu->dev and ops here. If the
> dev->iommu is set, is it safe to assume that we have a valid iommu->iommu_dev
> and ops always? (May be it is safer to do the checking in case something
> else breaks this assumption in future). Please let me know your thoughts.

I think it *could* happen that dev->iommu is set by iommu_fwspec_init() 
but iommu_probe_device() later refuses the device for whatever reason, 
so we would still need to check iommu->iommu_dev to be completely safe. 
We can assume iommu_dev->ops is valid, since if the IOMMU driver has 
returned something bogus from .probe_device then it's a major bug in 
that driver and crashing is the best indicator :)

Robin.

> 
> Thanks,
> Shameer
> 
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameer Kolothum]

The device iommu probe/attach might have failed leaving dev->iommu
to NULL and device drivers may still invoke these functions resulting
in a crash in iommu vendor driver code. Hence make sure we check that.

Also added iommu_ops to the "struct dev_iommu" and set it if the dev
is successfully associated with an iommu. 

Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
v1 --> v2:
 -Added iommu_ops to struct dev_iommu based on the discussion with Robin.
 -Rebased against iommu-tree core branch.
---
 drivers/iommu/iommu.c | 19 +++++++------------
 include/linux/iommu.h |  2 ++
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fd76e2f579fe..6023d0b7c542 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	}
 
 	dev->iommu->iommu_dev = iommu_dev;
+	dev->iommu->ops = iommu_dev->ops;
 
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group)) {
@@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
  */
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
-
-	if (ops && ops->dev_enable_feat)
-		return ops->dev_enable_feat(dev, feat);
+	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
+		return dev->iommu->ops->dev_enable_feat(dev, feat);
 
 	return -ENODEV;
 }
@@ -2881,10 +2880,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
  */
 int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
-
-	if (ops && ops->dev_disable_feat)
-		return ops->dev_disable_feat(dev, feat);
+	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
+		return dev->iommu->ops->dev_disable_feat(dev, feat);
 
 	return -EBUSY;
 }
@@ -2892,10 +2889,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 
 bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
-
-	if (ops && ops->dev_feat_enabled)
-		return ops->dev_feat_enabled(dev, feat);
+	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
+		return dev->iommu->ops->dev_feat_enabled(dev, feat);
 
 	return false;
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 524ffc2ff64f..ff0c76bdfb67 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -354,6 +354,7 @@ struct iommu_fault_param {
  * @fault_param: IOMMU detected device fault reporting data
  * @fwspec:	 IOMMU fwspec data
  * @iommu_dev:	 IOMMU device this device is linked to
+ * @ops:	 iommu-ops for talking to the iommu_dev
  * @priv:	 IOMMU Driver private data
  *
  * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
@@ -364,6 +365,7 @@ struct dev_iommu {
 	struct iommu_fault_param	*fault_param;
 	struct iommu_fwspec		*fwspec;
 	struct iommu_device		*iommu_dev;
+	const struct iommu_ops		*ops;
 	void				*priv;
 };
 
-- 
2.17.1

[PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

[Shameer Kolothum]

The device iommu probe/attach might have failed leaving dev->iommu
to NULL and device drivers may still invoke these functions resulting
in a crash in iommu vendor driver code. Hence make sure we check that.

Also added iommu_ops to the "struct dev_iommu" and set it if the dev
is successfully associated with an iommu. 

Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
v1 --> v2:
 -Added iommu_ops to struct dev_iommu based on the discussion with Robin.
 -Rebased against iommu-tree core branch.
---
 drivers/iommu/iommu.c | 19 +++++++------------
 include/linux/iommu.h |  2 ++
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fd76e2f579fe..6023d0b7c542 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	}
 
 	dev->iommu->iommu_dev = iommu_dev;
+	dev->iommu->ops = iommu_dev->ops;
 
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group)) {
@@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
  */
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
-
-	if (ops && ops->dev_enable_feat)
-		return ops->dev_enable_feat(dev, feat);
+	if (dev->iommu && dev->iommu->ops->dev_enable_feat)
+		return dev->iommu->ops->dev_enable_feat(dev, feat);
 
 	return -ENODEV;
 }
@@ -2881,10 +2880,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
  */
 int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
-
-	if (ops && ops->dev_disable_feat)
-		return ops->dev_disable_feat(dev, feat);
+	if (dev->iommu && dev->iommu->ops->dev_disable_feat)
+		return dev->iommu->ops->dev_disable_feat(dev, feat);
 
 	return -EBUSY;
 }
@@ -2892,10 +2889,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 
 bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat)
 {
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
-
-	if (ops && ops->dev_feat_enabled)
-		return ops->dev_feat_enabled(dev, feat);
+	if (dev->iommu && dev->iommu->ops->dev_feat_enabled)
+		return dev->iommu->ops->dev_feat_enabled(dev, feat);
 
 	return false;
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 524ffc2ff64f..ff0c76bdfb67 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -354,6 +354,7 @@ struct iommu_fault_param {
  * @fault_param: IOMMU detected device fault reporting data
  * @fwspec:	 IOMMU fwspec data
  * @iommu_dev:	 IOMMU device this device is linked to
+ * @ops:	 iommu-ops for talking to the iommu_dev
  * @priv:	 IOMMU Driver private data
  *
  * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
@@ -364,6 +365,7 @@ struct dev_iommu {
 	struct iommu_fault_param	*fault_param;
 	struct iommu_fwspec		*fwspec;
 	struct iommu_device		*iommu_dev;
+	const struct iommu_ops		*ops;
 	void				*priv;
 };
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu