Linux ARM-MSM sub-architecture
 help / color / mirror / Atom feed
* [PATCH v5 0/2] Fix user PD inimem requirements
@ 2024-07-22  5:54 Ekansh Gupta
  2024-07-22  5:54 ` [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD Ekansh Gupta
  2024-07-22  5:54 ` [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size Ekansh Gupta
  0 siblings, 2 replies; 10+ messages in thread
From: Ekansh Gupta @ 2024-07-22  5:54 UTC (permalink / raw)
  To: srinivas.kandagatla, linux-arm-msm
  Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd

This patch series fixes the incorrect initmem size assumptions for
signed and unsigned user PD.
Previous patch: https://lore.kernel.org/all/20240719085708.1764952-1-quic_ekangupt@quicinc.com/

Changes in v2:
  - Modified commit text.
  - Removed size check instead of updating max file size.
Changes in v3:
  - Added bound check again with a higher max size definition.
  - Modified commit text accordingly.
Changes in v4:
  - Defined new initmem specific MACROs.
  - Adding extra memory for unsigned PD.
  - Added comment suggesting the reason for this change.
  - Modified commit text.
Changes in v5:
  - Splitted the change into separate patches.

Ekansh Gupta (2):
  misc: fastrpc: Define a new initmem size for user PD
  misc: fastrpc: Increase unsigned PD initmem size

 drivers/misc/fastrpc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
2.34.1


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

* [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD
  2024-07-22  5:54 [PATCH v5 0/2] Fix user PD inimem requirements Ekansh Gupta
@ 2024-07-22  5:54 ` Ekansh Gupta
  2024-07-22  5:58   ` Greg KH
  2024-07-22  5:54 ` [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size Ekansh Gupta
  1 sibling, 1 reply; 10+ messages in thread
From: Ekansh Gupta @ 2024-07-22  5:54 UTC (permalink / raw)
  To: srinivas.kandagatla, linux-arm-msm
  Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd,
	stable

For user PD initialization, initmem is allocated and sent to DSP for
initial memory requirements like shell loading. The size of this memory
is decided based on the shell size that is passed by the user space.
With the current implementation, a minimum of 2MB is always allocated
for initmem even if the size passed by user is less than that. For this
a MACRO is being used which is intended for shell size bound check.
This minimum size of 2MB is not recommended as the PD will have very
less memory for heap and will have to request HLOS again for memory.
Define a new macro for initmem minimum length of 3MB.

Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index a7a2bcedb37e..a3a5b745936e 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -39,6 +39,7 @@
 #define FASTRPC_DSP_UTILITIES_HANDLE	2
 #define FASTRPC_CTXID_MASK (0xFF0)
 #define INIT_FILELEN_MAX (2 * 1024 * 1024)
+#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
 #define INIT_FILE_NAMELEN_MAX (128)
 #define FASTRPC_DEVICE_NAME	"fastrpc"
 
@@ -1410,7 +1411,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 			goto err;
 	}
 
-	memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
+	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
 		       1024 * 1024);
 	err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
 				&imem);
-- 
2.34.1


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

* [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size
  2024-07-22  5:54 [PATCH v5 0/2] Fix user PD inimem requirements Ekansh Gupta
  2024-07-22  5:54 ` [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD Ekansh Gupta
@ 2024-07-22  5:54 ` Ekansh Gupta
  2024-07-22  6:00   ` Greg KH
  1 sibling, 1 reply; 10+ messages in thread
From: Ekansh Gupta @ 2024-07-22  5:54 UTC (permalink / raw)
  To: srinivas.kandagatla, linux-arm-msm
  Cc: gregkh, quic_bkumar, linux-kernel, quic_chennak, dri-devel, arnd,
	stable

For unsigned PD offloading requirement, additional memory is required
because of additional static heap initialization. Without this
additional memory, PD initialization would fail. Increase the initmem
size by 2MB for unsigned PD initmem buffer allocation. Any additional
memory sent to DSP during PD init is used as the PD heap.

Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index a3a5b745936e..18668b020a87 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -40,6 +40,7 @@
 #define FASTRPC_CTXID_MASK (0xFF0)
 #define INIT_FILELEN_MAX (2 * 1024 * 1024)
 #define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
+#define FASTRPC_STATIC_HEAP_LEN (2 * 1024 * 1024)
 #define INIT_FILE_NAMELEN_MAX (128)
 #define FASTRPC_DEVICE_NAME	"fastrpc"
 
@@ -1411,8 +1412,14 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 			goto err;
 	}
 
+	/* Allocate buffer in kernel for donating to remote process.
+	 * Unsigned PD requires additional memory because of the
+	 * additional static heap initialized within the process.
+	 */
 	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
 		       1024 * 1024);
+	if (unsigned_module)
+		memlen += FASTRPC_STATIC_HEAP_LEN;
 	err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
 				&imem);
 	if (err)
-- 
2.34.1


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

* Re: [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD
  2024-07-22  5:54 ` [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD Ekansh Gupta
@ 2024-07-22  5:58   ` Greg KH
  2024-07-22  6:12     ` Ekansh Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2024-07-22  5:58 UTC (permalink / raw)
  To: Ekansh Gupta
  Cc: srinivas.kandagatla, linux-arm-msm, quic_bkumar, linux-kernel,
	quic_chennak, dri-devel, arnd, stable

On Mon, Jul 22, 2024 at 11:24:36AM +0530, Ekansh Gupta wrote:
> For user PD initialization, initmem is allocated and sent to DSP for
> initial memory requirements like shell loading. The size of this memory
> is decided based on the shell size that is passed by the user space.
> With the current implementation, a minimum of 2MB is always allocated
> for initmem even if the size passed by user is less than that. For this
> a MACRO is being used which is intended for shell size bound check.
> This minimum size of 2MB is not recommended as the PD will have very
> less memory for heap and will have to request HLOS again for memory.
> Define a new macro for initmem minimum length of 3MB.
> 
> Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index a7a2bcedb37e..a3a5b745936e 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -39,6 +39,7 @@
>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
>  #define FASTRPC_CTXID_MASK (0xFF0)
>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> +#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)

Meta-comment, for a future change, why not tabs to line things up?

>  #define INIT_FILE_NAMELEN_MAX (128)
>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>  
> @@ -1410,7 +1411,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  			goto err;
>  	}
>  
> -	memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
> +	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),

You are aligning it this way, but still checking the file size for
INIT_FILELEN_MAX, so that's not really going to change anything here.

How was this tested?

thanks,

greg k-h

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

* Re: [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size
  2024-07-22  5:54 ` [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size Ekansh Gupta
@ 2024-07-22  6:00   ` Greg KH
  2024-07-22  6:25     ` Ekansh Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2024-07-22  6:00 UTC (permalink / raw)
  To: Ekansh Gupta
  Cc: srinivas.kandagatla, linux-arm-msm, quic_bkumar, linux-kernel,
	quic_chennak, dri-devel, arnd, stable

On Mon, Jul 22, 2024 at 11:24:37AM +0530, Ekansh Gupta wrote:
> For unsigned PD offloading requirement, additional memory is required
> because of additional static heap initialization. Without this
> additional memory, PD initialization would fail. Increase the initmem
> size by 2MB for unsigned PD initmem buffer allocation. Any additional
> memory sent to DSP during PD init is used as the PD heap.
> 
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index a3a5b745936e..18668b020a87 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -40,6 +40,7 @@
>  #define FASTRPC_CTXID_MASK (0xFF0)
>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
>  #define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
> +#define FASTRPC_STATIC_HEAP_LEN (2 * 1024 * 1024)
>  #define INIT_FILE_NAMELEN_MAX (128)
>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>  
> @@ -1411,8 +1412,14 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  			goto err;
>  	}
>  
> +	/* Allocate buffer in kernel for donating to remote process.
> +	 * Unsigned PD requires additional memory because of the

What is "PD"?

> +	 * additional static heap initialized within the process.
> +	 */

Why are you using networking comment style for a non-networking file?

>  	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
>  		       1024 * 1024);
> +	if (unsigned_module)
> +		memlen += FASTRPC_STATIC_HEAP_LEN;

I don't understand, why is "static heap length" being added for
something that is "unsigned"?  Why isn't this just "SIGNING FREE SPACE"
or something like that?

thanks,

greg "naming is hard" k-h

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

* Re: [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD
  2024-07-22  5:58   ` Greg KH
@ 2024-07-22  6:12     ` Ekansh Gupta
  2024-07-22  7:39       ` Greg KH
  0 siblings, 1 reply; 10+ messages in thread
From: Ekansh Gupta @ 2024-07-22  6:12 UTC (permalink / raw)
  To: Greg KH
  Cc: srinivas.kandagatla, linux-arm-msm, quic_bkumar, linux-kernel,
	quic_chennak, dri-devel, arnd, stable



On 7/22/2024 11:28 AM, Greg KH wrote:
> On Mon, Jul 22, 2024 at 11:24:36AM +0530, Ekansh Gupta wrote:
>> For user PD initialization, initmem is allocated and sent to DSP for
>> initial memory requirements like shell loading. The size of this memory
>> is decided based on the shell size that is passed by the user space.
>> With the current implementation, a minimum of 2MB is always allocated
>> for initmem even if the size passed by user is less than that. For this
>> a MACRO is being used which is intended for shell size bound check.
>> This minimum size of 2MB is not recommended as the PD will have very
>> less memory for heap and will have to request HLOS again for memory.
>> Define a new macro for initmem minimum length of 3MB.
>>
>> Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index a7a2bcedb37e..a3a5b745936e 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -39,6 +39,7 @@
>>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
>>  #define FASTRPC_CTXID_MASK (0xFF0)
>>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
>> +#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
> Meta-comment, for a future change, why not tabs to line things up?
Sure, I'll add a comment.
Should I line up all the MACRO definitions? If yes, should I send it as a separate patch?
>
>>  #define INIT_FILE_NAMELEN_MAX (128)
>>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>>  
>> @@ -1410,7 +1411,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  			goto err;
>>  	}
>>  
>> -	memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
>> +	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
> You are aligning it this way, but still checking the file size for
> INIT_FILELEN_MAX, so that's not really going to change anything here.
File size is for the fastrpc shell that is to be loaded. This is the size of buffer allocated by user and passed
to the driver. The driver will map this buffer to SMMU before passing it to DSP. Therefore the bound
check on this size is necessary. As for initmem size for remote process on DSP, this size is also inclusive of
initial requirements of DSP size user process(user PD), hence separating it from file size macro.
> How was this tested?
This is tested with fastrpc use cases available in hexagon SDK:
https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps

--Ekansh
> thanks,
>
> greg k-h


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

* Re: [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size
  2024-07-22  6:00   ` Greg KH
@ 2024-07-22  6:25     ` Ekansh Gupta
  0 siblings, 0 replies; 10+ messages in thread
From: Ekansh Gupta @ 2024-07-22  6:25 UTC (permalink / raw)
  To: Greg KH
  Cc: srinivas.kandagatla, linux-arm-msm, quic_bkumar, linux-kernel,
	quic_chennak, dri-devel, arnd, stable



On 7/22/2024 11:30 AM, Greg KH wrote:
> On Mon, Jul 22, 2024 at 11:24:37AM +0530, Ekansh Gupta wrote:
>> For unsigned PD offloading requirement, additional memory is required
>> because of additional static heap initialization. Without this
>> additional memory, PD initialization would fail. Increase the initmem
>> size by 2MB for unsigned PD initmem buffer allocation. Any additional
>> memory sent to DSP during PD init is used as the PD heap.
>>
>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index a3a5b745936e..18668b020a87 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -40,6 +40,7 @@
>>  #define FASTRPC_CTXID_MASK (0xFF0)
>>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
>>  #define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
>> +#define FASTRPC_STATIC_HEAP_LEN (2 * 1024 * 1024)
>>  #define INIT_FILE_NAMELEN_MAX (128)
>>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>>  
>> @@ -1411,8 +1412,14 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  			goto err;
>>  	}
>>  
>> +	/* Allocate buffer in kernel for donating to remote process.
>> +	 * Unsigned PD requires additional memory because of the
> What is "PD"?
DSP PD(protection domain) is execution environment supported by DSP.
>
>> +	 * additional static heap initialized within the process.
>> +	 */
> Why are you using networking comment style for a non-networking file?
I observed similar style in this driver file. I will update this in proper style in the next patch.
>
>>  	memlen = ALIGN(max(FASTRPC_INITLEN_MIN, (int)init.filelen * 4),
>>  		       1024 * 1024);
>> +	if (unsigned_module)
>> +		memlen += FASTRPC_STATIC_HEAP_LEN;
> I don't understand, why is "static heap length" being added for
> something that is "unsigned"?  Why isn't this just "SIGNING FREE SPACE"
> or something like that?
The difference between signed PD and unsigned PD is:
Signed PD: Available on all DSPs and requires that the shared objects being loaded in the PD
are signed with a digital signature.

Unsigned PD: Sandboxed low-rights process that allows signature-free shared objects to run on
CDSP.

For unsigned PD there are some additional statically initialized heap for which additional memory
is required. I'll try to come up with a better name.

Thanks for the review.

--Ekansh
>
> thanks,
>
> greg "naming is hard" k-h
>


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

* Re: [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD
  2024-07-22  6:12     ` Ekansh Gupta
@ 2024-07-22  7:39       ` Greg KH
  2024-07-22  7:53         ` Ekansh Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2024-07-22  7:39 UTC (permalink / raw)
  To: Ekansh Gupta
  Cc: srinivas.kandagatla, linux-arm-msm, quic_bkumar, linux-kernel,
	quic_chennak, dri-devel, arnd, stable

On Mon, Jul 22, 2024 at 11:42:52AM +0530, Ekansh Gupta wrote:
> 
> 
> On 7/22/2024 11:28 AM, Greg KH wrote:
> > On Mon, Jul 22, 2024 at 11:24:36AM +0530, Ekansh Gupta wrote:
> >> For user PD initialization, initmem is allocated and sent to DSP for
> >> initial memory requirements like shell loading. The size of this memory
> >> is decided based on the shell size that is passed by the user space.
> >> With the current implementation, a minimum of 2MB is always allocated
> >> for initmem even if the size passed by user is less than that. For this
> >> a MACRO is being used which is intended for shell size bound check.
> >> This minimum size of 2MB is not recommended as the PD will have very
> >> less memory for heap and will have to request HLOS again for memory.
> >> Define a new macro for initmem minimum length of 3MB.
> >>
> >> Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
> >> Cc: stable <stable@kernel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index a7a2bcedb37e..a3a5b745936e 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -39,6 +39,7 @@
> >>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
> >>  #define FASTRPC_CTXID_MASK (0xFF0)
> >>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> >> +#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
> > Meta-comment, for a future change, why not tabs to line things up?
> Sure, I'll add a comment.

I didn't say anything about comments :(

> Should I line up all the MACRO definitions? If yes, should I send it as a separate patch?

As I said, yes, for a future change.

> > How was this tested?
> This is tested with fastrpc use cases available in hexagon SDK:
> https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps

Do you have regression tests that attempt to check the boundry
conditions and alignment here?

thanks,

greg k-h

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

* Re: [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD
  2024-07-22  7:39       ` Greg KH
@ 2024-07-22  7:53         ` Ekansh Gupta
  2024-07-22  8:21           ` Dmitry Baryshkov
  0 siblings, 1 reply; 10+ messages in thread
From: Ekansh Gupta @ 2024-07-22  7:53 UTC (permalink / raw)
  To: Greg KH
  Cc: srinivas.kandagatla, linux-arm-msm, quic_bkumar, linux-kernel,
	quic_chennak, dri-devel, arnd, stable



On 7/22/2024 1:09 PM, Greg KH wrote:
> On Mon, Jul 22, 2024 at 11:42:52AM +0530, Ekansh Gupta wrote:
>>
>> On 7/22/2024 11:28 AM, Greg KH wrote:
>>> On Mon, Jul 22, 2024 at 11:24:36AM +0530, Ekansh Gupta wrote:
>>>> For user PD initialization, initmem is allocated and sent to DSP for
>>>> initial memory requirements like shell loading. The size of this memory
>>>> is decided based on the shell size that is passed by the user space.
>>>> With the current implementation, a minimum of 2MB is always allocated
>>>> for initmem even if the size passed by user is less than that. For this
>>>> a MACRO is being used which is intended for shell size bound check.
>>>> This minimum size of 2MB is not recommended as the PD will have very
>>>> less memory for heap and will have to request HLOS again for memory.
>>>> Define a new macro for initmem minimum length of 3MB.
>>>>
>>>> Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
>>>> Cc: stable <stable@kernel.org>
>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>> ---
>>>>  drivers/misc/fastrpc.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index a7a2bcedb37e..a3a5b745936e 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -39,6 +39,7 @@
>>>>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
>>>>  #define FASTRPC_CTXID_MASK (0xFF0)
>>>>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
>>>> +#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
>>> Meta-comment, for a future change, why not tabs to line things up?
>> Sure, I'll add a comment.
> I didn't say anything about comments :(
Oops, sorry.
>
>> Should I line up all the MACRO definitions? If yes, should I send it as a separate patch?
> As I said, yes, for a future change.
Noted, thanks.
>>> How was this tested?
>> This is tested with fastrpc use cases available in hexagon SDK:
>> https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps
> Do you have regression tests that attempt to check the boundry
> conditions and alignment here?
For most of the test cases, I used the fastrpc lib:
https://github.com/quic/fastrpc

This library is taking care of passing proper shell size which is within the boundary for
all the platform that I've tried.
I'll try creating and running some regression tests for this change.

--Ekansh
>
> thanks,
>
> greg k-h


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

* Re: [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD
  2024-07-22  7:53         ` Ekansh Gupta
@ 2024-07-22  8:21           ` Dmitry Baryshkov
  0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Baryshkov @ 2024-07-22  8:21 UTC (permalink / raw)
  To: Ekansh Gupta
  Cc: Greg KH, srinivas.kandagatla, linux-arm-msm, quic_bkumar,
	linux-kernel, quic_chennak, dri-devel, arnd, stable

On Mon, Jul 22, 2024 at 01:23:56PM GMT, Ekansh Gupta wrote:
> 
> 
> On 7/22/2024 1:09 PM, Greg KH wrote:
> > On Mon, Jul 22, 2024 at 11:42:52AM +0530, Ekansh Gupta wrote:
> >>
> >> On 7/22/2024 11:28 AM, Greg KH wrote:
> >>> On Mon, Jul 22, 2024 at 11:24:36AM +0530, Ekansh Gupta wrote:
> >>>> For user PD initialization, initmem is allocated and sent to DSP for
> >>>> initial memory requirements like shell loading. The size of this memory
> >>>> is decided based on the shell size that is passed by the user space.
> >>>> With the current implementation, a minimum of 2MB is always allocated
> >>>> for initmem even if the size passed by user is less than that. For this
> >>>> a MACRO is being used which is intended for shell size bound check.
> >>>> This minimum size of 2MB is not recommended as the PD will have very
> >>>> less memory for heap and will have to request HLOS again for memory.
> >>>> Define a new macro for initmem minimum length of 3MB.
> >>>>
> >>>> Fixes: d73f71c7c6ee ("misc: fastrpc: Add support for create remote init process")
> >>>> Cc: stable <stable@kernel.org>
> >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >>>> ---
> >>>>  drivers/misc/fastrpc.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>> index a7a2bcedb37e..a3a5b745936e 100644
> >>>> --- a/drivers/misc/fastrpc.c
> >>>> +++ b/drivers/misc/fastrpc.c
> >>>> @@ -39,6 +39,7 @@
> >>>>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
> >>>>  #define FASTRPC_CTXID_MASK (0xFF0)
> >>>>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> >>>> +#define FASTRPC_INITLEN_MIN (3 * 1024 * 1024)
> >>> Meta-comment, for a future change, why not tabs to line things up?
> >> Sure, I'll add a comment.
> > I didn't say anything about comments :(
> Oops, sorry.
> >
> >> Should I line up all the MACRO definitions? If yes, should I send it as a separate patch?
> > As I said, yes, for a future change.
> Noted, thanks.
> >>> How was this tested?
> >> This is tested with fastrpc use cases available in hexagon SDK:
> >> https://developer.qualcomm.com/software/hexagon-dsp-sdk/sample-apps
> > Do you have regression tests that attempt to check the boundry
> > conditions and alignment here?
> For most of the test cases, I used the fastrpc lib:
> https://github.com/quic/fastrpc
> 
> This library is taking care of passing proper shell size which is within the boundary for
> all the platform that I've tried.
> I'll try creating and running some regression tests for this change.

Existing userspace implementation provides an example of good behaviour.
Please consider implenting and publishing 'bad' behaviour testcases
which make sure that the driver doesn't break if it gets passed 'bad'
data.

-- 
With best wishes
Dmitry

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

end of thread, other threads:[~2024-07-22  8:21 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-22  5:54 [PATCH v5 0/2] Fix user PD inimem requirements Ekansh Gupta
2024-07-22  5:54 ` [PATCH v5 1/2] misc: fastrpc: Define a new initmem size for user PD Ekansh Gupta
2024-07-22  5:58   ` Greg KH
2024-07-22  6:12     ` Ekansh Gupta
2024-07-22  7:39       ` Greg KH
2024-07-22  7:53         ` Ekansh Gupta
2024-07-22  8:21           ` Dmitry Baryshkov
2024-07-22  5:54 ` [PATCH v5 2/2] misc: fastrpc: Increase unsigned PD initmem size Ekansh Gupta
2024-07-22  6:00   ` Greg KH
2024-07-22  6:25     ` Ekansh Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox