[v1] qcom_tzmem: Enhance Error Handling for shmbridge

[PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Kuldeep Singh 1 month, 3 weeks ago

From: Qingqing Zhou <quic_qqzhou@quicinc.com>

Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
result[0] to qcom_scm for unsupported platform, tzmem will consider this
as an unknown error not the unsupported case on the platform.

Error log:
[    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
[    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4

Change the function call qcom_scm_shm_bridge_enable() to remap this
result[0] into the unsupported error and then tzmem can consider this as
unsupported case instead of reporting an error.

Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 10986cb11ec0..620313359042 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
 	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
 };
 
+enum qcom_scm_shm_bridge_result {
+	SHMBRIDGE_RESULT_NOTSUPP	= 4,
+};
+
 #define QSEECOM_MAX_APP_NAME_SIZE		64
 
 /* Each bit configures cold/warm boot address for one of the 4 CPUs */
@@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
 
 int qcom_scm_shm_bridge_enable(void)
 {
+	int ret;
+
 	struct qcom_scm_desc desc = {
 		.svc = QCOM_SCM_SVC_MP,
 		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
@@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
 					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
 		return -EOPNOTSUPP;
 
-	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
+	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
+		return -EOPNOTSUPP;
+
+	return ret ?: res.result[0];
 }
 EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
 
-- 
2.34.1

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Bjorn Andersson 1 month, 3 weeks ago

On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:

Please shorten the subject a bit, perhaps:
"firmware: qcom: scm: Improve unsupported SHM bridge detection"

> From: Qingqing Zhou <quic_qqzhou@quicinc.com>
> 
> Currently for enabling shm bridge, QTEE will return 0 and put error 4 into

s/for/when/

> result[0] to qcom_scm for unsupported platform, tzmem will consider this
> as an unknown error not the unsupported case on the platform.
> 
> Error log:
> [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
> [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
> 
> Change the function call qcom_scm_shm_bridge_enable() to remap this
> result[0] into the unsupported error and then tzmem can consider this as
> unsupported case instead of reporting an error.
> 

Sounds like we want a Fixes tag here.

> Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
> Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..620313359042 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>  };
>  
> +enum qcom_scm_shm_bridge_result {
> +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
> +};

This is not an enumeration, but a fixed defined constant. Please use
#define.

> +
>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>  
>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
>  int qcom_scm_shm_bridge_enable(void)
>  {
> +	int ret;
> +
>  	struct qcom_scm_desc desc = {
>  		.svc = QCOM_SCM_SVC_MP,
>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>  		return -EOPNOTSUPP;
>  
> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> +		return -EOPNOTSUPP;
> +
> +	return ret ?: res.result[0];

I'd prefer, with the additional check, that you'd structure it like this:

	if (ret)
		return ret;

	if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
		return -EOPNOTSUPP;

	return res.result[0];


That way we deal with SCM-call errors first, otherwise we inspect and
act on the returned data.

That said, the return value of this function, if non-zero, will trickle
back to and be returned from qcom_scm_probe(), where Linux expects to
see a valid error code. Are there any other result[0] values we should
handle, which would allow us to end this function with "return 0"?

Regards,
Bjorn

>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
>  
> -- 
> 2.34.1
>

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Kuldeep Singh 1 month, 3 weeks ago

On 10/7/2024 7:10 AM, Bjorn Andersson wrote:
> On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
> 
> Please shorten the subject a bit, perhaps:
> "firmware: qcom: scm: Improve unsupported SHM bridge detection"
> 
>> From: Qingqing Zhou <quic_qqzhou@quicinc.com>
>>
>> Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
> 
> s/for/when/

Ack.

> 
>> result[0] to qcom_scm for unsupported platform, tzmem will consider this
>> as an unknown error not the unsupported case on the platform.
>>
>> Error log:
>> [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
>> [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
>>
>> Change the function call qcom_scm_shm_bridge_enable() to remap this
>> result[0] into the unsupported error and then tzmem can consider this as
>> unsupported case instead of reporting an error.
>>
> 
> Sounds like we want a Fixes tag here.

Ack.

> 
>> Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
>> Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
>> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 10986cb11ec0..620313359042 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
>>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>>  };
>>  
>> +enum qcom_scm_shm_bridge_result {
>> +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
>> +};
> 
> This is not an enumeration, but a fixed defined constant. Please use
> #define.

Ack.

>> +
>>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>>  
>>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
>> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>>  
>>  int qcom_scm_shm_bridge_enable(void)
>>  {
>> +	int ret;
>> +
>>  	struct qcom_scm_desc desc = {
>>  		.svc = QCOM_SCM_SVC_MP,
>>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
>> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>>  		return -EOPNOTSUPP;
>>  
>> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
>> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
>> +		return -EOPNOTSUPP;
>> +
>> +	return ret ?: res.result[0];
> 
> I'd prefer, with the additional check, that you'd structure it like this:
> 
> 	if (ret)
> 		return ret;
> 
> 	if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> 		return -EOPNOTSUPP;
> 
> 	return res.result[0];

Sure, above looks more cleaner. Will update in next rev.

> 
> That way we deal with SCM-call errors first, otherwise we inspect and
> act on the returned data.
> 
> That said, the return value of this function, if non-zero, will trickle
> back to and be returned from qcom_scm_probe(), where Linux expects to
> see a valid error code. Are there any other result[0] values we should
> handle, which would allow us to end this function with "return 0"?

As qcom_scm_shm_bridge_enable() is an shm enablement call, need to handle
supported(or unsupported) scenario appropriately and other errors can be
propagated to qcom_tzmem/qcom_scm_probe.

Please note, other return values(related to access control) from QTEE are
failures and do not require conversion to Linux error codes.

-- 
Regards
Kuldeep

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Bjorn Andersson 1 month, 3 weeks ago

On Tue, Oct 08, 2024 at 02:10:02AM GMT, Kuldeep Singh wrote:
> On 10/7/2024 7:10 AM, Bjorn Andersson wrote:
> > On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
[..]
> >> +
> >>  #define QSEECOM_MAX_APP_NAME_SIZE		64
> >>  
> >>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> >> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
> >>  
> >>  int qcom_scm_shm_bridge_enable(void)
> >>  {
> >> +	int ret;
> >> +
> >>  	struct qcom_scm_desc desc = {
> >>  		.svc = QCOM_SCM_SVC_MP,
> >>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> >> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
> >>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
> >>  		return -EOPNOTSUPP;
> >>  
> >> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> >> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> >> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	return ret ?: res.result[0];
> > 
> > I'd prefer, with the additional check, that you'd structure it like this:
> > 
> > 	if (ret)
> > 		return ret;
> > 
> > 	if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> > 		return -EOPNOTSUPP;
> > 
> > 	return res.result[0];
> 
> Sure, above looks more cleaner. Will update in next rev.
> 

Thanks!

> > 
> > That way we deal with SCM-call errors first, otherwise we inspect and
> > act on the returned data.
> > 
> > That said, the return value of this function, if non-zero, will trickle
> > back to and be returned from qcom_scm_probe(), where Linux expects to
> > see a valid error code. Are there any other result[0] values we should
> > handle, which would allow us to end this function with "return 0"?
> 
> As qcom_scm_shm_bridge_enable() is an shm enablement call, need to handle
> supported(or unsupported) scenario appropriately and other errors can be
> propagated to qcom_tzmem/qcom_scm_probe.
> 
> Please note, other return values(related to access control) from QTEE are
> failures and do not require conversion to Linux error codes.
> 

I'm not familiar with the value space of such errors, but any value
other than -EOPNOTSUPP and 0 returned here will propagate back and be
the value returned to the driver core.

It seems reasonable to ensure that the return value space makes sense to
Linux, just in case something up the stack decides to act upon the value
we return.

Regards,
Bjorn

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Dmitry Baryshkov 1 month, 3 weeks ago

On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
> From: Qingqing Zhou <quic_qqzhou@quicinc.com>
> 
> Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
> result[0] to qcom_scm for unsupported platform, tzmem will consider this
> as an unknown error not the unsupported case on the platform.
> 
> Error log:
> [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
> [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
> 
> Change the function call qcom_scm_shm_bridge_enable() to remap this
> result[0] into the unsupported error and then tzmem can consider this as
> unsupported case instead of reporting an error.
> 
> Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
> Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..620313359042 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>  };
>  
> +enum qcom_scm_shm_bridge_result {
> +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
> +};
> +
>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>  
>  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
>  int qcom_scm_shm_bridge_enable(void)
>  {
> +	int ret;
> +
>  	struct qcom_scm_desc desc = {
>  		.svc = QCOM_SCM_SVC_MP,
>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>  		return -EOPNOTSUPP;
>  
> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> +		return -EOPNOTSUPP;
> +
> +	return ret ?: res.result[0];

Could you please make it less cryptic?

if (ret)
	return ret;

if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
	return -EOPNOTSUPP;

return res.result[0];

>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Mukesh Ojha 1 month, 3 weeks ago

On Sun, Oct 06, 2024 at 08:35:57PM +0300, Dmitry Baryshkov wrote:
> On Sat, Oct 05, 2024 at 07:31:49PM GMT, Kuldeep Singh wrote:
> > From: Qingqing Zhou <quic_qqzhou@quicinc.com>
> > 
> > Currently for enabling shm bridge, QTEE will return 0 and put error 4 into
> > result[0] to qcom_scm for unsupported platform, tzmem will consider this
> > as an unknown error not the unsupported case on the platform.
> > 
> > Error log:
> > [    0.177224] qcom_scm firmware:scm: error (____ptrval____): Failed to enable the TrustZone memory allocator
> > [    0.177244] qcom_scm firmware:scm: probe with driver qcom_scm failed with error 4
> > 
> > Change the function call qcom_scm_shm_bridge_enable() to remap this
> > result[0] into the unsupported error and then tzmem can consider this as
> > unsupported case instead of reporting an error.
> > 
> > Signed-off-by: Qingqing Zhou <quic_qqzhou@quicinc.com>
> > Co-developed-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> > Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> > ---
> >  drivers/firmware/qcom/qcom_scm.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 10986cb11ec0..620313359042 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -111,6 +111,10 @@ enum qcom_scm_qseecom_tz_cmd_info {
> >  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
> >  };
> >  
> > +enum qcom_scm_shm_bridge_result {
> > +	SHMBRIDGE_RESULT_NOTSUPP	= 4,
> > +};
> > +
> >  #define QSEECOM_MAX_APP_NAME_SIZE		64
> >  
> >  /* Each bit configures cold/warm boot address for one of the 4 CPUs */
> > @@ -1361,6 +1365,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
> >  
> >  int qcom_scm_shm_bridge_enable(void)
> >  {
> > +	int ret;
> > +
> >  	struct qcom_scm_desc desc = {
> >  		.svc = QCOM_SCM_SVC_MP,
> >  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
> > @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
> >  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
> >  		return -EOPNOTSUPP;
> >  
> > -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
> > +	ret = qcom_scm_call(__scm->dev, &desc, &res);
> > +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> > +		return -EOPNOTSUPP;
> > +
> > +	return ret ?: res.result[0];
> 
> Could you please make it less cryptic?
> 
> if (ret)
> 	return ret;
> 
> if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> 	return -EOPNOTSUPP;
> 
> return res.result[0];

Ack. for this.

-Mukesh
> 
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
> >  
> > -- 
> > 2.34.1
> > 
> 
> -- 
> With best wishes
> Dmitry
>

Re: [PATCH 1/2] firmware: qcom: scm: Return -EOPNOTSUPP for unsupported SHM bridge enabling

Posted by Kuldeep Singh 1 month, 3 weeks ago

>>  int qcom_scm_shm_bridge_enable(void)
>>  {
>> +	int ret;
>> +
>>  	struct qcom_scm_desc desc = {
>>  		.svc = QCOM_SCM_SVC_MP,
>>  		.cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
>> @@ -1373,7 +1379,11 @@ int qcom_scm_shm_bridge_enable(void)
>>  					  QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
>>  		return -EOPNOTSUPP;
>>  
>> -	return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
>> +	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	if (!ret && res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
>> +		return -EOPNOTSUPP;
>> +
>> +	return ret ?: res.result[0];
> 
> Could you please make it less cryptic?
> 
> if (ret)
> 	return ret;
> 
> if (res.result[0] == SHMBRIDGE_RESULT_NOTSUPP)
> 	return -EOPNOTSUPP;
> 
> return res.result[0];

Sure Dmitry, this looks more cleaner.
Will update in next rev.

-- 
Regards
Kuldeep