| Message ID | 20240304-qcom-scm-disable-clk-v1-1-b36e51577ca1@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails | expand |
On Wed, Mar 06, 2024 at 05:02:37PM +0100, Konrad Dybcio wrote: > > > On 3/6/24 05:10, Elliot Berman wrote: > > On Tue, Mar 05, 2024 at 10:15:19PM +0100, Konrad Dybcio wrote: > > > > > > > > > On 3/4/24 14:14, Gabor Juhos wrote: > > > > There are several functions which are calling qcom_scm_bw_enable() > > > > then returns immediately if the call fails and leaves the clocks > > > > enabled. > > > > > > > > Change the code of these functions to disable clocks when the > > > > qcom_scm_bw_enable() call fails. This also fixes a possible dma > > > > buffer leak in the qcom_scm_pas_init_image() function. > > > > > > > > Compile tested only due to lack of hardware with interconnect > > > > support. > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface") > > > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > > > > --- > > > > > > Taking a closer look, is there any argument against simply > > > putting the clk/bw en/dis calls in qcom_scm_call()? > > > > We shouldn't do this because the clk/bw en/dis calls are only needed in > > few SCM calls. > > Then the argument list could be expanded with `bool require_resources`, > or so still saving us a lot of boilerplate > I don't think there's reason for making this more general, because I think this is a problem specific to PAS - much related to Bartosz special handling of shmbridge for these calls. It would be very nice if someone could help document why this is. Regards, Bjorn
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 520de9b5633ab..e8460626fb0c4 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -569,13 +569,14 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size, ret = qcom_scm_bw_enable(); if (ret) - return ret; + goto disable_clk; desc.args[1] = mdata_phys; ret = qcom_scm_call(__scm->dev, &desc, &res); - qcom_scm_bw_disable(); + +disable_clk: qcom_scm_clk_disable(); out: @@ -637,10 +638,12 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size) ret = qcom_scm_bw_enable(); if (ret) - return ret; + goto disable_clk; ret = qcom_scm_call(__scm->dev, &desc, &res); qcom_scm_bw_disable(); + +disable_clk: qcom_scm_clk_disable(); return ret ? : res.result[0]; @@ -672,10 +675,12 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral) ret = qcom_scm_bw_enable(); if (ret) - return ret; + goto disable_clk; ret = qcom_scm_call(__scm->dev, &desc, &res); qcom_scm_bw_disable(); + +disable_clk: qcom_scm_clk_disable(); return ret ? : res.result[0]; @@ -706,11 +711,12 @@ int qcom_scm_pas_shutdown(u32 peripheral) ret = qcom_scm_bw_enable(); if (ret) - return ret; + goto disable_clk; ret = qcom_scm_call(__scm->dev, &desc, &res); - qcom_scm_bw_disable(); + +disable_clk: qcom_scm_clk_disable(); return ret ? : res.result[0];
There are several functions which are calling qcom_scm_bw_enable() then returns immediately if the call fails and leaves the clocks enabled. Change the code of these functions to disable clocks when the qcom_scm_bw_enable() call fails. This also fixes a possible dma buffer leak in the qcom_scm_pas_init_image() function. Compile tested only due to lack of hardware with interconnect support. Cc: stable@vger.kernel.org Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface") Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> --- Based on v6.8-rc7. Note: Removing the two empty lines from qcom_scm_pas_init_image() and fomr qcom_scm_pas_shutdown() functions is intentional to make those consistent with the other two functions. --- drivers/firmware/qcom/qcom_scm.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) --- base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72 change-id: 20240304-qcom-scm-disable-clk-08e7ad853fa1 Best regards,