[v1] arm64: qcom: add and enable SHM Bridge support

[PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

Posted by Bartosz Golaszewski 2 years, 3 months ago

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);
 
 	init_completion(&__scm->waitq_comp);
 
-- 
2.39.2

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

Posted by Om Prakash Singh 2 years, 2 months ago


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);
>

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

Posted by Bartosz Golaszewski 2 years, 2 months ago

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);
> >

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

Posted by Krzysztof Kozlowski 2 years, 3 months ago

On 28/08/2023 21:24, 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);

Your re-ordering is not effective here, I think. I don't understand it's
purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it
can be reordered:

"compiler is also forbidden from reordering successive instances of
READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder
other stuff.

"Ensuring that the compiler does not fold, spindle, or otherwise
mutilate accesses that either do not require ordering or that interact"
<- which means you do not require ordering here.

Best regards,
Krzysztof

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

Posted by Bartosz Golaszewski 2 years, 3 months ago

On Tue, 29 Aug 2023 at 09:59, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/08/2023 21:24, 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);
>
> Your re-ordering is not effective here, I think. I don't understand it's
> purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it
> can be reordered:
>
> "compiler is also forbidden from reordering successive instances of
> READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder
> other stuff.
>
> "Ensuring that the compiler does not fold, spindle, or otherwise
> mutilate accesses that either do not require ordering or that interact"
> <- which means you do not require ordering here.
>

Hmm, I had the list_add() implementation in mind as well as examples
from https://www.kernel.org/doc/Documentation/memory-barriers.txt and
was under the impression that WRITE_ONCE() here is enough. I need to
double check it.

Bart

Re: [PATCH 03/11] firmware: qcom-scm: atomically assign and read the global __scm pointer

Posted by Krzysztof Kozlowski 2 years, 3 months ago

On 29/08/2023 14:31, Bartosz Golaszewski wrote:
> On Tue, 29 Aug 2023 at 09:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 28/08/2023 21:24, 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);
>>
>> Your re-ordering is not effective here, I think. I don't understand it's
>> purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it
>> can be reordered:
>>
>> "compiler is also forbidden from reordering successive instances of
>> READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder
>> other stuff.
>>
>> "Ensuring that the compiler does not fold, spindle, or otherwise
>> mutilate accesses that either do not require ordering or that interact"
>> <- which means you do not require ordering here.
>>
> 
> Hmm, I had the list_add() implementation in mind as well as examples
> from https://www.kernel.org/doc/Documentation/memory-barriers.txt and
> was under the impression that WRITE_ONCE() here is enough. I need to
> double check it.

It all depends what do you want to achieve. If strict ordering of both
writes, then all the examples and descriptions from memory-barriers.txt
say that you need WRITE_ONCE in both places.

If you want to achieve something else, like scm->dev visible for other
CPUs before __scm=scm then you would need full barriers, IMHO.


Best regards,
Krzysztof