Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 10/02/17 15:33, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 02:35:39PM +, Robin Murphy wrote:
>> On 09/02/17 11:32, Joerg Roedel wrote:
>>> +   ret = iommu_device_sysfs_add(>iommu, iommu->dev, NULL,
>>> +"msm-smmu.%pa", );
>>> +   if (ret) {
>>> +   pr_err("Could not add msm-smmu at %pa to sysfs\n", );
>>> +   goto fail;
>>> +   }
>>
>> Nit: there's a bit of inconsistency with printing errors between the
>> various drivers (for both _sysfs_add and _register). I reckon if we want
>> error messages we may as well just fold them into the helper functions.
> 
> Yeah, this could be unified too. For now I looked how verbose the
> driver was that I was going to change and added messages to be
> consistent inside the drivers.
> 
>>
>>> +
>>> +   iommu_device_set_ops(>iommu, _iommu_ops);
>>> +   iommu_device_set_fwnode(>iommu, >dev.of_node->fwnode);
>>> +
>>> +   ret = iommu_device_register(>iommu);
>>> +   if (ret) {
>>> +   pr_err("Could not register msm-smmu at %pa\n", );
>>> +   goto fail;
>>> +   }
>>
>> I think there's a corresponding unregister missing for
>> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
>> back). I know it's not strictly a problem at the moment, but I do now
>> have IOMMU-drivers-as-modules working on top of the probe deferral
>> series... ;)
> 
> Well, that there was an iommu_register_instance() without any
> unregistration interface at all makes me believe that unregistering
> iommus is not really implemented yet.
> 
> And in fact, the remove functions for msm and arm-smmu seem to only
> disable the hardware, but are not removing the corresponding data
> structures.

For the ARM SMMUs at least, the SMMU-specific data is (well, should be)
all devm_* managed, thus freed automatically by the driver core after
remove() returns. It is true that there's an implicit expectation that
the SMMU won't be removed until all domains, groups and masters have
been explicitly torn down by the relevant detach()/remove()/free()
calls, although I guess the sysfs links might actually help enforce that.

> So I think we are fine from that side.

Sure, I mostly just wanted not to lose sight of the future possibility
of unloadable IOMMU drivers (admittedly I've not even tried that yet,
only loading them post-boot).

Robin.

> 
> 
>   Joerg
> 

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 10/02/17 15:33, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 02:35:39PM +, Robin Murphy wrote:
>> On 09/02/17 11:32, Joerg Roedel wrote:
>>> +   ret = iommu_device_sysfs_add(>iommu, iommu->dev, NULL,
>>> +"msm-smmu.%pa", );
>>> +   if (ret) {
>>> +   pr_err("Could not add msm-smmu at %pa to sysfs\n", );
>>> +   goto fail;
>>> +   }
>>
>> Nit: there's a bit of inconsistency with printing errors between the
>> various drivers (for both _sysfs_add and _register). I reckon if we want
>> error messages we may as well just fold them into the helper functions.
> 
> Yeah, this could be unified too. For now I looked how verbose the
> driver was that I was going to change and added messages to be
> consistent inside the drivers.
> 
>>
>>> +
>>> +   iommu_device_set_ops(>iommu, _iommu_ops);
>>> +   iommu_device_set_fwnode(>iommu, >dev.of_node->fwnode);
>>> +
>>> +   ret = iommu_device_register(>iommu);
>>> +   if (ret) {
>>> +   pr_err("Could not register msm-smmu at %pa\n", );
>>> +   goto fail;
>>> +   }
>>
>> I think there's a corresponding unregister missing for
>> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
>> back). I know it's not strictly a problem at the moment, but I do now
>> have IOMMU-drivers-as-modules working on top of the probe deferral
>> series... ;)
> 
> Well, that there was an iommu_register_instance() without any
> unregistration interface at all makes me believe that unregistering
> iommus is not really implemented yet.
> 
> And in fact, the remove functions for msm and arm-smmu seem to only
> disable the hardware, but are not removing the corresponding data
> structures.

For the ARM SMMUs at least, the SMMU-specific data is (well, should be)
all devm_* managed, thus freed automatically by the driver core after
remove() returns. It is true that there's an implicit expectation that
the SMMU won't be removed until all domains, groups and masters have
been explicitly torn down by the relevant detach()/remove()/free()
calls, although I guess the sysfs links might actually help enforce that.

> So I think we are fine from that side.

Sure, I mostly just wanted not to lose sight of the future possibility
of unloadable IOMMU drivers (admittedly I've not even tried that yet,
only loading them post-boot).

Robin.

> 
> 
>   Joerg
> 

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Joerg Roedel]

On Fri, Feb 10, 2017 at 02:35:39PM +, Robin Murphy wrote:
> On 09/02/17 11:32, Joerg Roedel wrote:
> > +   ret = iommu_device_sysfs_add(>iommu, iommu->dev, NULL,
> > +"msm-smmu.%pa", );
> > +   if (ret) {
> > +   pr_err("Could not add msm-smmu at %pa to sysfs\n", );
> > +   goto fail;
> > +   }
> 
> Nit: there's a bit of inconsistency with printing errors between the
> various drivers (for both _sysfs_add and _register). I reckon if we want
> error messages we may as well just fold them into the helper functions.

Yeah, this could be unified too. For now I looked how verbose the
driver was that I was going to change and added messages to be
consistent inside the drivers.

> 
> > +
> > +   iommu_device_set_ops(>iommu, _iommu_ops);
> > +   iommu_device_set_fwnode(>iommu, >dev.of_node->fwnode);
> > +
> > +   ret = iommu_device_register(>iommu);
> > +   if (ret) {
> > +   pr_err("Could not register msm-smmu at %pa\n", );
> > +   goto fail;
> > +   }
> 
> I think there's a corresponding unregister missing for
> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
> back). I know it's not strictly a problem at the moment, but I do now
> have IOMMU-drivers-as-modules working on top of the probe deferral
> series... ;)

Well, that there was an iommu_register_instance() without any
unregistration interface at all makes me believe that unregistering
iommus is not really implemented yet.

And in fact, the remove functions for msm and arm-smmu seem to only
disable the hardware, but are not removing the corresponding data
structures.

So I think we are fine from that side.


Joerg

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Joerg Roedel]

On Fri, Feb 10, 2017 at 02:35:39PM +, Robin Murphy wrote:
> On 09/02/17 11:32, Joerg Roedel wrote:
> > +   ret = iommu_device_sysfs_add(>iommu, iommu->dev, NULL,
> > +"msm-smmu.%pa", );
> > +   if (ret) {
> > +   pr_err("Could not add msm-smmu at %pa to sysfs\n", );
> > +   goto fail;
> > +   }
> 
> Nit: there's a bit of inconsistency with printing errors between the
> various drivers (for both _sysfs_add and _register). I reckon if we want
> error messages we may as well just fold them into the helper functions.

Yeah, this could be unified too. For now I looked how verbose the
driver was that I was going to change and added messages to be
consistent inside the drivers.

> 
> > +
> > +   iommu_device_set_ops(>iommu, _iommu_ops);
> > +   iommu_device_set_fwnode(>iommu, >dev.of_node->fwnode);
> > +
> > +   ret = iommu_device_register(>iommu);
> > +   if (ret) {
> > +   pr_err("Could not register msm-smmu at %pa\n", );
> > +   goto fail;
> > +   }
> 
> I think there's a corresponding unregister missing for
> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
> back). I know it's not strictly a problem at the moment, but I do now
> have IOMMU-drivers-as-modules working on top of the probe deferral
> series... ;)

Well, that there was an iommu_register_instance() without any
unregistration interface at all makes me believe that unregistering
iommus is not really implemented yet.

And in fact, the remove functions for msm and arm-smmu seem to only
disable the hardware, but are not removing the corresponding data
structures.

So I think we are fine from that side.


Joerg

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 09/02/17 11:32, Joerg Roedel wrote:
> From: Joerg Roedel 
> 
> Register the MSM IOMMUs to the iommu core and add sysfs
> entries for that driver.
> 
> Signed-off-by: Joerg Roedel 
> ---
>  drivers/iommu/msm_iommu.c | 73 
> +++
>  drivers/iommu/msm_iommu.h |  3 ++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index b09692b..30795cb 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -371,6 +371,58 @@ static int msm_iommu_domain_config(struct msm_priv *priv)
>   return 0;
>  }
>  
> +/* Must be called under msm_iommu_lock */
> +static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
> +{
> + struct msm_iommu_dev *iommu, *ret = NULL;
> + struct msm_iommu_ctx_dev *master;
> +
> + list_for_each_entry(iommu, _iommu_devices, dev_node) {
> + master = list_first_entry(>ctx_list,
> +   struct msm_iommu_ctx_dev,
> +   list);
> + if (master->of_node == dev->of_node) {
> + ret = iommu;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int msm_iommu_add_device(struct device *dev)
> +{
> + struct msm_iommu_dev *iommu;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(_iommu_lock, flags);
> +
> + iommu = find_iommu_for_dev(dev);
> + if (iommu)
> + iommu_device_link(>iommu, dev);
> + else
> + ret = -ENODEV;
> +
> + spin_unlock_irqrestore(_iommu_lock, flags);
> +
> + return ret;
> +}
> +
> +static void msm_iommu_remove_device(struct device *dev)
> +{
> + struct msm_iommu_dev *iommu;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_iommu_lock, flags);
> +
> + iommu = find_iommu_for_dev(dev);
> + if (iommu)
> + iommu_device_unlink(>iommu, dev);
> +
> + spin_unlock_irqrestore(_iommu_lock, flags);
> +}
> +
>  static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device 
> *dev)
>  {
>   int ret = 0;
> @@ -646,6 +698,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>   .unmap = msm_iommu_unmap,
>   .map_sg = default_iommu_map_sg,
>   .iova_to_phys = msm_iommu_iova_to_phys,
> + .add_device = msm_iommu_add_device,
> + .remove_device = msm_iommu_remove_device,
>   .pgsize_bitmap = MSM_IOMMU_PGSIZES,
>   .of_xlate = qcom_iommu_of_xlate,
>  };
> @@ -653,6 +707,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>  static int msm_iommu_probe(struct platform_device *pdev)
>  {
>   struct resource *r;
> + resource_size_t ioaddr;
>   struct msm_iommu_dev *iommu;
>   int ret, par, val;
>  
> @@ -696,6 +751,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
>   ret = PTR_ERR(iommu->base);
>   goto fail;
>   }
> + ioaddr = r->start;
>  
>   iommu->irq = platform_get_irq(pdev, 0);
>   if (iommu->irq < 0) {
> @@ -737,6 +793,23 @@ static int msm_iommu_probe(struct platform_device *pdev)
>   }
>  
>   list_add(>dev_node, _iommu_devices);
> +
> + ret = iommu_device_sysfs_add(>iommu, iommu->dev, NULL,
> +  "msm-smmu.%pa", );
> + if (ret) {
> + pr_err("Could not add msm-smmu at %pa to sysfs\n", );
> + goto fail;
> + }

Nit: there's a bit of inconsistency with printing errors between the
various drivers (for both _sysfs_add and _register). I reckon if we want
error messages we may as well just fold them into the helper functions.

> +
> + iommu_device_set_ops(>iommu, _iommu_ops);
> + iommu_device_set_fwnode(>iommu, >dev.of_node->fwnode);
> +
> + ret = iommu_device_register(>iommu);
> + if (ret) {
> + pr_err("Could not register msm-smmu at %pa\n", );
> + goto fail;
> + }

I think there's a corresponding unregister missing for
msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
back). I know it's not strictly a problem at the moment, but I do now
have IOMMU-drivers-as-modules working on top of the probe deferral
series... ;)

Robin.

> +
>   of_iommu_set_ops(pdev->dev.of_node, _iommu_ops);
>  
>   pr_info("device mapped at %p, irq %d with %d ctx banks\n",
> diff --git a/drivers/iommu/msm_iommu.h b/drivers/iommu/msm_iommu.h
> index 4ca25d5..ae92d27 100644
> --- a/drivers/iommu/msm_iommu.h
> +++ b/drivers/iommu/msm_iommu.h
> @@ -19,6 +19,7 @@
>  #define MSM_IOMMU_H
>  
>  #include 
> +#include 
>  #include 
>  
>  /* Sharability attributes of MSM IOMMU mappings */
> @@ -68,6 +69,8 @@ struct msm_iommu_dev {
>   struct list_head dom_node;
>   struct list_head ctx_list;
>   DECLARE_BITMAP(context_map, IOMMU_MAX_CBS);
> +
> + struct iommu_device iommu;
>  };
>  
>  /**
> 

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 09/02/17 11:32, Joerg Roedel wrote:
> From: Joerg Roedel 
> 
> Register the MSM IOMMUs to the iommu core and add sysfs
> entries for that driver.
> 
> Signed-off-by: Joerg Roedel 
> ---
>  drivers/iommu/msm_iommu.c | 73 
> +++
>  drivers/iommu/msm_iommu.h |  3 ++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index b09692b..30795cb 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -371,6 +371,58 @@ static int msm_iommu_domain_config(struct msm_priv *priv)
>   return 0;
>  }
>  
> +/* Must be called under msm_iommu_lock */
> +static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
> +{
> + struct msm_iommu_dev *iommu, *ret = NULL;
> + struct msm_iommu_ctx_dev *master;
> +
> + list_for_each_entry(iommu, _iommu_devices, dev_node) {
> + master = list_first_entry(>ctx_list,
> +   struct msm_iommu_ctx_dev,
> +   list);
> + if (master->of_node == dev->of_node) {
> + ret = iommu;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int msm_iommu_add_device(struct device *dev)
> +{
> + struct msm_iommu_dev *iommu;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(_iommu_lock, flags);
> +
> + iommu = find_iommu_for_dev(dev);
> + if (iommu)
> + iommu_device_link(>iommu, dev);
> + else
> + ret = -ENODEV;
> +
> + spin_unlock_irqrestore(_iommu_lock, flags);
> +
> + return ret;
> +}
> +
> +static void msm_iommu_remove_device(struct device *dev)
> +{
> + struct msm_iommu_dev *iommu;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_iommu_lock, flags);
> +
> + iommu = find_iommu_for_dev(dev);
> + if (iommu)
> + iommu_device_unlink(>iommu, dev);
> +
> + spin_unlock_irqrestore(_iommu_lock, flags);
> +}
> +
>  static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device 
> *dev)
>  {
>   int ret = 0;
> @@ -646,6 +698,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>   .unmap = msm_iommu_unmap,
>   .map_sg = default_iommu_map_sg,
>   .iova_to_phys = msm_iommu_iova_to_phys,
> + .add_device = msm_iommu_add_device,
> + .remove_device = msm_iommu_remove_device,
>   .pgsize_bitmap = MSM_IOMMU_PGSIZES,
>   .of_xlate = qcom_iommu_of_xlate,
>  };
> @@ -653,6 +707,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>  static int msm_iommu_probe(struct platform_device *pdev)
>  {
>   struct resource *r;
> + resource_size_t ioaddr;
>   struct msm_iommu_dev *iommu;
>   int ret, par, val;
>  
> @@ -696,6 +751,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
>   ret = PTR_ERR(iommu->base);
>   goto fail;
>   }
> + ioaddr = r->start;
>  
>   iommu->irq = platform_get_irq(pdev, 0);
>   if (iommu->irq < 0) {
> @@ -737,6 +793,23 @@ static int msm_iommu_probe(struct platform_device *pdev)
>   }
>  
>   list_add(>dev_node, _iommu_devices);
> +
> + ret = iommu_device_sysfs_add(>iommu, iommu->dev, NULL,
> +  "msm-smmu.%pa", );
> + if (ret) {
> + pr_err("Could not add msm-smmu at %pa to sysfs\n", );
> + goto fail;
> + }

Nit: there's a bit of inconsistency with printing errors between the
various drivers (for both _sysfs_add and _register). I reckon if we want
error messages we may as well just fold them into the helper functions.

> +
> + iommu_device_set_ops(>iommu, _iommu_ops);
> + iommu_device_set_fwnode(>iommu, >dev.of_node->fwnode);
> +
> + ret = iommu_device_register(>iommu);
> + if (ret) {
> + pr_err("Could not register msm-smmu at %pa\n", );
> + goto fail;
> + }

I think there's a corresponding unregister missing for
msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
back). I know it's not strictly a problem at the moment, but I do now
have IOMMU-drivers-as-modules working on top of the probe deferral
series... ;)

Robin.

> +
>   of_iommu_set_ops(pdev->dev.of_node, _iommu_ops);
>  
>   pr_info("device mapped at %p, irq %d with %d ctx banks\n",
> diff --git a/drivers/iommu/msm_iommu.h b/drivers/iommu/msm_iommu.h
> index 4ca25d5..ae92d27 100644
> --- a/drivers/iommu/msm_iommu.h
> +++ b/drivers/iommu/msm_iommu.h
> @@ -19,6 +19,7 @@
>  #define MSM_IOMMU_H
>  
>  #include 
> +#include 
>  #include 
>  
>  /* Sharability attributes of MSM IOMMU mappings */
> @@ -68,6 +69,8 @@ struct msm_iommu_dev {
>   struct list_head dom_node;
>   struct list_head ctx_list;
>   DECLARE_BITMAP(context_map, IOMMU_MAX_CBS);
> +
> + struct iommu_device iommu;
>  };
>  
>  /**
>