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

[Bjorn Andersson]

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