[PATCH] remoteproc: uclass: Modify uc_pdata->name to use combination of device name and device's parent name

[MD Danish Anwar]

On 05/08/24 12:08 pm, Roger Quadros wrote:
> 
> 
> On 19/07/2024 11:59, MD Danish Anwar wrote:
>> uc_pdata->name is populated from device tree property "remoteproc-name".
>> For those devcices that don't set "remoteproc-name", uc_pdata->name
>> falls back to dev->name.
>>
>> If two devices have same name, this will result into uc_pdata->name not
>> being unique and rproc_init() will fail.
>>
>> Fix this by using combination of dev->name and dev->parent->name instead
>> of using just the dev->name to populate uc_pdata->name.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar at ti.com>
>> ---
>> Cc: Andrew Davis <afd at ti.com>
>>
>> Failure Example:
>> In k3-am64-main.dtsi, both pru0_0 [1] and pru1_0 [2] will have
>> dev->name as "pru at 34000" although their parent name is different.
>>
>> pru0_0 has dev->name as "pru at 34000" and parent name as "icssg at 30000000"
>> pru1_0 has dev->name as "pru at 34000" and parent name as "icssg at 30080000"
>>
>> rproc_init() fails for pru1_0 as the the uc_pdata->name becomes same as the pru0_0.
>> More details on this issue can be found here [3]. It was suggested to use a
>> different combination if `dev->name` is not unique by Andrew Davis <afd at ti.com> [4]
>>
>> Failure Logs:
>> 	rproc_pre_probe: 'pru at 34000': using fdt
>> 	rproc_pre_probe: 'pru at 34000': using fdt
>> 	rproc_pre_probe: pru at 34000 duplicate name 'pru at 34000'
>> 	_rproc_probe_dev: pru at 34000: Failed to initialize - -22
>> 	rproc_boot: rproc_init() failed: -22
>>
>> To fix it, this commit uses combination of dev and dev's parent name.
>>
>> After this commit,
>> pru0_0 uc->pdata->name = "pru at 34000:icssg at 30000000"
>> pru1_0 uc->pdata->name = "pru at 34000:icssg at 30080000"
>>
>> Both the names are unique, thus rproc_init() succeeds.
>>
>> [1] https://elixir.bootlin.com/u-boot/v2024.07-rc3/source/dts/upstream/src/arm64/ti/k3-am64-main.dtsi#L1276
>> [2] https://elixir.bootlin.com/u-boot/v2024.07-rc3/source/dts/upstream/src/arm64/ti/k3-am64-main.dtsi#L1417
>> [3] https://lore.kernel.org/all/5cda289f-1d14-41f6-84e3-ff1d1034be13@ti.com/
>> [4] https://lore.kernel.org/all/e48f5818-182c-47ab-b384-379659830d38@ti.com/
>>
>>  drivers/remoteproc/rproc-uclass.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/rproc-uclass.c
>> index 3ba2b40dca..e550292dda 100644
>> --- a/drivers/remoteproc/rproc-uclass.c
>> +++ b/drivers/remoteproc/rproc-uclass.c
>> @@ -158,9 +158,15 @@ static int rproc_pre_probe(struct udevice *dev)
>>  		uc_pdata->driver_plat_data = pdata->driver_plat_data;
>>  	}
>>  
>> -	/* Else try using device Name */
>> -	if (!uc_pdata->name)
>> -		uc_pdata->name = dev->name;
>> +	/* Else try using a combination of device Name and devices's parent's name */
>> +	if (!uc_pdata->name) {
>> +		int rproc_name_size = 256;
> 
> Instead of hardcoding 256 why not use
> 		buf = malloc(strlen(dev->name) + strlen(dev->parent->name) + 2);	// 1 for null and one for '-'
> 
>> +		char *buf;
>> +
>> +		buf = malloc(rproc_name_size);
> 
> need to check/error out for malloc failure.

Sure Roger, I will make these changes and update the patch. Thanks for
the review.

> 
>> +		snprintf(buf, rproc_name_size, "%s-%s", dev->name, dev->parent->name);
>> +		uc_pdata->name = buf;
>> +	}
>>  	if (!uc_pdata->name) {
>>  		debug("Unnamed device!");
>>  		return -EINVAL;
> 

-- 
Thanks and Regards,
Danish