Re: [PATCH v4] remoteproc: stm32: fix probe error case

[Fabien DESSENNE]

Hi Mathieu


On 15/11/2019 7:55 PM, Mathieu Poirier wrote:
> Hi Fabien,
>
> On Fri, Nov 15, 2019 at 11:03:08AM +0100, Fabien Dessenne wrote:
>> If the rproc driver is probed before the mailbox driver and if the rproc
>> Device Tree node has some mailbox properties, the rproc driver probe
>> shall be deferred instead of being probed without mailbox support.
>>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@xxxxxx>
>> ---
>> Changes since v3: on error, free mailboxes from stm32_rproc_request_mbox()
>> Changes since v2: free other requested mailboxes after one request fails
>> Changes since v1: test IS_ERR() before checking PTR_ERR()
>> ---
>>   drivers/remoteproc/stm32_rproc.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index 2cf4b29..bcebb78 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -310,11 +310,12 @@ static const struct stm32_mbox stm32_rproc_mbox[MBOX_NB_MBX] = {
>>   	}
>>   };
>>   
>> -static void stm32_rproc_request_mbox(struct rproc *rproc)
>> +static int stm32_rproc_request_mbox(struct rproc *rproc)
>>   {
>>   	struct stm32_rproc *ddata = rproc->priv;
>>   	struct device *dev = &rproc->dev;
>>   	unsigned int i;
>> +	int j;
>>   	const unsigned char *name;
>>   	struct mbox_client *cl;
>>   
>> @@ -329,10 +330,20 @@ static void stm32_rproc_request_mbox(struct rproc *rproc)
>>   
>>   		ddata->mb[i].chan = mbox_request_channel_byname(cl, name);
>>   		if (IS_ERR(ddata->mb[i].chan)) {
>> +			if (PTR_ERR(ddata->mb[i].chan) == -EPROBE_DEFER)
>> +				goto err_probe;
>>   			dev_warn(dev, "cannot get %s mbox\n", name);
>>   			ddata->mb[i].chan = NULL;
>>   		}
>>   	}
>> +
>> +	return 0;
>> +
>> +err_probe:
>> +	for (j = i - 1; j >= 0; j--)
>> +		if (ddata->mb[j].chan)
>> +			mbox_free_channel(ddata->mb[j].chan);
> Do you need to set ddata->mb[i].chan to NULL as it is done in
> stm32_rproc_free_mbox?

This is probably useless : when we hit this error, we exit the probe 
function without any need to track the channels status. Later when the 
probe deferral triggers the probe call again, rproc_alloc() is called 
and zero-allocates the private data (=channels, ...)

The assignment to NULL in stm32_rproc_free_mbox is probably useless too, 
but I prefer to not clean it up now.


>
> Also I'm wondering about the error path for this function.  If something goes
> wrong in mbox_request_channel_byname() none of the previously allocated channels
> are freed and no further actions is taken.  Should we simply abort the probing
> of the rproc if any of channels can't be probed?

The mailboxes are optional (specified as DT optional properties) so we 
shall not break on mbox_request_channel() errors.


>
> Regardless of the above and without surprise:
>
> Tested-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
Thank you :)
>
>> +	return -EPROBE_DEFER;
>>   }
>>   
>>   static int stm32_rproc_set_hold_boot(struct rproc *rproc, bool hold)
>> @@ -596,7 +607,9 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> -	stm32_rproc_request_mbox(rproc);
>> +	ret = stm32_rproc_request_mbox(rproc);
>> +	if (ret)
>> +		goto free_rproc;
>>   
>>   	ret = rproc_add(rproc);
>>   	if (ret)
>> -- 
>> 2.7.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel