Re: [PATCH v3 1/3] remoteproc: k3-r5: Use devm_rproc_alloc() helper

[Beleswar Prasad Padhi <b-padhi@ti.com>]

Hi Andrew,

On 07/08/24 19:07, Andrew Davis wrote:
> On 8/7/24 1:22 AM, Beleswar Padhi wrote:
>> Use the device lifecycle managed allocation function. This helps prevent
>> mistakes like freeing out of order in cleanup functions and forgetting
>> to free on error paths.
>>
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index 39a47540c590..dbcd8840ae8d 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -1259,8 +1259,8 @@ static int k3_r5_cluster_rproc_init(struct 
>> platform_device *pdev)
>>               goto out;
>>           }
>>   -        rproc = rproc_alloc(cdev, dev_name(cdev), &k3_r5_rproc_ops,
>> -                    fw_name, sizeof(*kproc));
>> +        rproc = devm_rproc_alloc(cdev, dev_name(cdev), 
>> &k3_r5_rproc_ops,
>> +                     fw_name, sizeof(*kproc));
>>           if (!rproc) {
>>               ret = -ENOMEM;
>>               goto out;
>> @@ -1352,7 +1352,6 @@ static int k3_r5_cluster_rproc_init(struct 
>> platform_device *pdev)
>>   err_add:
>>       k3_r5_reserved_mem_exit(kproc);
>>   err_config:
>> -    rproc_free(rproc);
>>       core->rproc = NULL;
>
> I'd remove this line too, we don't check for NULL later, we check
> core->rproc->status, which will cause a nullptr dereference error
> instead of correctly detecting that the core is "offline".


Thanks for catching this! Will post revision with this change.

>
> Same below.
>
> Andrew
>
>>   out:
>>       /* undo core0 upon any failures on core1 in split-mode */
>> @@ -1399,7 +1398,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
>>             k3_r5_reserved_mem_exit(kproc);
>>   -        rproc_free(rproc);
>>           core->rproc = NULL;
>>       }
>>   }