| Message ID | 20241005140150.4109700-2-quic_kuldsing@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qcom_tzmem: Enhance Error Handling for shmbridge | expand |
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 >
>> 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.
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 >
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 >
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.
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
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);