[PATCH v2 1/4] firmware: qcom: scm: Remove log reporting memory allocation failure

[PATCH v2 1/4] firmware: qcom: scm: Remove log reporting memory allocation failure

[Mukesh Ojha]

Remove redundant memory allocation failure.

WARNING: Possible unnecessary 'out of memory' message
+       if (!mdata_buf) {
+               dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
---
Changes in v2: https://lore.kernel.org/lkml/20240227155308.18395-7-quic_mojha@quicinc.com/
 - Added R-by tag

 drivers/firmware/qcom/qcom_scm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 49ddbcab0680..a11fb063cc67 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -554,10 +554,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	 */
 	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
 				       GFP_KERNEL);
-	if (!mdata_buf) {
-		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
+	if (!mdata_buf)
 		return -ENOMEM;
-	}
+
 	memcpy(mdata_buf, metadata, size);
 
 	ret = qcom_scm_clk_enable();
-- 
2.7.4

[PATCH v2 2/4] firmware: scm: Remove redundant scm argument from qcom_scm_waitq_wakeup()

[Mukesh Ojha]

Remove redundant scm argument from qcom_scm_waitq_wakeup().

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v2: https://lore.kernel.org/lkml/20240227155308.18395-10-quic_mojha@quicinc.com/
 - Fixed incorrect word in the commit text.  

 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 a11fb063cc67..d9153204cba3 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1771,7 +1771,7 @@ int qcom_scm_wait_for_wq_completion(u32 wq_ctx)
 	return 0;
 }
 
-static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx)
+static int qcom_scm_waitq_wakeup(unsigned int wq_ctx)
 {
 	int ret;
 
@@ -1803,7 +1803,7 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
 			goto out;
 		}
 
-		ret = qcom_scm_waitq_wakeup(scm, wq_ctx);
+		ret = qcom_scm_waitq_wakeup(wq_ctx);
 		if (ret)
 			goto out;
 	} while (more_pending);
-- 
2.7.4

Re: [PATCH v2 2/4] firmware: scm: Remove redundant scm argument from qcom_scm_waitq_wakeup()

[Konrad Dybcio]

On 21.03.2024 16:24, Mukesh Ojha wrote:
> Remove redundant scm argument from qcom_scm_waitq_wakeup().
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 3/4] firmware: qcom: scm: Rework dload mode availability check

[Mukesh Ojha]

QCOM_SCM_BOOT_SET_DLOAD_MODE scm command is applicable for very
older SoCs where this command is supported from firmware and
for newer SoCs, dload mode tcsr registers is used for setting
the download mode.

Currently, qcom_scm_set_download_mode() checks for availability
of QCOM_SCM_BOOT_SET_DLOAD_MODE command even for SoCs where this
is not used. Fix this by switching the condition to keep the
command availability check only if dload mode registers are not
available.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
---
Changes in v2: https://lore.kernel.org/lkml/20240227155308.18395-5-quic_mojha@quicinc.com/
 - Reworded the commit text.

 drivers/firmware/qcom/qcom_scm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index d9153204cba3..e238ebe21099 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -495,17 +495,14 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 
 static void qcom_scm_set_download_mode(bool enable)
 {
-	bool avail;
 	int ret = 0;
 
-	avail = __qcom_scm_is_call_available(__scm->dev,
-					     QCOM_SCM_SVC_BOOT,
-					     QCOM_SCM_BOOT_SET_DLOAD_MODE);
-	if (avail) {
-		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
-	} else if (__scm->dload_mode_addr) {
+	if (__scm->dload_mode_addr) {
 		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+					 enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+	} else if (__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_BOOT,
+						QCOM_SCM_BOOT_SET_DLOAD_MODE)) {
+		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4

Re: [PATCH v2 3/4] firmware: qcom: scm: Rework dload mode availability check

[Konrad Dybcio]

On 21.03.2024 16:24, Mukesh Ojha wrote:
> QCOM_SCM_BOOT_SET_DLOAD_MODE scm command is applicable for very
> older SoCs where this command is supported from firmware and
> for newer SoCs, dload mode tcsr registers is used for setting
> the download mode.

This is a bit of a spaghetti sentence, but I get what you're saying.

And the patch looks good!

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 4/4] firmware: qcom: scm: Fix __scm and waitq completion variable initialization

[Mukesh Ojha]

It is possible qcom_scm_is_available() gives wrong indication that
if __scm is initialized while __scm->dev is not and similar issue
is also possible with __scm->waitq_comp.

Fix this appropriately by the use of release barrier and read barrier
that will make sure if __scm is initialized so, is all of its field
variable.

Fixes: d0f6fa7ba2d6 ("firmware: qcom: scm: Convert SCM to platform driver")
Fixes: 6bf325992236 ("firmware: qcom: scm: Add wait-queue handling logic")
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v2:
 - Added barrier instruction to make the stores available only
   after __scm initialization.
 - Moved __scm->waitq_comp initialized slight up in program order.
 - Added Fixes tag.

 drivers/firmware/qcom/qcom_scm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index e238ebe21099..f014a934a603 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1737,7 +1737,7 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm)
  */
 bool qcom_scm_is_available(void)
 {
-	return !!__scm;
+	return !!READ_ONCE(__scm);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_is_available);
 
@@ -1818,10 +1818,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);
@@ -1853,10 +1855,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 be available after this */
+	smp_store_release(&__scm, scm);
 
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0) {
-- 
2.7.4

Re: [PATCH v2 1/4] firmware: qcom: scm: Remove log reporting memory allocation failure

[Mukesh Ojha]

Gentle ping..

-Mukesh

On 3/21/2024 8:53 PM, Mukesh Ojha wrote:
> Remove redundant memory allocation failure.
> 
> WARNING: Possible unnecessary 'out of memory' message
> +       if (!mdata_buf) {
> +               dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> ---
> Changes in v2: https://lore.kernel.org/lkml/20240227155308.18395-7-quic_mojha@quicinc.com/
>   - Added R-by tag
> 
>   drivers/firmware/qcom/qcom_scm.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 49ddbcab0680..a11fb063cc67 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -554,10 +554,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>   	 */
>   	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
>   				       GFP_KERNEL);
> -	if (!mdata_buf) {
> -		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> +	if (!mdata_buf)
>   		return -ENOMEM;
> -	}
> +
>   	memcpy(mdata_buf, metadata, size);
>   
>   	ret = qcom_scm_clk_enable();

Re: (subset) [PATCH v2 1/4] firmware: qcom: scm: Remove log reporting memory allocation failure

[Bjorn Andersson]

On Thu, 21 Mar 2024 20:53:59 +0530, Mukesh Ojha wrote:
> Remove redundant memory allocation failure.
> 
> WARNING: Possible unnecessary 'out of memory' message
> +       if (!mdata_buf) {
> +               dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> 
> 
> [...]

Applied, thanks!

[1/4] firmware: qcom: scm: Remove log reporting memory allocation failure
      commit: 3de990f7895906a7a18d2dff63e3e525acaa4ecc
[2/4] firmware: scm: Remove redundant scm argument from qcom_scm_waitq_wakeup()
      commit: 000636d91d605f6209a635a29d0487af5b12b237
[3/4] firmware: qcom: scm: Rework dload mode availability check
      commit: 398a4c58f3f29ac3ff4d777dc91fe40a07bbca8c
[4/4] firmware: qcom: scm: Fix __scm and waitq completion variable initialization
      commit: 2e4955167ec5c04534cebea9e8273a907e7a75e1

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>