| Message ID | 20230828192507.117334-4-bartosz.golaszewski@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On 8/29/2023 12:54 AM, Bartosz Golaszewski wrote: > Checking for the availability of SCM bridge can happen from any context. > It's only by chance that we haven't run into concurrency issues but with > the upcoming SHM Bridge driver that will be initiated at the same > initcall level, we need to assure the assignment and readback of the > __scm pointer is atomic. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/firmware/qcom_scm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 980fcfa20b9f..422de70faff8 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > */ > bool qcom_scm_is_available(void) > { > - return !!__scm; > + return !!READ_ONCE(__scm); > } > EXPORT_SYMBOL(qcom_scm_is_available); > > @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev) > if (ret) > return ret; > > - __scm = scm; > - __scm->dev = &pdev->dev; > + scm->dev = &pdev->dev; > + WRITE_ONCE(__scm, scm); In my opinion "__scm = scm;" assignment should be done at the end of probe function success so that qcom_scm_is_available() return true only when probe is completed. Other changes may not be needed. > init_completion(&__scm->waitq_comp); >
On Tue, 17 Oct 2023 at 10:25, Om Prakash Singh <quic_omprsing@quicinc.com> wrote: > > > > On 8/29/2023 12:54 AM, Bartosz Golaszewski wrote: > > Checking for the availability of SCM bridge can happen from any context. > > It's only by chance that we haven't run into concurrency issues but with > > the upcoming SHM Bridge driver that will be initiated at the same > > initcall level, we need to assure the assignment and readback of the > > __scm pointer is atomic. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/firmware/qcom_scm.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > index 980fcfa20b9f..422de70faff8 100644 > > --- a/drivers/firmware/qcom_scm.c > > +++ b/drivers/firmware/qcom_scm.c > > @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > > */ > > bool qcom_scm_is_available(void) > > { > > - return !!__scm; > > + return !!READ_ONCE(__scm); > > } > > EXPORT_SYMBOL(qcom_scm_is_available); > > > > @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev) > > if (ret) > > return ret; > > > > - __scm = scm; > > - __scm->dev = &pdev->dev; > > + scm->dev = &pdev->dev; > > + WRITE_ONCE(__scm, scm); > > In my opinion "__scm = scm;" assignment should be done at the end of > probe function success so that qcom_scm_is_available() return true only > when probe is completed. > > Other changes may not be needed. > Om, this is v1. We're at v4 (soon v5). I agree with the comment but it's outside the scope of this series. I may address it later. It has been like this for a long time. Typically this module is built-in and initialized before all other drivers so it has never caused problems. Bart > > init_completion(&__scm->waitq_comp); > >
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index 980fcfa20b9f..422de70faff8 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) */ bool qcom_scm_is_available(void) { - return !!__scm; + return !!READ_ONCE(__scm); } EXPORT_SYMBOL(qcom_scm_is_available); @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev) if (ret) return ret; - __scm = scm; - __scm->dev = &pdev->dev; + scm->dev = &pdev->dev; + WRITE_ONCE(__scm, scm); init_completion(&__scm->waitq_comp);
Checking for the availability of SCM bridge can happen from any context. It's only by chance that we haven't run into concurrency issues but with the upcoming SHM Bridge driver that will be initiated at the same initcall level, we need to assure the assignment and readback of the __scm pointer is atomic. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- drivers/firmware/qcom_scm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)