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

2018-02-02 Thread Sricharan R 
Hi Robin,

On 2/2/2018 5:01 PM, Robin Murphy wrote:
> On 02/02/18 05:40, Sricharan R wrote:
>> Hi Robin/Vivek,
>>
>> On 2/1/2018 2:23 PM, Vivek Gautam wrote:
>>> Hi,
>>>
>>>
>>> On 1/31/2018 6:39 PM, Robin Murphy wrote:
 On 19/01/18 11:43, 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.

 Don't we need to balance this with a device_link_del() in .remove_device 
 (like exynos-iommu does)?
>>>
>>> Right. Will add device_link_del() call. Thanks for pointing out.
>>
>>   The reason for not adding device_link_del from .remove_device was, the 
>> core device_del
>>   which calls the .remove_device from notifier, calls device_links_purge 
>> before that.
>>   That does the same thing as device_link_del. So by the time .remove_device 
>> is called,
>>   device_links for that device is already cleaned up. Vivek, you may want to 
>> check once that
>>   calling device_link_del from .remove_device has no effect, just to confirm 
>> once more.
> 
> There is at least one path in which .remove_device is not called via the 
> notifier from device_del(), which is in the cleanup path of iommu_bus_init(). 
> AFAICS any links created by .add_device during that process would be left 
> dangling, because the device(s) would be live but otherwise disassociated 
> from the IOMMU afterwards.
> 
> From a maintenance perspective it's easier to have the call in its logical 
> place even if it does nothing 99% of the time; that way we shouldn't have to 
> keep an eye out for subtle changes in the power management code or driver 
> core that might invalidate the device_del() reasoning above, and the power 
> management guys shouldn't have to comprehend the internals of the IOMMU API 
> to make sense of the unbalanced call if they ever want to change their API.

 Ha, for a moment was thinking that with probe deferral add/remove_iommu_group 
in iommu_bus_init is dummy.
 But that may not be true for all Archs.
 Surely agree for the maintainability reason as well. Thanks.

Regards,
 Sricharan

-- 
"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 v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

2018-02-02 Thread Robin Murphy 

On 02/02/18 05:40, Sricharan R wrote:

Hi Robin/Vivek,

On 2/1/2018 2:23 PM, Vivek Gautam wrote:

Hi,


On 1/31/2018 6:39 PM, Robin Murphy wrote:

On 19/01/18 11:43, 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.


Don't we need to balance this with a device_link_del() in .remove_device (like 
exynos-iommu does)?


Right. Will add device_link_del() call. Thanks for pointing out.


  The reason for not adding device_link_del from .remove_device was, the core 
device_del
  which calls the .remove_device from notifier, calls device_links_purge before 
that.
  That does the same thing as device_link_del. So by the time .remove_device is 
called,
  device_links for that device is already cleaned up. Vivek, you may want to 
check once that
  calling device_link_del from .remove_device has no effect, just to confirm 
once more.


There is at least one path in which .remove_device is not called via the 
notifier from device_del(), which is in the cleanup path of 
iommu_bus_init(). AFAICS any links created by .add_device during that 
process would be left dangling, because the device(s) would be live but 
otherwise disassociated from the IOMMU afterwards.


From a maintenance perspective it's easier to have the call in its 
logical place even if it does nothing 99% of the time; that way we 
shouldn't have to keep an eye out for subtle changes in the power 
management code or driver core that might invalidate the device_del() 
reasoning above, and the power management guys shouldn't have to 
comprehend the internals of the IOMMU API to make sense of the 
unbalanced call if they ever want to change their API.


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

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

2018-02-01 Thread Sricharan R 
Hi Robin/Vivek,

On 2/1/2018 2:23 PM, Vivek Gautam wrote:
> Hi,
> 
> 
> On 1/31/2018 6:39 PM, Robin Murphy wrote:
>> On 19/01/18 11:43, 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.
>>
>> Don't we need to balance this with a device_link_del() in .remove_device 
>> (like exynos-iommu does)?
> 
> Right. Will add device_link_del() call. Thanks for pointing out.

 The reason for not adding device_link_del from .remove_device was, the core 
device_del 
 which calls the .remove_device from notifier, calls device_links_purge before 
that.
 That does the same thing as device_link_del. So by the time .remove_device is 
called,
 device_links for that device is already cleaned up. Vivek, you may want to 
check once that
 calling device_link_del from .remove_device has no effect, just to confirm 
once more.

Regards,
 Sricharan
 
> 
> regards
> Vivek
> 
>>
>> Robin.
>>
>>> Signed-off-by: Sricharan R 
>>> ---
>>>   drivers/iommu/arm-smmu.c | 11 +++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 95478bfb182c..33bbcfedb896 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
>>>   struct arm_smmu_device *smmu;
>>>   struct arm_smmu_master_cfg *cfg;
>>>   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>> +    struct device_link *link;
>>>   int i, ret;
>>>     if (using_legacy_binding) {
>>> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
>>>     pm_runtime_put_sync(smmu->dev);
>>>   +    /*
>>> + * Establish the link between smmu and master, so that the
>>> + * smmu gets runtime enabled/disabled as per the master's
>>> + * needs.
>>> + */
>>> +    link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>>> +    if (!link)
>>> +    dev_warn(smmu->dev, "Unable to create device link between %s and 
>>> %s\n",
>>> + dev_name(smmu->dev), dev_name(dev));
>>> +
>>>   return 0;
>>>     out_cfg_free:
>>>
> 

-- 
"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 v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

2018-02-01 Thread Vivek Gautam 

Hi,


On 1/31/2018 6:39 PM, Robin Murphy wrote:

On 19/01/18 11:43, 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.


Don't we need to balance this with a device_link_del() in 
.remove_device (like exynos-iommu does)?


Right. Will add device_link_del() call. Thanks for pointing out.

regards
Vivek



Robin.


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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 95478bfb182c..33bbcfedb896 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
  struct arm_smmu_device *smmu;
  struct arm_smmu_master_cfg *cfg;
  struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+    struct device_link *link;
  int i, ret;
    if (using_legacy_binding) {
@@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device 
*dev)

    pm_runtime_put_sync(smmu->dev);
  +    /*
+ * Establish the link between smmu and master, so that the
+ * smmu gets runtime enabled/disabled as per the master's
+ * needs.
+ */
+    link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
+    if (!link)
+    dev_warn(smmu->dev, "Unable to create device link between %s 
and %s\n",

+ dev_name(smmu->dev), dev_name(dev));
+
  return 0;
    out_cfg_free:



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

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

2018-01-31 Thread Robin Murphy 

On 19/01/18 11:43, 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.


Don't we need to balance this with a device_link_del() in .remove_device 
(like exynos-iommu does)?


Robin.


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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 95478bfb182c..33bbcfedb896 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
struct arm_smmu_device *smmu;
struct arm_smmu_master_cfg *cfg;
struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+   struct device_link *link;
int i, ret;
  
  	if (using_legacy_binding) {

@@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
  
  	pm_runtime_put_sync(smmu->dev);
  
+	/*

+* Establish the link between smmu and master, so that the
+* smmu gets runtime enabled/disabled as per the master's
+* needs.
+*/
+   link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
+   if (!link)
+   dev_warn(smmu->dev, "Unable to create device link between %s and 
%s\n",
+dev_name(smmu->dev), dev_name(dev));
+
return 0;
  
  out_cfg_free:



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

[PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

2018-01-19 Thread Vivek Gautam 
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 
---
 drivers/iommu/arm-smmu.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 95478bfb182c..33bbcfedb896 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
struct arm_smmu_device *smmu;
struct arm_smmu_master_cfg *cfg;
struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+   struct device_link *link;
int i, ret;
 
if (using_legacy_binding) {
@@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
 
pm_runtime_put_sync(smmu->dev);
 
+   /*
+* Establish the link between smmu and master, so that the
+* smmu gets runtime enabled/disabled as per the master's
+* needs.
+*/
+   link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
+   if (!link)
+   dev_warn(smmu->dev, "Unable to create device link between %s 
and %s\n",
+dev_name(smmu->dev), dev_name(dev));
+
return 0;
 
 out_cfg_free:
-- 
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