Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

On Fri, Mar 9, 2018 at 6:04 PM, Robin Murphy  wrote:
> On 09/03/18 07:11, Vivek Gautam wrote:
>>
>> On Thu, Mar 8, 2018 at 10:29 AM, Vivek Gautam
>>  wrote:
>>>
>>> On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy 
>>> wrote:

 On 02/03/18 10:10, Vivek Gautam wrote:
>
>
> From: Sricharan R 
>
> Finally add the device link between the master device and
> smmu, so that the smmu gets runtime enabled/disabled only when the
> master needs it. This is done from add_device callback which gets
> called once when the master is added to the smmu.
>
> Signed-off-by: Sricharan R 
> Signed-off-by: Vivek Gautam 
> ---
>drivers/iommu/arm-smmu.c | 21 +
>1 file changed, 21 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 3d6a1875431f..bb1ea82c1003 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -217,6 +217,9 @@ struct arm_smmu_device {
>  /* IOMMU core code handle */
>  struct iommu_device iommu;
> +
> +   /* runtime PM link to master */
> +   struct device_link *link;



 Just the one?
>>
>>
>> we will either have to count all the devices that are present on the
>> iommu bus, or
>> maintain a list to which all the links can be added.
>> But to add the list, we will have to initialize a LIST_HEAD in struct
>> device_link
>> as well.
>>
>> Or, I think we don't even need to maintain a pointer to link with smmu.
>> In arm_smmu_remove_device(), we can find out the correct link, and delete
>> it.
>>
>>  list_for_each_entry(link, >links.suppliers, c_node)
>>  if (link->supplier == smmu->dev);
>> device_link_del(link);
>>
>> Should that be fine?
>>
>> Rafael, does the above snippet looks right to you? Context: smmu->dev
>> is the supplier, and dev is the consumer. We want to find the link,
>> and delete it.
>
>
> Actually, looking at the existing code, it seems like device_link_add() will
> in fact look up and return any existing link between a given supplier and
> consumer - is that intentional API behaviour that users may rely on to avoid
> keeping track of explicit link pointers?
> (or conversely, might it be
> reasonable to factor out a device_link_find() function?)

Yea, that sounds better.

regards
Vivek

>
> Robin.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

[Robin Murphy]

On 09/03/18 07:11, Vivek Gautam wrote:

On Thu, Mar 8, 2018 at 10:29 AM, Vivek Gautam
 wrote:

On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy  wrote:

On 02/03/18 10:10, Vivek Gautam wrote:


From: Sricharan R 

Finally add the device link between the master device and
smmu, so that the smmu gets runtime enabled/disabled only when the
master needs it. This is done from add_device callback which gets
called once when the master is added to the smmu.

Signed-off-by: Sricharan R 
Signed-off-by: Vivek Gautam 
---
   drivers/iommu/arm-smmu.c | 21 +
   1 file changed, 21 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 3d6a1875431f..bb1ea82c1003 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -217,6 +217,9 @@ struct arm_smmu_device {
 /* IOMMU core code handle */
 struct iommu_device iommu;
+
+   /* runtime PM link to master */
+   struct device_link *link;



Just the one?


we will either have to count all the devices that are present on the
iommu bus, or
maintain a list to which all the links can be added.
But to add the list, we will have to initialize a LIST_HEAD in struct
device_link
as well.

Or, I think we don't even need to maintain a pointer to link with smmu.
In arm_smmu_remove_device(), we can find out the correct link, and delete it.

 list_for_each_entry(link, >links.suppliers, c_node)
 if (link->supplier == smmu->dev);
device_link_del(link);

Should that be fine?

Rafael, does the above snippet looks right to you? Context: smmu->dev
is the supplier, and dev is the consumer. We want to find the link,
and delete it.


Actually, looking at the existing code, it seems like device_link_add() 
will in fact look up and return any existing link between a given 
supplier and consumer - is that intentional API behaviour that users may 
rely on to avoid keeping track of explicit link pointers? (or 
conversely, might it be reasonable to factor out a device_link_find() 
function?)


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

Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy  wrote:
> On 02/03/18 10:10, Vivek Gautam wrote:
>>
>> From: Sricharan R 
>>
>> Finally add the device link between the master device and
>> smmu, so that the smmu gets runtime enabled/disabled only when the
>> master needs it. This is done from add_device callback which gets
>> called once when the master is added to the smmu.
>>
>> Signed-off-by: Sricharan R 
>> Signed-off-by: Vivek Gautam 
>> ---
>>   drivers/iommu/arm-smmu.c | 21 +
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 3d6a1875431f..bb1ea82c1003 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -217,6 +217,9 @@ struct arm_smmu_device {
>> /* IOMMU core code handle */
>> struct iommu_device iommu;
>> +
>> +   /* runtime PM link to master */
>> +   struct device_link *link;
>
>
> Just the one?
>
>>   };
>> enum arm_smmu_context_fmt {
>> @@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev)
>> iommu_device_link(>iommu, dev);
>>   + /*
>> +* Establish the link between smmu and master, so that the
>> +* smmu gets runtime enabled/disabled as per the master's
>> +* needs.
>> +*/
>> +   smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>
>
> Maybe I've misunderstood how the API works, but AFAICS the second and
> subsequent devices are all just going to overwrite (and leak) the link of
> the previous one...

Also, noticed one more thing while testing on sdm845. When we are
conditionally enabling the runtime pm, we should create the device
link too conditionally, i.e. only in the case the smmu->dev has
runtime pm_enabled we can create this device link between smmu and the
master device.
Otherwise when the master tries to do a pm_runtime_get() over itself,
the device link will ensure that pm_runtime_get() for smmu is done
first. But that will fail when we don't have pm runtime enabled over
smmu, and so the master device's pm_runtime_get() will fail too.
Will fix this in the next version.

Thanks
Vivek

>
>> +   if (!smmu->link) {
>> +   dev_warn(smmu->dev, "Unable to create device link between
>> %s and %s\n",
>> +dev_name(smmu->dev), dev_name(dev));
>> +   ret = -ENODEV;
>> +   goto out_unlink;
>> +   }
>> +
>> arm_smmu_rpm_put(smmu);
>> return 0;
>>   +out_unlink:
>> +   iommu_device_unlink(>iommu, dev);
>> +   arm_smmu_master_free_smes(fwspec);
>>   out_rpm_put:
>> arm_smmu_rpm_put(smmu);
>>   out_cfg_free:
>> @@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device
>> *dev)
>> cfg  = fwspec->iommu_priv;
>> smmu = cfg->smmu;
>>   + device_link_del(smmu->link);
>
>
> ...and equivalently you end up with a double-free (or more) here of a link
> which may not have belonged to dev anyway.
>
> Robin.
>
>
>> +
>> ret = arm_smmu_rpm_get(smmu);
>> if (ret < 0)
>> return;
>>
>



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

On Thu, Mar 8, 2018 at 10:29 AM, Vivek Gautam
 wrote:
> On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy  wrote:
>> On 02/03/18 10:10, Vivek Gautam wrote:
>>>
>>> From: Sricharan R 
>>>
>>> Finally add the device link between the master device and
>>> smmu, so that the smmu gets runtime enabled/disabled only when the
>>> master needs it. This is done from add_device callback which gets
>>> called once when the master is added to the smmu.
>>>
>>> Signed-off-by: Sricharan R 
>>> Signed-off-by: Vivek Gautam 
>>> ---
>>>   drivers/iommu/arm-smmu.c | 21 +
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 3d6a1875431f..bb1ea82c1003 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -217,6 +217,9 @@ struct arm_smmu_device {
>>> /* IOMMU core code handle */
>>> struct iommu_device iommu;
>>> +
>>> +   /* runtime PM link to master */
>>> +   struct device_link *link;
>>
>>
>> Just the one?

we will either have to count all the devices that are present on the
iommu bus, or
maintain a list to which all the links can be added.
But to add the list, we will have to initialize a LIST_HEAD in struct
device_link
as well.

Or, I think we don't even need to maintain a pointer to link with smmu.
In arm_smmu_remove_device(), we can find out the correct link, and delete it.

list_for_each_entry(link, >links.suppliers, c_node)
if (link->supplier == smmu->dev);
   device_link_del(link);

Should that be fine?

Rafael, does the above snippet looks right to you? Context: smmu->dev
is the supplier, and dev is the consumer. We want to find the link,
and delete it.

regards
Vivek

>>
>>>   };
>>> enum arm_smmu_context_fmt {
>>> @@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev)
>>> iommu_device_link(>iommu, dev);
>>>   + /*
>>> +* Establish the link between smmu and master, so that the
>>> +* smmu gets runtime enabled/disabled as per the master's
>>> +* needs.
>>> +*/
>>> +   smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>>
>>
>> Maybe I've misunderstood how the API works, but AFAICS the second and
>> subsequent devices are all just going to overwrite (and leak) the link of
>> the previous one...
>
> Sorry, my bad. Will take care of this.
>
> regards
> Vivek
>
>>
>>> +   if (!smmu->link) {
>>> +   dev_warn(smmu->dev, "Unable to create device link between
>>> %s and %s\n",
>>> +dev_name(smmu->dev), dev_name(dev));
>>> +   ret = -ENODEV;
>>> +   goto out_unlink;
>>> +   }
>>> +
>>> arm_smmu_rpm_put(smmu);
>>> return 0;
>>>   +out_unlink:
>>> +   iommu_device_unlink(>iommu, dev);
>>> +   arm_smmu_master_free_smes(fwspec);
>>>   out_rpm_put:
>>> arm_smmu_rpm_put(smmu);
>>>   out_cfg_free:
>>> @@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device
>>> *dev)
>>> cfg  = fwspec->iommu_priv;
>>> smmu = cfg->smmu;
>>>   + device_link_del(smmu->link);
>>
>>
>> ...and equivalently you end up with a double-free (or more) here of a link
>> which may not have belonged to dev anyway.
>>
>> Robin.
>>
>>
>>> +
>>> ret = arm_smmu_rpm_get(smmu);
>>> if (ret < 0)
>>> return;
>>>
>>
>
>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy  wrote:
> On 02/03/18 10:10, Vivek Gautam wrote:
>>
>> From: Sricharan R 
>>
>> Finally add the device link between the master device and
>> smmu, so that the smmu gets runtime enabled/disabled only when the
>> master needs it. This is done from add_device callback which gets
>> called once when the master is added to the smmu.
>>
>> Signed-off-by: Sricharan R 
>> Signed-off-by: Vivek Gautam 
>> ---
>>   drivers/iommu/arm-smmu.c | 21 +
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 3d6a1875431f..bb1ea82c1003 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -217,6 +217,9 @@ struct arm_smmu_device {
>> /* IOMMU core code handle */
>> struct iommu_device iommu;
>> +
>> +   /* runtime PM link to master */
>> +   struct device_link *link;
>
>
> Just the one?
>
>>   };
>> enum arm_smmu_context_fmt {
>> @@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev)
>> iommu_device_link(>iommu, dev);
>>   + /*
>> +* Establish the link between smmu and master, so that the
>> +* smmu gets runtime enabled/disabled as per the master's
>> +* needs.
>> +*/
>> +   smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>
>
> Maybe I've misunderstood how the API works, but AFAICS the second and
> subsequent devices are all just going to overwrite (and leak) the link of
> the previous one...

Sorry, my bad. Will take care of this.

regards
Vivek

>
>> +   if (!smmu->link) {
>> +   dev_warn(smmu->dev, "Unable to create device link between
>> %s and %s\n",
>> +dev_name(smmu->dev), dev_name(dev));
>> +   ret = -ENODEV;
>> +   goto out_unlink;
>> +   }
>> +
>> arm_smmu_rpm_put(smmu);
>> return 0;
>>   +out_unlink:
>> +   iommu_device_unlink(>iommu, dev);
>> +   arm_smmu_master_free_smes(fwspec);
>>   out_rpm_put:
>> arm_smmu_rpm_put(smmu);
>>   out_cfg_free:
>> @@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device
>> *dev)
>> cfg  = fwspec->iommu_priv;
>> smmu = cfg->smmu;
>>   + device_link_del(smmu->link);
>
>
> ...and equivalently you end up with a double-free (or more) here of a link
> which may not have belonged to dev anyway.
>
> Robin.
>
>
>> +
>> ret = arm_smmu_rpm_get(smmu);
>> if (ret < 0)
>> return;
>>
>



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

[Robin Murphy]

On 02/03/18 10:10, Vivek Gautam wrote:

From: Sricharan R 

Finally add the device link between the master device and
smmu, so that the smmu gets runtime enabled/disabled only when the
master needs it. This is done from add_device callback which gets
called once when the master is added to the smmu.

Signed-off-by: Sricharan R 
Signed-off-by: Vivek Gautam 
---
  drivers/iommu/arm-smmu.c | 21 +
  1 file changed, 21 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 3d6a1875431f..bb1ea82c1003 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -217,6 +217,9 @@ struct arm_smmu_device {
  
  	/* IOMMU core code handle */

struct iommu_device iommu;
+
+   /* runtime PM link to master */
+   struct device_link *link;


Just the one?


  };
  
  enum arm_smmu_context_fmt {

@@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev)
  
  	iommu_device_link(>iommu, dev);
  
+	/*

+* Establish the link between smmu and master, so that the
+* smmu gets runtime enabled/disabled as per the master's
+* needs.
+*/
+   smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);


Maybe I've misunderstood how the API works, but AFAICS the second and 
subsequent devices are all just going to overwrite (and leak) the link 
of the previous one...



+   if (!smmu->link) {
+   dev_warn(smmu->dev, "Unable to create device link between %s and 
%s\n",
+dev_name(smmu->dev), dev_name(dev));
+   ret = -ENODEV;
+   goto out_unlink;
+   }
+
arm_smmu_rpm_put(smmu);
  
  	return 0;
  
+out_unlink:

+   iommu_device_unlink(>iommu, dev);
+   arm_smmu_master_free_smes(fwspec);
  out_rpm_put:
arm_smmu_rpm_put(smmu);
  out_cfg_free:
@@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device *dev)
cfg  = fwspec->iommu_priv;
smmu = cfg->smmu;
  
+	device_link_del(smmu->link);


...and equivalently you end up with a double-free (or more) here of a 
link which may not have belonged to dev anyway.


Robin.


+
ret = arm_smmu_rpm_get(smmu);
if (ret < 0)
return;


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