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
