| Message ID | 20240227155308.18395-8-quic_mojha@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc SCM driver changes | expand |
On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: > qcom_scm_is_available() gives wrong indication if __scm > is initialized but __scm->dev is not. > > Fix this appropriately by making sure if __scm is > initialized and then it is associated with its > device. > This seems like a bug fix, and should as such have a Fixes: tag and probably Cc: stable@vger.kernel.org > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/firmware/qcom/qcom_scm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 6c252cddd44e..6f14254c0c10 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) > if (!scm) > return -ENOMEM; > > + scm->dev = &pdev->dev; > ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); > if (ret < 0) > return ret; > @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) > return ret; > > __scm = scm; > - __scm->dev = &pdev->dev; Is it sufficient to just move the line up, or do we need a barrier of some sort here? Regards, Bjorn > > init_completion(&__scm->waitq_comp); > > -- > 2.43.0.254.ga26002b62827 >
On 3/3/2024 12:55 AM, Bjorn Andersson wrote: > On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: >> qcom_scm_is_available() gives wrong indication if __scm >> is initialized but __scm->dev is not. >> >> Fix this appropriately by making sure if __scm is >> initialized and then it is associated with its >> device. >> > > This seems like a bug fix, and should as such have a Fixes: tag and > probably Cc: stable@vger.kernel.org > >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> drivers/firmware/qcom/qcom_scm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c >> index 6c252cddd44e..6f14254c0c10 100644 >> --- a/drivers/firmware/qcom/qcom_scm.c >> +++ b/drivers/firmware/qcom/qcom_scm.c >> @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) >> if (!scm) >> return -ENOMEM; >> >> + scm->dev = &pdev->dev; >> ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); >> if (ret < 0) >> return ret; >> @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) >> return ret; >> >> __scm = scm; >> - __scm->dev = &pdev->dev; > > Is it sufficient to just move the line up, or do we need a barrier of > some sort here? Would be good to use, smp_mb() before the assignment __scm = scm along with moving below line __scm->dev = &pdev->dev somewhere up. -Mukesh > > Regards, > Bjorn > >> >> init_completion(&__scm->waitq_comp); >> >> -- >> 2.43.0.254.ga26002b62827 >>
On Mon, Mar 18, 2024 at 06:38:20PM +0530, Mukesh Ojha wrote: > > > On 3/3/2024 12:55 AM, Bjorn Andersson wrote: > > On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: > > > qcom_scm_is_available() gives wrong indication if __scm > > > is initialized but __scm->dev is not. > > > > > > Fix this appropriately by making sure if __scm is > > > initialized and then it is associated with its > > > device. > > > > > > > This seems like a bug fix, and should as such have a Fixes: tag and > > probably Cc: stable@vger.kernel.org > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > --- > > > drivers/firmware/qcom/qcom_scm.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > index 6c252cddd44e..6f14254c0c10 100644 > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) > > > if (!scm) > > > return -ENOMEM; > > > + scm->dev = &pdev->dev; > > > ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); > > > if (ret < 0) > > > return ret; > > > @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) > > > return ret; > > > __scm = scm; > > > - __scm->dev = &pdev->dev; > > > > Is it sufficient to just move the line up, or do we need a barrier of > > some sort here? > > Would be good to use, smp_mb() before the assignment > __scm = scm > along with moving below line > __scm->dev = &pdev->dev > Full memory barrier is not needed here. store variant is sufficient. WRITE_ONCE() + smp_store_release() will fit here no? Thanks, Pavan
On 3/19/2024 6:47 AM, Pavan Kondeti wrote: > On Mon, Mar 18, 2024 at 06:38:20PM +0530, Mukesh Ojha wrote: >> >> >> On 3/3/2024 12:55 AM, Bjorn Andersson wrote: >>> On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: >>>> qcom_scm_is_available() gives wrong indication if __scm >>>> is initialized but __scm->dev is not. >>>> >>>> Fix this appropriately by making sure if __scm is >>>> initialized and then it is associated with its >>>> device. >>>> >>> >>> This seems like a bug fix, and should as such have a Fixes: tag and >>> probably Cc: stable@vger.kernel.org >>> >>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>> --- >>>> drivers/firmware/qcom/qcom_scm.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c >>>> index 6c252cddd44e..6f14254c0c10 100644 >>>> --- a/drivers/firmware/qcom/qcom_scm.c >>>> +++ b/drivers/firmware/qcom/qcom_scm.c >>>> @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) >>>> if (!scm) >>>> return -ENOMEM; >>>> + scm->dev = &pdev->dev; >>>> ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); >>>> if (ret < 0) >>>> return ret; >>>> @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) >>>> return ret; >>>> __scm = scm; >>>> - __scm->dev = &pdev->dev; >>> >>> Is it sufficient to just move the line up, or do we need a barrier of >>> some sort here? >> >> Would be good to use, smp_mb() before the assignment >> __scm = scm >> along with moving below line >> __scm->dev = &pdev->dev >> > > Full memory barrier is not needed here. store variant is sufficient. > WRITE_ONCE() + smp_store_release() will fit here no? Thanks for the comment, i again have a look at it and agree we don't need a full barrier here. And we can do either of the below two ways. -Mukesh // 1st way diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 49ddbcab0680..b638fb407fc6 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -1741,7 +1741,12 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm) */ bool qcom_scm_is_available(void) { - return !!__scm; + bool avail; */ bool qcom_scm_is_available(void) { - return !!__scm; + bool avail; + + avail = !!READ_ONCE(__scm); + smp_rmb(); + + return avail; } EXPORT_SYMBOL_GPL(qcom_scm_is_available); @@ -1822,10 +1827,12 @@ static int qcom_scm_probe(struct platform_device *pdev) if (!scm) return -ENOMEM; + scm->dev = &pdev->dev; ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); if (ret < 0) return ret; + init_completion(&scm->waitq_comp); mutex_init(&scm->scm_bw_lock); scm->path = devm_of_icc_get(&pdev->dev, NULL); @@ -1857,10 +1864,8 @@ static int qcom_scm_probe(struct platform_device *pdev) if (ret) return ret; - __scm = scm; - __scm->dev = &pdev->dev; - - init_completion(&__scm->waitq_comp); + smp_wmb(); + WRITE_ONCE(__scm, scm); // 2nd way diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 49ddbcab0680..911699123f9f 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -1741,7 +1741,7 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm) */ bool qcom_scm_is_available(void) { - return !!__scm; + return !!smp_load_acquire(&__scm); } EXPORT_SYMBOL_GPL(qcom_scm_is_available); @@ -1822,10 +1822,12 @@ static int qcom_scm_probe(struct platform_device *pdev) if (!scm) return -ENOMEM; + scm->dev = &pdev->dev; ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); if (ret < 0) return ret; + init_completion(&scm->waitq_comp); mutex_init(&scm->scm_bw_lock); scm->path = devm_of_icc_get(&pdev->dev, NULL); @@ -1857,10 +1859,8 @@ static int qcom_scm_probe(struct platform_device *pdev) if (ret) return ret; - __scm = scm; - __scm->dev = &pdev->dev; - - init_completion(&__scm->waitq_comp); + /* Let all above stores available after this. */ + smp_store_release(&__scm, scm); irq = platform_get_irq_optional(pdev, 0); if (irq < 0) {
On Tue, Mar 19, 2024 at 03:38:57PM +0530, Mukesh Ojha wrote: > > > On 3/19/2024 6:47 AM, Pavan Kondeti wrote: > > On Mon, Mar 18, 2024 at 06:38:20PM +0530, Mukesh Ojha wrote: > > > > > > > > > On 3/3/2024 12:55 AM, Bjorn Andersson wrote: > > > > On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: > > > > > qcom_scm_is_available() gives wrong indication if __scm > > > > > is initialized but __scm->dev is not. > > > > > > > > > > Fix this appropriately by making sure if __scm is > > > > > initialized and then it is associated with its > > > > > device. > > > > > > > > > > > > > This seems like a bug fix, and should as such have a Fixes: tag and > > > > probably Cc: stable@vger.kernel.org > > > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > > > --- > > > > > drivers/firmware/qcom/qcom_scm.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > > > index 6c252cddd44e..6f14254c0c10 100644 > > > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > > > @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) > > > > > if (!scm) > > > > > return -ENOMEM; > > > > > + scm->dev = &pdev->dev; > > > > > ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); > > > > > if (ret < 0) > > > > > return ret; > > > > > @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) > > > > > return ret; > > > > > __scm = scm; > > > > > - __scm->dev = &pdev->dev; > > > > > > > > Is it sufficient to just move the line up, or do we need a barrier of > > > > some sort here? > > > > > > Would be good to use, smp_mb() before the assignment > > > __scm = scm > > > along with moving below line > > > __scm->dev = &pdev->dev > > > > > > > Full memory barrier is not needed here. store variant is sufficient. > > WRITE_ONCE() + smp_store_release() will fit here no? > > Thanks for the comment, i again have a look at it and agree we don't > need a full barrier here. > > And we can do either of the below two ways. > > -Mukesh > > > // 1st way > > diff --git a/drivers/firmware/qcom/qcom_scm.c > b/drivers/firmware/qcom/qcom_scm.c > index 49ddbcab0680..b638fb407fc6 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -1741,7 +1741,12 @@ static int qcom_scm_qseecom_init(struct qcom_scm > *scm) > */ > bool qcom_scm_is_available(void) > { > - return !!__scm; > + bool avail; > */ > bool qcom_scm_is_available(void) > { > - return !!__scm; > + bool avail; > + > + avail = !!READ_ONCE(__scm); > + smp_rmb(); > + > + return avail; > } > EXPORT_SYMBOL_GPL(qcom_scm_is_available); > Your original problem statement: qcom_scm_is_available() gives wrong indication if __scm is initialized but __scm->dev is not. This does not require read side barrier as there is an address dependency. If the writer does it *correctly*, the reader would always observe __scm->dev != NULL when __scm != NULL without any barrier. Thanks, Pavan
On 3/19/2024 3:52 PM, Pavan Kondeti wrote: > On Tue, Mar 19, 2024 at 03:38:57PM +0530, Mukesh Ojha wrote: >> >> >> On 3/19/2024 6:47 AM, Pavan Kondeti wrote: >>> On Mon, Mar 18, 2024 at 06:38:20PM +0530, Mukesh Ojha wrote: >>>> >>>> >>>> On 3/3/2024 12:55 AM, Bjorn Andersson wrote: >>>>> On Tue, Feb 27, 2024 at 09:23:06PM +0530, Mukesh Ojha wrote: >>>>>> qcom_scm_is_available() gives wrong indication if __scm >>>>>> is initialized but __scm->dev is not. >>>>>> >>>>>> Fix this appropriately by making sure if __scm is >>>>>> initialized and then it is associated with its >>>>>> device. >>>>>> >>>>> >>>>> This seems like a bug fix, and should as such have a Fixes: tag and >>>>> probably Cc: stable@vger.kernel.org >>>>> >>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>>>> --- >>>>>> drivers/firmware/qcom/qcom_scm.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c >>>>>> index 6c252cddd44e..6f14254c0c10 100644 >>>>>> --- a/drivers/firmware/qcom/qcom_scm.c >>>>>> +++ b/drivers/firmware/qcom/qcom_scm.c >>>>>> @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) >>>>>> if (!scm) >>>>>> return -ENOMEM; >>>>>> + scm->dev = &pdev->dev; >>>>>> ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); >>>>>> if (ret < 0) >>>>>> return ret; >>>>>> @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) >>>>>> return ret; >>>>>> __scm = scm; >>>>>> - __scm->dev = &pdev->dev; >>>>> >>>>> Is it sufficient to just move the line up, or do we need a barrier of >>>>> some sort here? >>>> >>>> Would be good to use, smp_mb() before the assignment >>>> __scm = scm >>>> along with moving below line >>>> __scm->dev = &pdev->dev >>>> >>> >>> Full memory barrier is not needed here. store variant is sufficient. >>> WRITE_ONCE() + smp_store_release() will fit here no? >> >> Thanks for the comment, i again have a look at it and agree we don't >> need a full barrier here. >> >> And we can do either of the below two ways. >> >> -Mukesh >> >> >> // 1st way >> >> diff --git a/drivers/firmware/qcom/qcom_scm.c >> b/drivers/firmware/qcom/qcom_scm.c >> index 49ddbcab0680..b638fb407fc6 100644 >> --- a/drivers/firmware/qcom/qcom_scm.c >> +++ b/drivers/firmware/qcom/qcom_scm.c >> @@ -1741,7 +1741,12 @@ static int qcom_scm_qseecom_init(struct qcom_scm >> *scm) >> */ >> bool qcom_scm_is_available(void) >> { >> - return !!__scm; >> + bool avail; >> */ >> bool qcom_scm_is_available(void) >> { >> - return !!__scm; >> + bool avail; >> + >> + avail = !!READ_ONCE(__scm); >> + smp_rmb(); >> + >> + return avail; >> } >> EXPORT_SYMBOL_GPL(qcom_scm_is_available); >> > > Your original problem statement: qcom_scm_is_available() gives wrong indication > if __scm is initialized but __scm->dev is not. > > This does not require read side barrier as there is an address > dependency. If the writer does it *correctly*, the reader would always > observe __scm->dev != NULL when __scm != NULL without any barrier. It looks like write barrier pairs with an address-dependency barrier, a control dependency, an acquire barrier, a release barrier, a read barrier, or a general barrier. So, smp_rmb() is redundant here. Also, for correction, we may not need smp_load_acquire() in the 1st way and just using READ_ONCE() is enough. -Mukesh > > Thanks, > Pavan
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 6c252cddd44e..6f14254c0c10 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -1859,6 +1859,7 @@ static int qcom_scm_probe(struct platform_device *pdev) if (!scm) return -ENOMEM; + scm->dev = &pdev->dev; ret = qcom_scm_find_dload_address(&pdev->dev, &scm->dload_mode_addr); if (ret < 0) return ret; @@ -1895,7 +1896,6 @@ static int qcom_scm_probe(struct platform_device *pdev) return ret; __scm = scm; - __scm->dev = &pdev->dev; init_completion(&__scm->waitq_comp);
qcom_scm_is_available() gives wrong indication if __scm is initialized but __scm->dev is not. Fix this appropriately by making sure if __scm is initialized and then it is associated with its device. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- drivers/firmware/qcom/qcom_scm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)