2024-02-28 18:50:47

by Unnathi Chalicheemala

Subject: [PATCH 1/3] firmware: qcom-scm: Initialize waitq before setting global __scm

Suppose there's another device probing at the same time as SCM driver.
It calls into SCM and triggers WAITQ sleep but waitq completion
structure may not be initialized yet.
Fix this by moving initialization of waitq before setting global __scm.

Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
Signed-off-by: Unnathi Chalicheemala <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 520de9b5633a..c1be8270ead1 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1854,11 +1854,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
if (ret)
return ret;

+ init_completion(&scm->waitq_comp);
+
__scm = scm;
__scm->dev = &pdev->dev;

- init_completion(&__scm->waitq_comp);
-
irq = platform_get_irq_optional(pdev, 0);
if (irq < 0) {
if (irq != -ENXIO)

--
2.25.1

2024-03-05 20:36:17

by Bartosz Golaszewski

Subject: Re: [PATCH 1/3] firmware: qcom-scm: Initialize waitq before setting global __scm

On Wed, Feb 28, 2024 at 7:50 PM Unnathi Chalicheemala
<[email protected]> wrote:
>
> Suppose there's another device probing at the same time as SCM driver.
> It calls into SCM and triggers WAITQ sleep but waitq completion
> structure may not be initialized yet.
> Fix this by moving initialization of waitq before setting global __scm.
>
> Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
> Signed-off-by: Unnathi Chalicheemala <[email protected]>
> ---
> drivers/firmware/qcom/qcom_scm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..c1be8270ead1 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1854,11 +1854,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + init_completion(&scm->waitq_comp);
> +
> __scm = scm;
> __scm->dev = &pdev->dev;

If another driver can actually probe at the same time then this needs
to be moved to the end of this function, the `__scm->dev` assignment
must be done before the `__scm = scm` one and it must use
WRITE_ONCE().

Bart

>
> - init_completion(&__scm->waitq_comp);
> -
> irq = platform_get_irq_optional(pdev, 0);
> if (irq < 0) {
> if (irq != -ENXIO)
>
> --
> 2.25.1
>
>