linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] remoteproc: Add support for predefined notifyids
@ 2020-01-15 10:21 Clement Leger
  2020-01-15 14:06 ` Arnaud POULIQUEN
  0 siblings, 1 reply; 10+ messages in thread
From: Clement Leger @ 2020-01-15 10:21 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson
  Cc: linux-remoteproc, linux-kernel, Clement Leger

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;
 }
 
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)
 
 /**
  * struct fw_rsc_carveout - physically contiguous memory request
-- 
2.15.0.276.g89ea799


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-15 10:21 [PATCH] remoteproc: Add support for predefined notifyids Clement Leger
@ 2020-01-15 14:06 ` Arnaud POULIQUEN
  2020-01-15 14:28   ` Clément Leger
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaud POULIQUEN @ 2020-01-15 14:06 UTC (permalink / raw)
  To: Clement Leger, Ohad Ben-Cohen, Bjorn Andersson
  Cc: linux-remoteproc, linux-kernel

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.

>  
> 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

Regards,
Arnaud
>  
>  /**
>   * struct fw_rsc_carveout - physically contiguous memory request
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-15 14:06 ` Arnaud POULIQUEN
@ 2020-01-15 14:28   ` Clément Leger
  2020-01-15 15:09     ` Arnaud POULIQUEN
  0 siblings, 1 reply; 10+ messages in thread
From: Clément Leger @ 2020-01-15 14:28 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, linux-kernel

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.

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.

> 
>>  
>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-15 14:28   ` Clément Leger
@ 2020-01-15 15:09     ` Arnaud POULIQUEN
  2020-01-15 15:11       ` Clément Leger
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaud POULIQUEN @ 2020-01-15 15:09 UTC (permalink / raw)
  To: Clément Leger
  Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, linux-kernel



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.
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.
> 
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 :)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-15 15:09     ` Arnaud POULIQUEN
@ 2020-01-15 15:11       ` Clément Leger
  2020-01-17 22:52         ` Mathieu Poirier
  0 siblings, 1 reply; 10+ messages in thread
From: Clément Leger @ 2020-01-15 15:11 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, linux-kernel



----- 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.
> 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.
>> 
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-15 15:11       ` Clément Leger
@ 2020-01-17 22:52         ` Mathieu Poirier
  2020-01-19 19:40           ` Clément Leger
  0 siblings, 1 reply; 10+ messages in thread
From: Mathieu Poirier @ 2020-01-17 22:52 UTC (permalink / raw)
  To: Clément Leger
  Cc: Arnaud Pouliquen, Ohad Ben-Cohen, Bjorn Andersson,
	linux-remoteproc, linux-kernel

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.

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. 

> > 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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-17 22:52         ` Mathieu Poirier
@ 2020-01-19 19:40           ` Clément Leger
  2020-01-20  9:52             ` Arnaud POULIQUEN
  2020-01-20 16:28             ` Mathieu Poirier
  0 siblings, 2 replies; 10+ messages in thread
From: Clément Leger @ 2020-01-19 19:40 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Arnaud Pouliquen, Ohad Ben-Cohen, Bjorn Andersson,
	linux-remoteproc, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-19 19:40           ` Clément Leger
@ 2020-01-20  9:52             ` Arnaud POULIQUEN
  2020-01-20 16:28             ` Mathieu Poirier
  1 sibling, 0 replies; 10+ messages in thread
From: Arnaud POULIQUEN @ 2020-01-20  9:52 UTC (permalink / raw)
  To: Clément Leger, Mathieu Poirier
  Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, linux-kernel

Hi,

On 1/19/20 8:40 PM, Clément Leger wrote:
> 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
My concern here is that the resource table can be uncorrelated from the Firmware.
For instance the remote proc firmware could run autonomously.
In this case only the resource table will be managed.
Should we consider that this resource table must be reloaded on a stop/start from application?
Now regarding the da that is also reset to 0 (instead of FW_RSC_ADDR_ANY) in rproc_free_vring, i suppose that
my scenario is not realistic...on restart rproc would try to allocate vring at address 0, Leading to a crash.

> 
>>
>> 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 ?
point also not clear for me. In existing code the notifyids is always overwritten.
Mathieu could you details what you have in mind here?
> 
>>
>>>> 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 ?
0 seems a valid value. I'm not aware about a constraint on the notify ID.
With the current implementation, the initial values are simply ignored, therefore do not seem to be a problem.    
With Clément's patch, the issue is for legacy firmwares with both vrings IDs initialized to the same value (0 or another value different from -1).
The rproc_alloc_vring fails while trying to reserve the same IDs for both vring.
If this patch is introduced with a new version of the resource table a constraint on the notiifyID seems acceptable.

Thanks,
Arnaud

> 
>>
>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-19 19:40           ` Clément Leger
  2020-01-20  9:52             ` Arnaud POULIQUEN
@ 2020-01-20 16:28             ` Mathieu Poirier
  2020-01-20 19:10               ` Bjorn Andersson
  1 sibling, 1 reply; 10+ messages in thread
From: Mathieu Poirier @ 2020-01-20 16:28 UTC (permalink / raw)
  To: Clément Leger
  Cc: Arnaud Pouliquen, Ohad Ben-Cohen, Bjorn Andersson,
	linux-remoteproc, linux-kernel

On Sun, 19 Jan 2020 at 12:40, Clément Leger <cleger@kalray.eu> wrote:
>
> 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 ?

My apologies for not expressing myself clearly here - let me try again.

At this time notifyids in the firmware's resource table can be set to
anything because the code will overwrite them.  With this patch
firmware images that don't have their notifyids set to -1 will see a
change in how ids are assigned, something that has the potential to
break user space.

Regards,
Mathieu

>
> >
> >> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] remoteproc: Add support for predefined notifyids
  2020-01-20 16:28             ` Mathieu Poirier
@ 2020-01-20 19:10               ` Bjorn Andersson
  0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2020-01-20 19:10 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Cl?ment Leger, Arnaud Pouliquen, Ohad Ben-Cohen,
	linux-remoteproc, linux-kernel

On Mon 20 Jan 08:28 PST 2020, Mathieu Poirier wrote:

> On Sun, 19 Jan 2020 at 12:40, Clément Leger <cleger@kalray.eu> wrote:
> >
> > 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 ?
> 
> My apologies for not expressing myself clearly here - let me try again.
> 
> At this time notifyids in the firmware's resource table can be set to
> anything because the code will overwrite them.  With this patch
> firmware images that don't have their notifyids set to -1 will see a
> change in how ids are assigned, something that has the potential to
> break user space.
> 

Right, we should have had a check in place to ensure that we only
accepted -1 here, as that's what the code relies upon.

If a conclusion on the new resource table format is imminent it sounds
reasonable to document the behaviour for the current version and make it
behave as as expected in the new one.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-01-20 19:10 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 10:21 [PATCH] remoteproc: Add support for predefined notifyids Clement Leger
2020-01-15 14:06 ` Arnaud POULIQUEN
2020-01-15 14:28   ` Clément Leger
2020-01-15 15:09     ` Arnaud POULIQUEN
2020-01-15 15:11       ` Clément Leger
2020-01-17 22:52         ` Mathieu Poirier
2020-01-19 19:40           ` Clément Leger
2020-01-20  9:52             ` Arnaud POULIQUEN
2020-01-20 16:28             ` Mathieu Poirier
2020-01-20 19:10               ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).