Re: [PATCH] remoteproc: Add support for predefined notifyids

["Clément Leger" <cleger@kalray.eu>]

Hi Mathieu,

----- On 17 Jan, 2020, at 23:52, Mathieu Poirier mathieu.poirier@linaro.org wrote:

> Hey guys,
> 
> On Wed, Jan 15, 2020 at 04:11:03PM +0100, Clément Leger wrote:
>> 
>> 
>> ----- On 15 Jan, 2020, at 16:09, Arnaud Pouliquen arnaud.pouliquen@st.com wrote:
>> 
>> > On 1/15/20 3:28 PM, Clément Leger wrote:
>> >> Hi Arnaud,
>> >> 
>> >> ----- On 15 Jan, 2020, at 15:06, Arnaud Pouliquen arnaud.pouliquen@st.com wrote:
>> >> 
>> >>> Hi Clément,
>> >>>
>> >>> On 1/15/20 11:21 AM, Clement Leger wrote:
>> >>>> In order to support preallocated notify ids, if their value is
>> >>>> equal to FW_RSC_NOTIFY_ID_ANY, then do no allocate a notify id
>> >>>> dynamically but try to allocate the requested one. This is useful when
>> >>>> using custom ids to bind them to custom vendor resources. For instance,
>> >>>> it allow to assign a group of queues to a specific interrupti in order
>> >>>> to dispatch notifications.
>> >>>>
>> >>>> Signed-off-by: Clement Leger <cleger@kalray.eu>
>> >>>> ---
>> >>>>  drivers/remoteproc/remoteproc_core.c | 27 +++++++++++++++++++--------
>> >>>>  include/linux/remoteproc.h           |  1 +
>> >>>>  2 files changed, 20 insertions(+), 8 deletions(-)
>> >>>>
>> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> >>>> b/drivers/remoteproc/remoteproc_core.c
>> >>>> index 307df98347ba..b1485fcd0f11 100644
>> >>>> --- a/drivers/remoteproc/remoteproc_core.c
>> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
>> >>>> @@ -351,14 +351,27 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>> >>>>  	/*
>> >>>>  	 * Assign an rproc-wide unique index for this vring
>> >>>>  	 * TODO: assign a notifyid for rvdev updates as well
>> >>>> -	 * TODO: support predefined notifyids (via resource table)
>> >>>>  	 */
>> >>>> -	ret = idr_alloc(&rproc->notifyids, rvring, 0, 0, GFP_KERNEL);
>> >>>> -	if (ret < 0) {
>> >>>> -		dev_err(dev, "idr_alloc failed: %d\n", ret);
>> >>>> -		return ret;
>> >>>> +	if (rsc->vring[i].notifyid == FW_RSC_NOTIFY_ID_ANY) {
>> >>>> +		ret = idr_alloc(&rproc->notifyids, rvring, 0, 0, GFP_KERNEL);
>> >>>> +		if (ret < 0) {
>> >>>> +			dev_err(dev, "idr_alloc failed: %d\n", ret);
>> >>>> +			return ret;
>> >>>> +		}
>> >>>> +		notifyid = ret;
>> >>>> +
>> >>>> +		/* Let the rproc know the notifyid of this vring.*/
>> >>>> +		rsc->vring[i].notifyid = notifyid;
>> >>>> +	} else {
>> >>>> +		/* Reserve requested notify_id */
>> >>>> +		notifyid = rsc->vring[i].notifyid;
>> >>>> +		ret = idr_alloc(&rproc->notifyids, rvring, notifyid,
>> >>>> +				notifyid + 1, GFP_KERNEL);
>> >>>> +		if (ret < 0) {
>> >>>> +			dev_err(dev, "idr_alloc failed: %d\n", ret);
>> >>>> +			return ret;
>> >>>> +		}
>> >>>>  	}
>> >>>> -	notifyid = ret;
>> >>>>  
>> >>>>  	/* Potentially bump max_notifyid */
>> >>>>  	if (notifyid > rproc->max_notifyid)
>> >>>> @@ -366,8 +379,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>> >>>>  
>> >>>>  	rvring->notifyid = notifyid;
>> >>>>  
>> >>>> -	/* Let the rproc know the notifyid of this vring.*/
>> >>>> -	rsc->vring[i].notifyid = notifyid;
>> >>>>  	return 0;
>> >>>>  }
>> >>> The rproc_free_vring function resets the notifyid to -1 on free.
>> >>> This could generate a side effect if the resource table is not reloaded.
>> >> 
>> >> Oh indeed, I did not thought of that. What would you recommend ?
>> >> If using -1 in free vring, notify ids will be reallocated at next
>> >> round.
>> > Regarding the code i'm not sure that it is useful to reset the notifyID to -1 on
>> > free.
> 
> I'm not sure setting notifyid to -1 in rproc_free_vring() is such a big problem.
> No matter the code path I look at, if rproc_free_vring() is called something
> serious has happened and the resource table will be reloaded if another attempt
> at booting the remote processor is done.  It can also be that a graceful
> shutdown is underway, in which case the resource table will be reloaded anyway
> if/when the slave is brought back in service.
> 
> Let me know if I'm missing a scenario.

No, you are actually right

> 
> To me the real problem is if a FW image has set the notifyids in the resource
> table to 0xffffffff, thinking they will be overwritten.  In that case things
> will really south.

Hum, if set to 0xFFFFFFFF, then they will be assigned dynamically and updated
in the resource table (with this patch). But your probably mean existing code,
right ?

> 
>> > In current version, on alloc, the notifyID is overwriten without check.
>> > And as vdev status is updated, vring struct in resource table should be
>> > considered as invalid
>> > Except if i missed a usecase/race condition...
>> > 
>> >> 
>> >> I was also worried that it would break some existing user applications
>> >> which uses "0" as a notify id in vring but expect the id to be
>> >> allocated dynamically. With my modification, it means it will try to
>> >> use "0" as a predefined id, leading to allocation failure.
> 
> From my point of view they will have been lucky for all this time.  Even with
> a new version of the resource table (which I think is the right way go)
> cases like this will break.

Agreed, and actually, I may have missread some code. I can't find were I read
that. By the way, is there any documentation which state the allowed values of
notify ids ?

> 
> Thanks,
> Mathieu
> 
>> >> 
>> > Yes this could introduce regression for firmware that sets 0 as default value.
>> > Probably better to introduce this patch with a new version of the resource table
>> > :)
>> 
>> Understood ;)
>> 
>> Regards,
>> 
>> Clément
>> 
>> > 
>> > Regards
>> > Arnaud
>> >>>
>> >>>>  
>> >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> >>>> index 16ad66683ad0..dcae3394243e 100644
>> >>>> --- a/include/linux/remoteproc.h
>> >>>> +++ b/include/linux/remoteproc.h
>> >>>> @@ -123,6 +123,7 @@ enum fw_resource_type {
>> >>>>  };
>> >>>>  
>> >>>>  #define FW_RSC_ADDR_ANY (-1)
>> >>>> +#define FW_RSC_NOTIFY_ID_ANY (-1)This define can also be used in
>> >>>> rproc_free_vring
>> >> 
>> >> Indeed.
>> >> 
>> >> Thanks for your review.
>> >> 
>> >> Regards,
>> >> 
>> >> Clément
>> >> 
>> >>>
>> >>> Regards,
>> >>> Arnaud
>> >>>>  
>> >>>>  /**
> > > >>>   * struct fw_rsc_carveout - physically contiguous memory request