| 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/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) {
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(-)