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

2018-03-15 Thread Robin Murphy 

On 15/03/18 08:57, Vivek Gautam wrote:

Hi Robin,


On Wed, Mar 14, 2018 at 11:20 PM, Robin Murphy  wrote:

On 13/03/18 08:55, 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 
Reviewed-by: Tomasz Figa 
---
   drivers/iommu/arm-smmu.c | 29 +
   1 file changed, 29 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 56a04ae80bf3..64953ff2281f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
 iommu_device_link(>iommu, dev);
   + if (pm_runtime_enabled(smmu->dev)) {
+   struct device_link *link;
+
+   /*
+* 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) {



FWIW, given that we don't really care about link itself, I'd be quite happy
to simplify that lot down to:

 if (pm_runtime_enabled(smmu_dev) &&
 !device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME)) {


Sure, will update this.




+   dev_warn(smmu->dev,
+"Unable to add link to the consumer
%s\n",
+dev_name(dev));



(side note: since device_link_add() already prints a message on success,
maybe it could print its own failure message too?)


Should we make device_link that verbose - to print failure messages at
each step (there are atleast a couple where we return link as NULL),
or we can let the users handle printing the message?


I didn't mean to imply anything more than the idea below (although the 
whole function could of course be refactored further to report explicit 
error values). It just seems a bit unbalanced for the core code to be 
noisy about success yet silent about failure, but ultimately that's an 
entirely separate issue which doesn't have to have any bearing on this 
series.


Robin.

->8-
diff --git a/drivers/base/core.c b/drivers/base/core.c
index b2261f92f2f1..895da95f5cb9 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -275,6 +275,9 @@ struct device_link *device_link_add(struct device 
*consumer,

  out:
device_pm_unlock();
device_links_write_unlock();
+   if (!link)
+		dev_warn(consumer, "Failed to link to supplier %s\n", 
dev_name(supplier));

+
return link;
 }
 EXPORT_SYMBOL_GPL(device_link_add);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2018-03-15 Thread Robin Murphy 

On 15/03/18 06:18, Tomasz Figa wrote:

On Thu, Mar 15, 2018 at 2:50 AM, Robin Murphy  wrote:

On 13/03/18 08:55, 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 
Reviewed-by: Tomasz Figa 
---
   drivers/iommu/arm-smmu.c | 29 +
   1 file changed, 29 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 56a04ae80bf3..64953ff2281f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
 iommu_device_link(>iommu, dev);
   + if (pm_runtime_enabled(smmu->dev)) {
+   struct device_link *link;
+
+   /*
+* 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) {



FWIW, given that we don't really care about link itself, I'd be quite happy
to simplify that lot down to:

 if (pm_runtime_enabled(smmu_dev) &&
 !device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME)) {


+   dev_warn(smmu->dev,
+"Unable to add link to the consumer
%s\n",
+dev_name(dev));



(side note: since device_link_add() already prints a message on success,
maybe it could print its own failure message too?)


I think we care whether adding the link succeeded. If it fails to be
added, we might end up with a complete system lockup on a system with
power domains.


Well, yeah, that was implicit - the point is that we *only* care about 
whether it succeeded or not. Thus we may as well just check for NULL 
directly instead of assigning the value as if we were actually going to 
do anything with it.


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

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

2018-03-15 Thread Vivek Gautam 
Hi Robin,


On Wed, Mar 14, 2018 at 11:20 PM, Robin Murphy  wrote:
> On 13/03/18 08:55, 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 
>> Reviewed-by: Tomasz Figa 
>> ---
>>   drivers/iommu/arm-smmu.c | 29 +
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 56a04ae80bf3..64953ff2281f 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
>> iommu_device_link(>iommu, dev);
>>   + if (pm_runtime_enabled(smmu->dev)) {
>> +   struct device_link *link;
>> +
>> +   /*
>> +* 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) {
>
>
> FWIW, given that we don't really care about link itself, I'd be quite happy
> to simplify that lot down to:
>
> if (pm_runtime_enabled(smmu_dev) &&
> !device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME)) {

Sure, will update this.

>
>> +   dev_warn(smmu->dev,
>> +"Unable to add link to the consumer
>> %s\n",
>> +dev_name(dev));
>
>
> (side note: since device_link_add() already prints a message on success,
> maybe it could print its own failure message too?)

Should we make device_link that verbose - to print failure messages at
each step (there are atleast a couple where we return link as NULL),
or we can let the users handle printing the message?

regards
Vivek

>
> Robin.
>
>
>> +   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:
>> @@ -1486,6 +1507,14 @@ static void arm_smmu_remove_device(struct device
>> *dev)
>> cfg  = fwspec->iommu_priv;
>> smmu = cfg->smmu;
>>   + if (pm_runtime_enabled(smmu->dev)) {
>> +   struct device_link *link;
>> +
>> +   link = device_link_find(dev, smmu->dev);
>> +   if (link)
>> +   device_link_del(link);
>> +   }
>> +
>> 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 v9 4/5] iommu/arm-smmu: Add the device_link between masters and smmu

2018-03-15 Thread Tomasz Figa 
On Thu, Mar 15, 2018 at 2:50 AM, Robin Murphy  wrote:
> On 13/03/18 08:55, 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 
>> Reviewed-by: Tomasz Figa 
>> ---
>>   drivers/iommu/arm-smmu.c | 29 +
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 56a04ae80bf3..64953ff2281f 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
>> iommu_device_link(>iommu, dev);
>>   + if (pm_runtime_enabled(smmu->dev)) {
>> +   struct device_link *link;
>> +
>> +   /*
>> +* 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) {
>
>
> FWIW, given that we don't really care about link itself, I'd be quite happy
> to simplify that lot down to:
>
> if (pm_runtime_enabled(smmu_dev) &&
> !device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME)) {
>
>> +   dev_warn(smmu->dev,
>> +"Unable to add link to the consumer
>> %s\n",
>> +dev_name(dev));
>
>
> (side note: since device_link_add() already prints a message on success,
> maybe it could print its own failure message too?)

I think we care whether adding the link succeeded. If it fails to be
added, we might end up with a complete system lockup on a system with
power domains.

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

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

2018-03-14 Thread Robin Murphy 

On 13/03/18 08:55, 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 
Reviewed-by: Tomasz Figa 
---
  drivers/iommu/arm-smmu.c | 29 +
  1 file changed, 29 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 56a04ae80bf3..64953ff2281f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
  
  	iommu_device_link(>iommu, dev);
  
+	if (pm_runtime_enabled(smmu->dev)) {

+   struct device_link *link;
+
+   /*
+* 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) {


FWIW, given that we don't really care about link itself, I'd be quite 
happy to simplify that lot down to:


if (pm_runtime_enabled(smmu_dev) &&
!device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME)) {


+   dev_warn(smmu->dev,
+"Unable to add link to the consumer %s\n",
+dev_name(dev));


(side note: since device_link_add() already prints a message on success, 
maybe it could print its own failure message too?)


Robin.


+   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:
@@ -1486,6 +1507,14 @@ static void arm_smmu_remove_device(struct device *dev)
cfg  = fwspec->iommu_priv;
smmu = cfg->smmu;
  
+	if (pm_runtime_enabled(smmu->dev)) {

+   struct device_link *link;
+
+   link = device_link_find(dev, smmu->dev);
+   if (link)
+   device_link_del(link);
+   }
+
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

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

2018-03-13 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 
Signed-off-by: Vivek Gautam 
Reviewed-by: Tomasz Figa 
---
 drivers/iommu/arm-smmu.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 56a04ae80bf3..64953ff2281f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
 
iommu_device_link(>iommu, dev);
 
+   if (pm_runtime_enabled(smmu->dev)) {
+   struct device_link *link;
+
+   /*
+* 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 add link to the consumer %s\n",
+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:
@@ -1486,6 +1507,14 @@ static void arm_smmu_remove_device(struct device *dev)
cfg  = fwspec->iommu_priv;
smmu = cfg->smmu;
 
+   if (pm_runtime_enabled(smmu->dev)) {
+   struct device_link *link;
+
+   link = device_link_find(dev, smmu->dev);
+   if (link)
+   device_link_del(link);
+   }
+
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