[PATCH 0/5] firmware: arm_scpi: series with smaller improvements

[PATCH 0/5] firmware: arm_scpi: series with smaller improvements

[Heiner Kallweit]

This series includes smaller improvements to make all memory allocations
device-managed and eventually get rid of scpi_remove. This also improves
the cleanup path in scpi_probe.

Heiner Kallweit (5):
  firmware: arm_scpi: remove usage of drvdata and don't reset scpi_info to null
  firmware: arm_scpi: remove two unneeded devm_kfree's in scpi_remove
  firmware: arm_scpi: pre-populate dvfs info in scpi_probe
  firmware: arm_scpi: make freeing mbox channels device-managed
  firmware: arm_scpi: remove scpi_remove

 drivers/firmware/arm_scpi.c | 120 +++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 69 deletions(-)

-- 
2.14.2

[PATCH 1/5] firmware: arm_scpi: remove usage of drvdata and don't reset scpi_info to null

[Heiner Kallweit]

I do not see a benefit in using drvdata as variable scpi_info is
available everywhere anyway.

And setting scpi_info to NULL in scpi_remove isn't needed IMO.
If arm_scpi is built-in, then this code is never used. And if arm_scpi
is built as a module and some other module calls get_scpi_ops() then
due to this dependency scpi_remove is called only after the other
module has been removed. Last but not least users usually store the
result of get_scpi_ops(), therefore setting scpi_info to NULL wouldn't
really help.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/firmware/arm_scpi.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index c5ce096b..c942761e 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -866,8 +866,6 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
 static ssize_t protocol_version_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev);
-
 	return sprintf(buf, "%d.%d\n",
 		       PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
 		       PROTOCOL_REV_MINOR(scpi_info->protocol_version));
@@ -877,8 +875,6 @@ static DEVICE_ATTR_RO(protocol_version);
 static ssize_t firmware_version_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev);
-
 	return sprintf(buf, "%d.%d.%d\n",
 		       FW_REV_MAJOR(scpi_info->firmware_version),
 		       FW_REV_MINOR(scpi_info->firmware_version),
@@ -909,21 +905,17 @@ static int scpi_remove(struct platform_device *pdev)
 {
 	int i;
 	struct device *dev = &pdev->dev;
-	struct scpi_drvinfo *info = platform_get_drvdata(pdev);
-
-	scpi_info = NULL; /* stop exporting SCPI ops through get_scpi_ops */
 
 	of_platform_depopulate(dev);
 	sysfs_remove_groups(&dev->kobj, versions_groups);
-	scpi_free_channels(dev, info->channels, info->num_chans);
-	platform_set_drvdata(pdev, NULL);
+	scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans);
 
-	for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
-		kfree(info->dvfs[i]->opps);
-		kfree(info->dvfs[i]);
+	for (i = 0; i < MAX_DVFS_DOMAINS && scpi_info->dvfs[i]; i++) {
+		kfree(scpi_info->dvfs[i]->opps);
+		kfree(scpi_info->dvfs[i]);
 	}
-	devm_kfree(dev, info->channels);
-	devm_kfree(dev, info);
+	devm_kfree(dev, scpi_info->channels);
+	devm_kfree(dev, scpi_info);
 
 	return 0;
 }
@@ -1031,8 +1023,6 @@ static int scpi_probe(struct platform_device *pdev)
 	scpi_info->num_chans = count;
 	scpi_info->commands = scpi_std_commands;
 
-	platform_set_drvdata(pdev, scpi_info);
-
 	if (scpi_info->is_legacy) {
 		/* Replace with legacy variants */
 		scpi_ops.clk_set_val = legacy_scpi_clk_set_val;
-- 
2.14.2

[PATCH 1/5] firmware: arm_scpi: remove usage of drvdata and don't reset scpi_info to null

[Sudeep Holla]

On 29/09/17 22:43, Heiner Kallweit wrote:
> I do not see a benefit in using drvdata as variable scpi_info is
> available everywhere anyway.
> 

I will reword above before committing as:
"There's no benefit using drvdata as variable scpi_info is global"

> And setting scpi_info to NULL in scpi_remove isn't needed IMO.
> If arm_scpi is built-in, then this code is never used. And if arm_scpi
> is built as a module and some other module calls get_scpi_ops() then
> due to this dependency scpi_remove is called only after the other
> module has been removed. Last but not least users usually store the
> result of get_scpi_ops(), therefore setting scpi_info to NULL wouldn't
> really help.
> 

Agreed. I am fine with the change. Only issue I see is to support
multiple instances of SCPI on a platform, but that may need more rework
anyways.

-- 
Regards,
Sudeep

[PATCH 2/5] firmware: arm_scpi: remove two unneeded devm_kfree's in scpi_remove

[Heiner Kallweit]

Both memory areas are free'd anyway when the device is destroyed,
so we don't have to do it manually.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/firmware/arm_scpi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index c942761e..d80d11ae 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -914,8 +914,6 @@ static int scpi_remove(struct platform_device *pdev)
 		kfree(scpi_info->dvfs[i]->opps);
 		kfree(scpi_info->dvfs[i]);
 	}
-	devm_kfree(dev, scpi_info->channels);
-	devm_kfree(dev, scpi_info);
 
 	return 0;
 }
-- 
2.14.2

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Heiner Kallweit]

Pre-populating the dvfs info data in scpi_probe allows to make all
memory allocations device-managed. This helps to simplify scpi_remove
and eventually to get rid of scpi_remove completely.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/firmware/arm_scpi.c | 57 ++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index d80d11ae..fd79adb5 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -644,35 +644,35 @@ static int opp_cmp_func(const void *opp1, const void *opp2)
 }
 
 static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
+{
+	if (domain >= MAX_DVFS_DOMAINS)
+		return ERR_PTR(-EINVAL);
+
+	return scpi_info->dvfs[domain] ?: ERR_PTR(-EINVAL);
+}
+
+static int scpi_dvfs_populate_info(struct device *dev, u8 domain)
 {
 	struct scpi_dvfs_info *info;
 	struct scpi_opp *opp;
 	struct dvfs_info buf;
 	int ret, i;
 
-	if (domain >= MAX_DVFS_DOMAINS)
-		return ERR_PTR(-EINVAL);
-
-	if (scpi_info->dvfs[domain])	/* data already populated */
-		return scpi_info->dvfs[domain];
-
 	ret = scpi_send_message(CMD_GET_DVFS_INFO, &domain, sizeof(domain),
 				&buf, sizeof(buf));
 	if (ret)
-		return ERR_PTR(ret);
+		return ret;
 
-	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	info = devm_kmalloc(dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	info->count = DVFS_OPP_COUNT(buf.header);
 	info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
 
-	info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
-	if (!info->opps) {
-		kfree(info);
-		return ERR_PTR(-ENOMEM);
-	}
+	info->opps = devm_kcalloc(dev, info->count, sizeof(*opp), GFP_KERNEL);
+	if (!info->opps)
+		return -ENOMEM;
 
 	for (i = 0, opp = info->opps; i < info->count; i++, opp++) {
 		opp->freq = le32_to_cpu(buf.opps[i].freq);
@@ -682,7 +682,20 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 	sort(info->opps, info->count, sizeof(*opp), opp_cmp_func, NULL);
 
 	scpi_info->dvfs[domain] = info;
-	return info;
+	return 0;
+}
+
+static int scpi_dvfs_populate(struct device *dev)
+{
+	int ret, i;
+
+	for (i = 0; i < MAX_DVFS_DOMAINS; i++) {
+		ret = scpi_dvfs_populate_info(dev, i);
+		if (ret)
+			break;
+	}
+
+	return i > 0 ? 0 : ret;
 }
 
 static int scpi_dev_domain_id(struct device *dev)
@@ -903,18 +916,12 @@ scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count)
 
 static int scpi_remove(struct platform_device *pdev)
 {
-	int i;
 	struct device *dev = &pdev->dev;
 
 	of_platform_depopulate(dev);
 	sysfs_remove_groups(&dev->kobj, versions_groups);
 	scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans);
 
-	for (i = 0; i < MAX_DVFS_DOMAINS && scpi_info->dvfs[i]; i++) {
-		kfree(scpi_info->dvfs[i]->opps);
-		kfree(scpi_info->dvfs[i]);
-	}
-
 	return 0;
 }
 
@@ -1020,6 +1027,7 @@ static int scpi_probe(struct platform_device *pdev)
 	scpi_info->channels = scpi_chan;
 	scpi_info->num_chans = count;
 	scpi_info->commands = scpi_std_commands;
+	scpi_info->scpi_ops = &scpi_ops;
 
 	if (scpi_info->is_legacy) {
 		/* Replace with legacy variants */
@@ -1039,13 +1047,18 @@ static int scpi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = scpi_dvfs_populate(dev);
+	if (ret) {
+		scpi_remove(pdev);
+		return ret;
+	}
+
 	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
 		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
 		  PROTOCOL_REV_MINOR(scpi_info->protocol_version),
 		  FW_REV_MAJOR(scpi_info->firmware_version),
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
-	scpi_info->scpi_ops = &scpi_ops;
 
 	ret = sysfs_create_groups(&dev->kobj, versions_groups);
 	if (ret)
-- 
2.14.2

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Sudeep Holla]

On 29/09/17 22:44, Heiner Kallweit wrote:
> Pre-populating the dvfs info data in scpi_probe allows to make all
> memory allocations device-managed. This helps to simplify scpi_remove
> and eventually to get rid of scpi_remove completely.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/firmware/arm_scpi.c | 57 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index d80d11ae..fd79adb5 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -644,35 +644,35 @@ static int opp_cmp_func(const void *opp1, const void *opp2)
>  }
>  
>  static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
> +{
> +	if (domain >= MAX_DVFS_DOMAINS)
> +		return ERR_PTR(-EINVAL);
> +
> +	return scpi_info->dvfs[domain] ?: ERR_PTR(-EINVAL);
> +}
> +
> +static int scpi_dvfs_populate_info(struct device *dev, u8 domain)
>  {
>  	struct scpi_dvfs_info *info;
>  	struct scpi_opp *opp;
>  	struct dvfs_info buf;
>  	int ret, i;
>  
> -	if (domain >= MAX_DVFS_DOMAINS)
> -		return ERR_PTR(-EINVAL);
> -
> -	if (scpi_info->dvfs[domain])	/* data already populated */
> -		return scpi_info->dvfs[domain];
> -
>  	ret = scpi_send_message(CMD_GET_DVFS_INFO, &domain, sizeof(domain),
>  				&buf, sizeof(buf));
>  	if (ret)
> -		return ERR_PTR(ret);
> +		return ret;
>  
> -	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	info = devm_kmalloc(dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>  
>  	info->count = DVFS_OPP_COUNT(buf.header);
>  	info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
>  
> -	info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
> -	if (!info->opps) {
> -		kfree(info);
> -		return ERR_PTR(-ENOMEM);
> -	}
> +	info->opps = devm_kcalloc(dev, info->count, sizeof(*opp), GFP_KERNEL);
> +	if (!info->opps)
> +		return -ENOMEM;
>  
>  	for (i = 0, opp = info->opps; i < info->count; i++, opp++) {
>  		opp->freq = le32_to_cpu(buf.opps[i].freq);
> @@ -682,7 +682,20 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>  	sort(info->opps, info->count, sizeof(*opp), opp_cmp_func, NULL);
>  
>  	scpi_info->dvfs[domain] = info;
> -	return info;
> +	return 0;
> +}
> +
> +static int scpi_dvfs_populate(struct device *dev)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < MAX_DVFS_DOMAINS; i++) {
> +		ret = scpi_dvfs_populate_info(dev, i);
> +		if (ret)
> +			break;

Does it make sense to continue to complete all MAX_DVFS_DOMAINS ?
Just wondering if there will be any firmware that returns errors for
few domains(e.g. not supported in initial versions of firmware).
I don't want to stop probing due to that. Let me know what you think.

-- 
Regards,
Sudeep

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Heiner Kallweit]

Am 02.10.2017 um 13:17 schrieb Sudeep Holla:
> 
> 
> On 29/09/17 22:44, Heiner Kallweit wrote:
>> Pre-populating the dvfs info data in scpi_probe allows to make all
>> memory allocations device-managed. This helps to simplify scpi_remove
>> and eventually to get rid of scpi_remove completely.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 57 ++++++++++++++++++++++++++++-----------------
>>  1 file changed, 35 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index d80d11ae..fd79adb5 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -644,35 +644,35 @@ static int opp_cmp_func(const void *opp1, const void *opp2)
>>  }
>>  
>>  static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>> +{
>> +	if (domain >= MAX_DVFS_DOMAINS)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return scpi_info->dvfs[domain] ?: ERR_PTR(-EINVAL);
>> +}
>> +
>> +static int scpi_dvfs_populate_info(struct device *dev, u8 domain)
>>  {
>>  	struct scpi_dvfs_info *info;
>>  	struct scpi_opp *opp;
>>  	struct dvfs_info buf;
>>  	int ret, i;
>>  
>> -	if (domain >= MAX_DVFS_DOMAINS)
>> -		return ERR_PTR(-EINVAL);
>> -
>> -	if (scpi_info->dvfs[domain])	/* data already populated */
>> -		return scpi_info->dvfs[domain];
>> -
>>  	ret = scpi_send_message(CMD_GET_DVFS_INFO, &domain, sizeof(domain),
>>  				&buf, sizeof(buf));
>>  	if (ret)
>> -		return ERR_PTR(ret);
>> +		return ret;
>>  
>> -	info = kmalloc(sizeof(*info), GFP_KERNEL);
>> +	info = devm_kmalloc(dev, sizeof(*info), GFP_KERNEL);
>>  	if (!info)
>> -		return ERR_PTR(-ENOMEM);
>> +		return -ENOMEM;
>>  
>>  	info->count = DVFS_OPP_COUNT(buf.header);
>>  	info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
>>  
>> -	info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
>> -	if (!info->opps) {
>> -		kfree(info);
>> -		return ERR_PTR(-ENOMEM);
>> -	}
>> +	info->opps = devm_kcalloc(dev, info->count, sizeof(*opp), GFP_KERNEL);
>> +	if (!info->opps)
>> +		return -ENOMEM;
>>  
>>  	for (i = 0, opp = info->opps; i < info->count; i++, opp++) {
>>  		opp->freq = le32_to_cpu(buf.opps[i].freq);
>> @@ -682,7 +682,20 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>>  	sort(info->opps, info->count, sizeof(*opp), opp_cmp_func, NULL);
>>  
>>  	scpi_info->dvfs[domain] = info;
>> -	return info;
>> +	return 0;
>> +}
>> +
>> +static int scpi_dvfs_populate(struct device *dev)
>> +{
>> +	int ret, i;
>> +
>> +	for (i = 0; i < MAX_DVFS_DOMAINS; i++) {
>> +		ret = scpi_dvfs_populate_info(dev, i);
>> +		if (ret)
>> +			break;
> 
> Does it make sense to continue to complete all MAX_DVFS_DOMAINS ?
> Just wondering if there will be any firmware that returns errors for
> few domains(e.g. not supported in initial versions of firmware).
> I don't want to stop probing due to that. Let me know what you think.
> 
The (legacy) SCPI firmware on my system seems to ignore the domain
in CMD_GET_DVFS_INFO. It returns the same dvfs info independent of
the domain parameter. So indeed prepopulating may not be the best idea.

Still we need to do something in the remove callback to deal with the
scenario you describe (error for few domains).

Remove does
for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
and therefore stops at the first unpopulated domain and doesn't free
the memory for further populated domains. I'll provide a patch for it.

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Sudeep Holla]

On 02/10/17 23:07, Heiner Kallweit wrote:
> Am 02.10.2017 um 13:17 schrieb Sudeep Holla:
>> 
>> 
>> On 29/09/17 22:44, Heiner Kallweit wrote:
>>> Pre-populating the dvfs info data in scpi_probe allows to make 
>>> all memory allocations device-managed. This helps to simplify 
>>> scpi_remove and eventually to get rid of scpi_remove completely.
>>> 
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

[...]

>> Does it make sense to continue to complete all MAX_DVFS_DOMAINS ? 
>> Just wondering if there will be any firmware that returns errors 
>> for few domains(e.g. not supported in initial versions of 
>> firmware). I don't want to stop probing due to that. Let me know 
>> what you think.
>> 
> The (legacy) SCPI firmware on my system seems to ignore the domain
> in CMD_GET_DVFS_INFO. It returns the same dvfs info independent of
> the domain parameter. So indeed prepopulating may not be the best
> idea.
> 

I can't follow that. Does the behavior change probe time vs runtime ?
I am fine with probe time populate, just that we can't stop or propagate
any error as it fails to probe other dependent drivers which may work
fine without DVFS(e.g. clocks, sensors and power domains)

> Still we need to do something in the remove callback to deal with the
> scenario you describe (error for few domains).

devm_* APIs will take care of freeing DVFS domain info, so what else
needs to be done ? I just noticed devm_kfree(NULL) can produce WARN_ON
splat, is that what you are referring ?

> 
> Remove does for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
> and therefore stops at the first unpopulated domain and doesn't free
> the memory for further populated domains. I'll provide a patch for
> it.
> 

Does that mean you are re-introducing scpi_remove ? I kind of liked
removing it.

-- 
Regards,
Sudeep

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Heiner Kallweit]

Am 03.10.2017 um 12:57 schrieb Sudeep Holla:
> 
> 
> On 02/10/17 23:07, Heiner Kallweit wrote:
>> Am 02.10.2017 um 13:17 schrieb Sudeep Holla:
>>>
>>>
>>> On 29/09/17 22:44, Heiner Kallweit wrote:
>>>> Pre-populating the dvfs info data in scpi_probe allows to make 
>>>> all memory allocations device-managed. This helps to simplify 
>>>> scpi_remove and eventually to get rid of scpi_remove completely.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> [...]
> 
>>> Does it make sense to continue to complete all MAX_DVFS_DOMAINS ? 
>>> Just wondering if there will be any firmware that returns errors 
>>> for few domains(e.g. not supported in initial versions of 
>>> firmware). I don't want to stop probing due to that. Let me know 
>>> what you think.
>>>
>> The (legacy) SCPI firmware on my system seems to ignore the domain
>> in CMD_GET_DVFS_INFO. It returns the same dvfs info independent of
>> the domain parameter. So indeed prepopulating may not be the best
>> idea.
>>
> 
> I can't follow that. Does the behavior change probe time vs runtime ?
> I am fine with probe time populate, just that we can't stop or propagate
> any error as it fails to probe other dependent drivers which may work
> fine without DVFS(e.g. clocks, sensors and power domains)
> 
>> Still we need to do something in the remove callback to deal with the
>> scenario you describe (error for few domains).
> 
> devm_* APIs will take care of freeing DVFS domain info, so what else
> needs to be done ? I just noticed devm_kfree(NULL) can produce WARN_ON
> splat, is that what you are referring ?
> 
>>
>> Remove does for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
>> and therefore stops at the first unpopulated domain and doesn't free
>> the memory for further populated domains. I'll provide a patch for
>> it.
>>
> 
> Does that mean you are re-introducing scpi_remove ? I kind of liked
> removing it.
> 
Sorry for the confusion. Then I'll go with the original approach and
just make sure that errors whilst populating dvfs info are ignored.

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Sudeep Holla]

On 03/10/17 17:00, Heiner Kallweit wrote:
> Am 03.10.2017 um 12:57 schrieb Sudeep Holla:
>>
>>
>> On 02/10/17 23:07, Heiner Kallweit wrote:
>>> Am 02.10.2017 um 13:17 schrieb Sudeep Holla:
>>>>
>>>>
>>>> On 29/09/17 22:44, Heiner Kallweit wrote:
>>>>> Pre-populating the dvfs info data in scpi_probe allows to make 
>>>>> all memory allocations device-managed. This helps to simplify 
>>>>> scpi_remove and eventually to get rid of scpi_remove completely.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>
>> [...]
>>
>>>> Does it make sense to continue to complete all MAX_DVFS_DOMAINS ? 
>>>> Just wondering if there will be any firmware that returns errors 
>>>> for few domains(e.g. not supported in initial versions of 
>>>> firmware). I don't want to stop probing due to that. Let me know 
>>>> what you think.
>>>>
>>> The (legacy) SCPI firmware on my system seems to ignore the domain
>>> in CMD_GET_DVFS_INFO. It returns the same dvfs info independent of
>>> the domain parameter. So indeed prepopulating may not be the best
>>> idea.
>>>
>>
>> I can't follow that. Does the behavior change probe time vs runtime ?
>> I am fine with probe time populate, just that we can't stop or propagate
>> any error as it fails to probe other dependent drivers which may work
>> fine without DVFS(e.g. clocks, sensors and power domains)
>>
>>> Still we need to do something in the remove callback to deal with the
>>> scenario you describe (error for few domains).
>>
>> devm_* APIs will take care of freeing DVFS domain info, so what else
>> needs to be done ? I just noticed devm_kfree(NULL) can produce WARN_ON
>> splat, is that what you are referring ?
>>
>>>
>>> Remove does for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
>>> and therefore stops at the first unpopulated domain and doesn't free
>>> the memory for further populated domains. I'll provide a patch for
>>> it.
>>>
>>
>> Does that mean you are re-introducing scpi_remove ? I kind of liked
>> removing it.
>>
> Sorry for the confusion. Then I'll go with the original approach and
> just make sure that errors whilst populating dvfs info are ignored.
> 

Indeed.

If you are fine with the below fixup, then I can add it myself. Let me know.

Regards,
Sudeep

--
drivers/firmware/arm_scpi.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index b0d2d21e6805..f713a64c1052 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -685,17 +685,12 @@ static int scpi_dvfs_populate_info(struct device
*dev, u8 domain)
 	return 0;
 }

-static int scpi_dvfs_populate(struct device *dev)
+static void scpi_dvfs_populate(struct device *dev)
 {
-	int ret, i;
-
-	for (i = 0; i < MAX_DVFS_DOMAINS; i++) {
-		ret = scpi_dvfs_populate_info(dev, i);
-		if (ret)
-			break;
-	}
+	int domain;

-	return i > 0 ? 0 : ret;
+	for (domain = 0; domain < MAX_DVFS_DOMAINS; domain++)
+		scpi_dvfs_populate_info(dev, domain);
 }

 static int scpi_dev_domain_id(struct device *dev)
@@ -1027,9 +1022,7 @@ static int scpi_probe(struct platform_device *pdev)
 		return ret;
 	}

-	ret = scpi_dvfs_populate(dev);
-	if (ret)
-		return ret;
+	scpi_dvfs_populate(dev);

 	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
 		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Heiner Kallweit]

Am 03.10.2017 um 18:18 schrieb Sudeep Holla:
> 
> 
> On 03/10/17 17:00, Heiner Kallweit wrote:
>> Am 03.10.2017 um 12:57 schrieb Sudeep Holla:
>>>
>>>
>>> On 02/10/17 23:07, Heiner Kallweit wrote:
>>>> Am 02.10.2017 um 13:17 schrieb Sudeep Holla:
>>>>>
>>>>>
>>>>> On 29/09/17 22:44, Heiner Kallweit wrote:
>>>>>> Pre-populating the dvfs info data in scpi_probe allows to make 
>>>>>> all memory allocations device-managed. This helps to simplify 
>>>>>> scpi_remove and eventually to get rid of scpi_remove completely.
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> [...]
>>>
>>>>> Does it make sense to continue to complete all MAX_DVFS_DOMAINS ? 
>>>>> Just wondering if there will be any firmware that returns errors 
>>>>> for few domains(e.g. not supported in initial versions of 
>>>>> firmware). I don't want to stop probing due to that. Let me know 
>>>>> what you think.
>>>>>
>>>> The (legacy) SCPI firmware on my system seems to ignore the domain
>>>> in CMD_GET_DVFS_INFO. It returns the same dvfs info independent of
>>>> the domain parameter. So indeed prepopulating may not be the best
>>>> idea.
>>>>
>>>
>>> I can't follow that. Does the behavior change probe time vs runtime ?
>>> I am fine with probe time populate, just that we can't stop or propagate
>>> any error as it fails to probe other dependent drivers which may work
>>> fine without DVFS(e.g. clocks, sensors and power domains)
>>>
>>>> Still we need to do something in the remove callback to deal with the
>>>> scenario you describe (error for few domains).
>>>
>>> devm_* APIs will take care of freeing DVFS domain info, so what else
>>> needs to be done ? I just noticed devm_kfree(NULL) can produce WARN_ON
>>> splat, is that what you are referring ?
>>>
>>>>
>>>> Remove does for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
>>>> and therefore stops at the first unpopulated domain and doesn't free
>>>> the memory for further populated domains. I'll provide a patch for
>>>> it.
>>>>
>>>
>>> Does that mean you are re-introducing scpi_remove ? I kind of liked
>>> removing it.
>>>
>> Sorry for the confusion. Then I'll go with the original approach and
>> just make sure that errors whilst populating dvfs info are ignored.
>>
> 
> Indeed.
> 
> If you are fine with the below fixup, then I can add it myself. Let me know.
> 
Fine with me.

Regards, Heiner

> Regards,
> Sudeep
> 
> --
> drivers/firmware/arm_scpi.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index b0d2d21e6805..f713a64c1052 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -685,17 +685,12 @@ static int scpi_dvfs_populate_info(struct device
> *dev, u8 domain)
>  	return 0;
>  }
> 
> -static int scpi_dvfs_populate(struct device *dev)
> +static void scpi_dvfs_populate(struct device *dev)
>  {
> -	int ret, i;
> -
> -	for (i = 0; i < MAX_DVFS_DOMAINS; i++) {
> -		ret = scpi_dvfs_populate_info(dev, i);
> -		if (ret)
> -			break;
> -	}
> +	int domain;
> 
> -	return i > 0 ? 0 : ret;
> +	for (domain = 0; domain < MAX_DVFS_DOMAINS; domain++)
> +		scpi_dvfs_populate_info(dev, domain);
>  }
> 
>  static int scpi_dev_domain_id(struct device *dev)
> @@ -1027,9 +1022,7 @@ static int scpi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
> 
> -	ret = scpi_dvfs_populate(dev);
> -	if (ret)
> -		return ret;
> +	scpi_dvfs_populate(dev);
> 
>  	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>  		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
> 
> 

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Sudeep Holla]

On 03/10/17 19:19, Heiner Kallweit wrote:
> Am 03.10.2017 um 18:18 schrieb Sudeep Holla:
>>
>>
>> On 03/10/17 17:00, Heiner Kallweit wrote:

[...]

>>>>>
>>>>> Remove does for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
>>>>> and therefore stops at the first unpopulated domain and doesn't free
>>>>> the memory for further populated domains. I'll provide a patch for
>>>>> it.
>>>>>
>>>>
>>>> Does that mean you are re-introducing scpi_remove ? I kind of liked
>>>> removing it.
>>>>
>>> Sorry for the confusion. Then I'll go with the original approach and
>>> just make sure that errors whilst populating dvfs info are ignored.
>>>
>>
>> Indeed.
>>
>> If you are fine with the below fixup, then I can add it myself. Let me know.
>>
> Fine with me.

Thanks for confirming, pushed now[1]. Can you please double check if
possible.

-- 
Regards,
Sudeep

[1] https://git.kernel.org/sudeep.holla/linux/h/for-next/scpi-updates

[PATCH 3/5] firmware: arm_scpi: pre-populate dvfs info in scpi_probe

[Heiner Kallweit]

Am 04.10.2017 um 12:10 schrieb Sudeep Holla:
> 
> 
> On 03/10/17 19:19, Heiner Kallweit wrote:
>> Am 03.10.2017 um 18:18 schrieb Sudeep Holla:
>>>
>>>
>>> On 03/10/17 17:00, Heiner Kallweit wrote:
> 
> [...]
> 
>>>>>>
>>>>>> Remove does for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
>>>>>> and therefore stops at the first unpopulated domain and doesn't free
>>>>>> the memory for further populated domains. I'll provide a patch for
>>>>>> it.
>>>>>>
>>>>>
>>>>> Does that mean you are re-introducing scpi_remove ? I kind of liked
>>>>> removing it.
>>>>>
>>>> Sorry for the confusion. Then I'll go with the original approach and
>>>> just make sure that errors whilst populating dvfs info are ignored.
>>>>
>>>
>>> Indeed.
>>>
>>> If you are fine with the below fixup, then I can add it myself. Let me know.
>>>
>> Fine with me.
> 
> Thanks for confirming, pushed now[1]. Can you please double check if
> possible.
> 
Compiles and works fine here.
By the way, I have three more small patches which I think are improvements (YMMV).
Will submit them later today.

[PATCH 4/5] firmware: arm_scpi: make freeing mbox channels device-managed

[Heiner Kallweit]

Make freeing the mbox channels device-managed, thus further simplifying
scpi_remove and and one further step to get rid of scpi_remove.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/firmware/arm_scpi.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index fd79adb5..c91f3241 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -902,16 +902,13 @@ static struct attribute *versions_attrs[] = {
 };
 ATTRIBUTE_GROUPS(versions);
 
-static void
-scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count)
+static void scpi_free_channels(void *data)
 {
+	struct scpi_drvinfo *info = data;
 	int i;
 
-	for (i = 0; i < count && pchan->chan; i++, pchan++) {
-		mbox_free_channel(pchan->chan);
-		devm_kfree(dev, pchan->xfers);
-		devm_iounmap(dev, pchan->rx_payload);
-	}
+	for (i = 0; i < info->num_chans; i++)
+		mbox_free_channel(info->channels[i].chan);
 }
 
 static int scpi_remove(struct platform_device *pdev)
@@ -920,7 +917,6 @@ static int scpi_remove(struct platform_device *pdev)
 
 	of_platform_depopulate(dev);
 	sysfs_remove_groups(&dev->kobj, versions_groups);
-	scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans);
 
 	return 0;
 }
@@ -953,7 +949,6 @@ static int scpi_probe(struct platform_device *pdev)
 {
 	int count, idx, ret;
 	struct resource res;
-	struct scpi_chan *scpi_chan;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 
@@ -970,13 +965,19 @@ static int scpi_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
-	if (!scpi_chan)
+	scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
+					   GFP_KERNEL);
+	if (!scpi_info->channels)
 		return -ENOMEM;
 
-	for (idx = 0; idx < count; idx++) {
+	ret = devm_add_action(dev, scpi_free_channels, scpi_info);
+	if (ret)
+		return ret;
+
+	for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
 		resource_size_t size;
-		struct scpi_chan *pchan = scpi_chan + idx;
+		int idx = scpi_info->num_chans;
+		struct scpi_chan *pchan = scpi_info->channels + idx;
 		struct mbox_client *cl = &pchan->cl;
 		struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
 
@@ -984,15 +985,14 @@ static int scpi_probe(struct platform_device *pdev)
 		of_node_put(shmem);
 		if (ret) {
 			dev_err(dev, "failed to get SCPI payload mem resource\n");
-			goto err;
+			return ret;
 		}
 
 		size = resource_size(&res);
 		pchan->rx_payload = devm_ioremap(dev, res.start, size);
 		if (!pchan->rx_payload) {
 			dev_err(dev, "failed to ioremap SCPI payload\n");
-			ret = -EADDRNOTAVAIL;
-			goto err;
+			return -EADDRNOTAVAIL;
 		}
 		pchan->tx_payload = pchan->rx_payload + (size >> 1);
 
@@ -1018,14 +1018,9 @@ static int scpi_probe(struct platform_device *pdev)
 				dev_err(dev, "failed to get channel%d err %d\n",
 					idx, ret);
 		}
-err:
-		scpi_free_channels(dev, scpi_chan, idx);
-		scpi_info = NULL;
 		return ret;
 	}
 
-	scpi_info->channels = scpi_chan;
-	scpi_info->num_chans = count;
 	scpi_info->commands = scpi_std_commands;
 	scpi_info->scpi_ops = &scpi_ops;
 
-- 
2.14.2

[PATCH 4/5] firmware: arm_scpi: make freeing mbox channels device-managed

[Sudeep Holla]

On 29/09/17 22:44, Heiner Kallweit wrote:
> Make freeing the mbox channels device-managed, thus further simplifying
> scpi_remove and and one further step to get rid of scpi_remove.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/firmware/arm_scpi.c | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index fd79adb5..c91f3241 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -902,16 +902,13 @@ static struct attribute *versions_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(versions);
>  
> -static void
> -scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count)
> +static void scpi_free_channels(void *data)
>  {
> +	struct scpi_drvinfo *info = data;
>  	int i;
>  
> -	for (i = 0; i < count && pchan->chan; i++, pchan++) {
> -		mbox_free_channel(pchan->chan);
> -		devm_kfree(dev, pchan->xfers);
> -		devm_iounmap(dev, pchan->rx_payload);
> -	}
> +	for (i = 0; i < info->num_chans; i++)
> +		mbox_free_channel(info->channels[i].chan);
>  }
>  
>  static int scpi_remove(struct platform_device *pdev)
> @@ -920,7 +917,6 @@ static int scpi_remove(struct platform_device *pdev)
>  
>  	of_platform_depopulate(dev);
>  	sysfs_remove_groups(&dev->kobj, versions_groups);
> -	scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans);
>  
>  	return 0;
>  }
> @@ -953,7 +949,6 @@ static int scpi_probe(struct platform_device *pdev)
>  {
>  	int count, idx, ret;
>  	struct resource res;
> -	struct scpi_chan *scpi_chan;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  
> @@ -970,13 +965,19 @@ static int scpi_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
> -	if (!scpi_chan)
> +	scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
> +					   GFP_KERNEL);
> +	if (!scpi_info->channels)
>  		return -ENOMEM;
>  
> -	for (idx = 0; idx < count; idx++) {
> +	ret = devm_add_action(dev, scpi_free_channels, scpi_info);

Neat, I wasn't aware of such API before.

-- 
-- 
Regards,
Sudeep

[PATCH 5/5] firmware: arm_scpi: remove scpi_remove

[Heiner Kallweit]

sysfs_create_groups and of_platform_populate can be replaced with the
device-managed versions what allows us to remove scpi_remove.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/firmware/arm_scpi.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index c91f3241..a71907e6 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -911,16 +911,6 @@ static void scpi_free_channels(void *data)
 		mbox_free_channel(info->channels[i].chan);
 }
 
-static int scpi_remove(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-
-	of_platform_depopulate(dev);
-	sysfs_remove_groups(&dev->kobj, versions_groups);
-
-	return 0;
-}
-
 #define MAX_SCPI_XFERS		10
 static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
 {
@@ -1038,15 +1028,12 @@ static int scpi_probe(struct platform_device *pdev)
 	ret = scpi_init_versions(scpi_info);
 	if (ret) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
-		scpi_remove(pdev);
 		return ret;
 	}
 
 	ret = scpi_dvfs_populate(dev);
-	if (ret) {
-		scpi_remove(pdev);
+	if (ret)
 		return ret;
-	}
 
 	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
 		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
@@ -1055,11 +1042,11 @@ static int scpi_probe(struct platform_device *pdev)
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
 
-	ret = sysfs_create_groups(&dev->kobj, versions_groups);
+	ret = devm_device_add_groups(dev, versions_groups);
 	if (ret)
 		dev_err(dev, "unable to create sysfs version group\n");
 
-	return of_platform_populate(dev->of_node, NULL, NULL, dev);
+	return devm_of_platform_populate(dev);
 }
 
 static const struct of_device_id scpi_of_match[] = {
@@ -1076,7 +1063,6 @@ static struct platform_driver scpi_driver = {
 		.of_match_table = scpi_of_match,
 	},
 	.probe = scpi_probe,
-	.remove = scpi_remove,
 };
 module_platform_driver(scpi_driver);
 
-- 
2.14.2

[PATCH 0/5] firmware: arm_scpi: series with smaller improvements

[Sudeep Holla]

On 29/09/17 22:25, Heiner Kallweit wrote:
> This series includes smaller improvements to make all memory allocations
> device-managed and eventually get rid of scpi_remove. This also improves
> the cleanup path in scpi_probe.
> 
Overall looks good, just one question on DVFS in patch 3. I vaguely
remember the need to support that in past.

-- 
Regards,
Sudeep

[PATCH 0/5] firmware: arm_scpi: series with smaller improvements

[Heiner Kallweit]

Am 02.10.2017 um 13:25 schrieb Sudeep Holla:
> 
> 
> On 29/09/17 22:25, Heiner Kallweit wrote:
>> This series includes smaller improvements to make all memory allocations
>> device-managed and eventually get rid of scpi_remove. This also improves
>> the cleanup path in scpi_probe.
>>
> Overall looks good, just one question on DVFS in patch 3. I vaguely
> remember the need to support that in past.
> 
I will remove the prepopulating in patch 3 and just submit the fix
described in my reply to your review comment.
Do you want me to re-submit the full patch series or just patches 3-5?

[PATCH 0/5] firmware: arm_scpi: series with smaller improvements

[Sudeep Holla]

On 02/10/17 23:10, Heiner Kallweit wrote:
> Am 02.10.2017 um 13:25 schrieb Sudeep Holla:
>>
>>
>> On 29/09/17 22:25, Heiner Kallweit wrote:
>>> This series includes smaller improvements to make all memory allocations
>>> device-managed and eventually get rid of scpi_remove. This also improves
>>> the cleanup path in scpi_probe.
>>>
>> Overall looks good, just one question on DVFS in patch 3. I vaguely
>> remember the need to support that in past.
>>
> I will remove the prepopulating in patch 3 and just submit the fix
> described in my reply to your review comment.
> Do you want me to re-submit the full patch series or just patches 3-5?
> 

It's up to you, am fine either way.

-- 
Regards,
Sudeep