[PATCH v12 0/9] Misc SCM driver changes

[PATCH v12 0/9] Misc SCM driver changes

[Mukesh Ojha]

Earlier version was just introducing secure rmw API introduction
and its use in pinctrl/scm dload register.

Current version also seems to fix some of the identified issue
in scm driver code.

Patch #1-3 patches are for secure rmw api.
Patch #4 is slight optimization for the newer SoCs.
Patch #5 is using the introduce api.
Patch #6-9 try to avoid NUll pointer or to remove redundant code.

Change from v11: https://lore.kernel.org/lkml/1704727654-13999-1-git-send-email-quic_mojha@quicinc.com/
 - New patches #1 and #6-9
 - Renamed scm_query_lock to scm_lock and reuse it in qcom_scm_io_rmw()
 - Added comment for scm_lock

Changes from v10:
 - Rebased on linux-next tag 20240108

Changes from v9: https://lore.kernel.org/lkml/1698648967-974-1-git-send-email-quic_mojha@quicinc.com/
 - Added 3/4 new patch.
 - commit subject modification.

Change from v8: https://lore.kernel.org/lkml/1698235506-16993-1-git-send-email-quic_mojha@quicinc.com/
 - Introduce enum for dload mode constants as per suggestion from [Elliot].
 - Rebased on linux-next.

Changes from v7: https://lore.kernel.org/lkml/1696440338-12561-1-git-send-email-quic_mojha@quicinc.com/
 - Rebased it on next-20231025.
 - Added reviewed-by tag and take care of comment made about
   commit text should be in imperative mode.
 - Modified the name of the API to qcom_scm_io_rmw() as per suggestion
   made by [Dmitry]
 - Moved spinlock inside qcom_scm structure.
 - Corrected the patch order as per subsystem SCM first then pinctrl.

Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mojha@quicinc.com/ - Removed mistakenly added macros.
   https://lore.kernel.org/lkml/9da888dc-401a-4cbb-b616-b4654fa79e35@quicinc.com/
 - Added Acked-by tag from Linus.w to 2/3.
Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mojha@quicinc.com/
 - Removed mistakenly added macros.
   https://lore.kernel.org/lkml/9da888dc-401a-4cbb-b616-b4654fa79e35@quicinc.com/
 - Added Acked-by tag from Linus.w to 2/3.

Changes in v6: https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
 - Rebased it on latest tag available on linux-next
 - Added missed Poovendhan sign-off on 15/17 and tested-by tag from
   Kathiravan. Thanks to him for testing and reminding me of missing sign-off.
 - Addressed comments made on dload mode patch v6 version

Changes in v5: https://lore.kernel.org/lkml/1680017869-22421-1-git-send-email-quic_mojha@quicinc.com/
  - Tried to fix the issue reported by kernel test robot
    https://lore.kernel.org/lkml/202303280535.acb66sQT-lkp@intel.com/

  - Applied some of the improvement suggested by [Bjorn.andersson]

    . Dropped 'both' instead support full,mini or mini,full for setting download
    mode to collect both minidump and full dump.

    . logging improvement.

Changes in v4: https://lore.kernel.org/lkml/1679935281-18445-1-git-send-email-quic_mojha@quicinc.com/
  - val should be shifted within the function [srinivas.kandagatla]
    i.e new = (old & ~mask) | (val << ffs(mask) - 1);
  - Added Acked-by [linus.walleij] on pinctrl change.

Changes in v3 : https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/
 - Removed [1] from the series and sent as a separate patch[2], although this series
   should be applied on top [2].
  [1] https://lore.kernel.org/lkml/1677664555-30191-2-git-send-email-quic_mojha@quicinc.com/
  [2] https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/
 - Introduce new exported symbol on suggestion from [srinivas.kandagatla]
 - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
 - Addressed comment given by [dmitry.baryshkov]
 - Converted non-standard Originally-by to Signed-off-by.

Changes in v2: https://lore.kernel.org/lkml/1677664555-30191-1-git-send-email-quic_mojha@quicinc.com/
 - Addressed comment made by [bjorn]
 - Added download mask.
 - Passed download mode as parameter
 - Accept human accepatable download mode string.
 - enable = !!dload_mode
 - Shifted module param callback to somewhere down in
   the file so that it no longer need to know the
   prototype of qcom_scm_set_download_mode()
 - updated commit text.

Mukesh Ojha (9):
  firmware: qcom: scm: Rename scm_query_lock to scm_lock
  firmware: qcom: scm: provide a read-modify-write function
  firmware: qcom: scm: Modify only the download bits in TCSR register
  firmware: qcom: scm: Rework dload mode availability check
  pinctrl: qcom: Use qcom_scm_io_rmw() function
  firmware: qcom: scm: Remove log reporting memory allocation failure
  firmware: qcom: scm: Fix __scm->dev assignement
  firmware: qcom: scm: Add check to prevent Null pointer dereference
  firmware: scm: Remove redundant scm argument from
    qcom_scm_waitq_wakeup()

 drivers/firmware/qcom/qcom_scm.c       | 162 +++++++++++++++++++------
 drivers/pinctrl/qcom/pinctrl-msm.c     |  10 +-
 include/linux/firmware/qcom/qcom_scm.h |   1 +
 3 files changed, 128 insertions(+), 45 deletions(-)

-- 
2.43.0.254.ga26002b62827

[PATCH v12 1/9] firmware: qcom: scm: Rename scm_query_lock to scm_lock

[Mukesh Ojha]

scm_query_lock is global spin lock and only used for query
purpose with trustzone and that too for one time to get the
convention of scm communication. It is possible that, it
can reused for other purpose.

Rename scm_query_lock to scm_lock.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 520de9b5633a..2d0ba529cf56 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -193,7 +193,7 @@ static void qcom_scm_bw_disable(void)
 }
 
 enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
-static DEFINE_SPINLOCK(scm_query_lock);
+static DEFINE_SPINLOCK(scm_lock);
 
 static enum qcom_scm_convention __get_convention(void)
 {
@@ -250,14 +250,14 @@ static enum qcom_scm_convention __get_convention(void)
 
 	probed_convention = SMC_CONVENTION_LEGACY;
 found:
-	spin_lock_irqsave(&scm_query_lock, flags);
+	spin_lock_irqsave(&scm_lock, flags);
 	if (probed_convention != qcom_scm_convention) {
 		qcom_scm_convention = probed_convention;
 		pr_info("qcom_scm: convention: %s%s\n",
 			qcom_scm_convention_names[qcom_scm_convention],
 			forced ? " (forced)" : "");
 	}
-	spin_unlock_irqrestore(&scm_query_lock, flags);
+	spin_unlock_irqrestore(&scm_lock, flags);
 
 	return qcom_scm_convention;
 }
-- 
2.43.0.254.ga26002b62827

[PATCH v12 2/9] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

It is possible that different bits of a secure register is used
for different purpose and currently, there is no such available
function from SCM driver to do that; one similar usage was pointed
by Srinivas K. inside pinctrl-msm where interrupt configuration
register lying in secure region and written via read-modify-write
operation.

Export qcom_scm_io_rmw() to do read-modify-write operation on secure
register and reuse it wherever applicable, also document scm_lock
to convey its usage.

Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
---
 drivers/firmware/qcom/qcom_scm.c       | 33 ++++++++++++++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 2d0ba529cf56..8f766fce5f7c 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -193,6 +193,11 @@ static void qcom_scm_bw_disable(void)
 }
 
 enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
+/*
+ * scm_lock to serialize call to query SMC convention and
+ * to atomically operate(read-modify-write) on different
+ * bits of secure register.
+ */
 static DEFINE_SPINLOCK(scm_lock);
 
 static enum qcom_scm_convention __get_convention(void)
@@ -481,6 +486,34 @@ static int qcom_scm_disable_sdi(void)
 	return ret ? : res.result[0];
 }
 
+int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
+{
+	unsigned long flags;
+	unsigned int old;
+	unsigned int new;
+	int ret;
+
+	if (!__scm)
+		return -EPROBE_DEFER;
+
+	/*
+	 * Lock to atomically do rmw operation on different bits
+	 * of secure register
+	 */
+	spin_lock_irqsave(&scm_lock, flags);
+	ret = qcom_scm_io_readl(addr, &old);
+	if (ret)
+		goto unlock;
+
+	new = (old & ~mask) | (val & mask);
+
+	ret = qcom_scm_io_writel(addr, new);
+unlock:
+	spin_unlock_irqrestore(&scm_lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index ccaf28846054..3a8bb2e603b3 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
 
 int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
 
 bool qcom_scm_restore_sec_cfg_available(void);
 int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-- 
2.43.0.254.ga26002b62827

[PATCH v12 3/9] firmware: qcom: scm: Modify only the download bits in TCSR register

[Mukesh Ojha]

Crashdump collection is done based on DLOAD bits of TCSR register.
To retain other bits, scm driver need to read the register and
modify only the DLOAD bits, as other bits in TCSR may have their
own significance.

Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 8f766fce5f7c..bd6bfdf2d828 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -4,6 +4,8 @@
  */
 
 #include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/cpumask.h>
@@ -114,6 +116,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
 #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
 
+#define QCOM_DLOAD_MASK		GENMASK(5, 4)
+enum qcom_dload_mode {
+	QCOM_DLOAD_NODUMP	= 0,
+	QCOM_DLOAD_FULLDUMP	= 1,
+};
+
 static const char * const qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_UNKNOWN] = "unknown",
 	[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -531,6 +539,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 
 static void qcom_scm_set_download_mode(bool enable)
 {
+	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
 	bool avail;
 	int ret = 0;
 
@@ -540,8 +549,9 @@ static void qcom_scm_set_download_mode(bool enable)
 	if (avail) {
 		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
 	} else if (__scm->dload_mode_addr) {
-		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+		ret = qcom_scm_io_rmw(__scm->dload_mode_addr,
+				      QCOM_DLOAD_MASK,
+				      FIELD_PREP(QCOM_DLOAD_MASK, val));
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.43.0.254.ga26002b62827

[PATCH v12 4/9] firmware: qcom: scm: Rework dload mode availability check

[Mukesh Ojha]

QCOM_SCM_BOOT_SET_DLOAD_MODE was only valid for very older
target and firmware and for recent targets there is dload
mode tcsr registers available to set the download mode.

So, it is better to keep it as fallback check instead of
checking its availability and failing it always.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index bd6bfdf2d828..3fd89cddba3b 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -540,18 +540,16 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 static void qcom_scm_set_download_mode(bool enable)
 {
 	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
-	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_rmw(__scm->dload_mode_addr,
 				      QCOM_DLOAD_MASK,
 				      FIELD_PREP(QCOM_DLOAD_MASK, val));
+	} 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.43.0.254.ga26002b62827

[PATCH v12 5/9] pinctrl: qcom: Use qcom_scm_io_rmw() function

[Mukesh Ojha]

Use qcom_scm_io_rmw() exported function in pinctrl-msm
driver.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
@Linus.Walleij,

I have removed your Ack on this patch after your comment
on https://lore.kernel.org/lkml/CACRpkdbnj3W3k=snTx3iadHWU+RNv9GY4B3O4K0hu8TY+DrK=Q@mail.gmail.com/

If you agree on the current solution, please ack this again.

 drivers/pinctrl/qcom/pinctrl-msm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index aeaf0d1958f5..1bd5c8c43fcd 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1082,22 +1082,20 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	if (g->intr_target_width)
 		intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
 
+	intr_target_mask <<= g->intr_target_bit;
 	if (pctrl->intr_target_use_scm) {
 		u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
 		int ret;
 
-		qcom_scm_io_readl(addr, &val);
-		val &= ~(intr_target_mask << g->intr_target_bit);
-		val |= g->intr_target_kpss_val << g->intr_target_bit;
-
-		ret = qcom_scm_io_writel(addr, val);
+		val = g->intr_target_kpss_val << g->intr_target_bit;
+		ret = qcom_scm_io_rmw(addr, intr_target_mask, val);
 		if (ret)
 			dev_err(pctrl->dev,
 				"Failed routing %lu interrupt to Apps proc",
 				d->hwirq);
 	} else {
 		val = msm_readl_intr_target(pctrl, g);
-		val &= ~(intr_target_mask << g->intr_target_bit);
+		val &= ~intr_target_mask;
 		val |= g->intr_target_kpss_val << g->intr_target_bit;
 		msm_writel_intr_target(val, pctrl, g);
 	}
-- 
2.43.0.254.ga26002b62827

[PATCH v12 6/9] 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>
---
 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 3fd89cddba3b..6c252cddd44e 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -598,10 +598,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.43.0.254.ga26002b62827

[PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Mukesh Ojha]

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

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);
 
-- 
2.43.0.254.ga26002b62827

[PATCH v12 8/9] firmware: qcom: scm: Add check to prevent Null pointer dereference

[Mukesh Ojha]

There are multiple place in SCM driver __scm->dev is being
accessed without checking if it is valid or not and all
not all of function needs the device but it is needed
for some cases when the number of argument passed is more
and dma_map_single () api is used.

Add a NULL check for the cases when it is fine even to pass
device as NULL and add qcom_scm_is_available() check for
cases when it is needed for DMA api's.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 88 ++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 6f14254c0c10..a1dce417e6ec 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -465,7 +465,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 	struct qcom_scm_res res;
 	int ret;
 
-	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
 
 	return ret ? : res.result[0];
 }
@@ -591,6 +591,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	};
 	struct qcom_scm_res res;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	/*
 	 * During the scm call memory protection will be enabled for the meta
 	 * data blob, so make sure it's physically contiguous, 4K aligned and
@@ -637,6 +640,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
  */
 void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
 {
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	if (!ctx->ptr)
 		return;
 
@@ -671,6 +677,9 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
 	};
 	struct qcom_scm_res res;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	ret = qcom_scm_clk_enable();
 	if (ret)
 		return ret;
@@ -706,6 +715,9 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
 	};
 	struct qcom_scm_res res;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	ret = qcom_scm_clk_enable();
 	if (ret)
 		return ret;
@@ -740,6 +752,9 @@ int qcom_scm_pas_shutdown(u32 peripheral)
 	};
 	struct qcom_scm_res res;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	ret = qcom_scm_clk_enable();
 	if (ret)
 		return ret;
@@ -776,11 +791,11 @@ bool qcom_scm_pas_supported(u32 peripheral)
 	};
 	struct qcom_scm_res res;
 
-	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
+	if (!__qcom_scm_is_call_available(__scm ? __scm->dev : NULL, QCOM_SCM_SVC_PIL,
 					  QCOM_SCM_PIL_PAS_IS_SUPPORTED))
 		return false;
 
-	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
 
 	return ret ? false : !!res.result[0];
 }
@@ -840,7 +855,7 @@ int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
 	int ret;
 
 
-	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
+	ret = qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, &res);
 	if (ret >= 0)
 		*val = res.result[0];
 
@@ -859,7 +874,7 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 
-	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
+	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
 
@@ -871,7 +886,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
  */
 bool qcom_scm_restore_sec_cfg_available(void)
 {
-	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
+	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
+					    QCOM_SCM_SVC_MP,
 					    QCOM_SCM_MP_RESTORE_SEC_CFG);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg_available);
@@ -889,7 +905,7 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
 	struct qcom_scm_res res;
 	int ret;
 
-	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
 
 	return ret ? : res.result[0];
 }
@@ -907,7 +923,7 @@ int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
 	struct qcom_scm_res res;
 	int ret;
 
-	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
 
 	if (size)
 		*size = res.result[0];
@@ -930,7 +946,7 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
 	};
 	int ret;
 
-	ret = qcom_scm_call(__scm->dev, &desc, NULL);
+	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
 
 	/* the pg table has been initialized already, ignore the error */
 	if (ret == -EPERM)
@@ -951,7 +967,7 @@ int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 
-	return qcom_scm_call(__scm->dev, &desc, NULL);
+	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_cp_pool_size);
 
@@ -973,7 +989,7 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
 	};
 	struct qcom_scm_res res;
 
-	ret = qcom_scm_call(__scm->dev, &desc, &res);
+	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
 
 	return ret ? : res.result[0];
 }
@@ -1038,6 +1054,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	int ret, i, b;
 	u64 srcvm_bits = *srcvm;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	src_sz = hweight64(srcvm_bits) * sizeof(*src);
 	mem_to_map_sz = sizeof(*mem_to_map);
 	dest_sz = dest_cnt * sizeof(*destvm);
@@ -1093,7 +1112,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
  */
 bool qcom_scm_ocmem_lock_available(void)
 {
-	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_OCMEM,
+	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
+					    QCOM_SCM_SVC_OCMEM,
 					    QCOM_SCM_OCMEM_LOCK_CMD);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock_available);
@@ -1120,7 +1140,7 @@ int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
 		.arginfo = QCOM_SCM_ARGS(4),
 	};
 
-	return qcom_scm_call(__scm->dev, &desc, NULL);
+	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock);
 
@@ -1143,7 +1163,7 @@ int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
 		.arginfo = QCOM_SCM_ARGS(3),
 	};
 
-	return qcom_scm_call(__scm->dev, &desc, NULL);
+	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
 
@@ -1155,9 +1175,11 @@ EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
  */
 bool qcom_scm_ice_available(void)
 {
-	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
+	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
+					    QCOM_SCM_SVC_ES,
 					    QCOM_SCM_ES_INVALIDATE_ICE_KEY) &&
-		__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
+		__qcom_scm_is_call_available(__scm ?__scm->dev : NULL,
+					     QCOM_SCM_SVC_ES,
 					     QCOM_SCM_ES_CONFIG_SET_ICE_KEY);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_ice_available);
@@ -1184,7 +1206,7 @@ int qcom_scm_ice_invalidate_key(u32 index)
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 
-	return qcom_scm_call(__scm->dev, &desc, NULL);
+	return qcom_scm_call(__scm ?__scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_ice_invalidate_key);
 
@@ -1228,6 +1250,9 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
 	dma_addr_t key_phys;
 	int ret;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	/*
 	 * 'key' may point to vmalloc()'ed memory, but we need to pass a
 	 * physical address that's been properly flushed.  The sanctioned way to
@@ -1262,7 +1287,12 @@ EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
 bool qcom_scm_hdcp_available(void)
 {
 	bool avail;
-	int ret = qcom_scm_clk_enable();
+	int ret;
+
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
+	ret = qcom_scm_clk_enable();
 
 	if (ret)
 		return ret;
@@ -1307,6 +1337,9 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 	};
 	struct qcom_scm_res res;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
 		return -ERANGE;
 
@@ -1335,7 +1368,7 @@ int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt)
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 
-	return qcom_scm_call(__scm->dev, &desc, NULL);
+	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_pt_format);
 
@@ -1351,13 +1384,15 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
 	};
 
 
-	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
+	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_qsmmu500_wait_safe_toggle);
 
 bool qcom_scm_lmh_dcvsh_available(void)
 {
-	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH);
+	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
+					    QCOM_SCM_SVC_LMH,
+					    QCOM_SCM_LMH_LIMIT_DCVSH);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
 
@@ -1371,7 +1406,7 @@ int qcom_scm_lmh_profile_change(u32 profile_id)
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 
-	return qcom_scm_call(__scm->dev, &desc, NULL);
+	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_lmh_profile_change);
 
@@ -1394,6 +1429,9 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL);
 	if (!payload_buf)
 		return -ENOMEM;
@@ -1568,6 +1606,9 @@ int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
 	char *name_buf;
 	int status;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	if (app_name_len >= name_buf_size)
 		return -EINVAL;
 
@@ -1638,6 +1679,9 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
 	dma_addr_t rsp_phys;
 	int status;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	/* Map request buffer */
 	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
 	status = dma_mapping_error(__scm->dev, req_phys);
-- 
2.43.0.254.ga26002b62827

[PATCH v12 9/9] firmware: scm: Remove redundant scm argument from qcom_scm_waitq_wakeup()

[Mukesh Ojha]

Remove unused scm argument from qcom_scm_waitq_wakeup().

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 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 a1dce417e6ec..d91f5872e003 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1853,7 +1853,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;
 
@@ -1885,7 +1885,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.43.0.254.ga26002b62827

Re: [PATCH v12 8/9] firmware: qcom: scm: Add check to prevent Null pointer dereference

[Elliot Berman]

On Tue, Feb 27, 2024 at 09:23:07PM +0530, Mukesh Ojha wrote:
> There are multiple place in SCM driver __scm->dev is being
> accessed without checking if it is valid or not and all
> not all of function needs the device but it is needed
> for some cases when the number of argument passed is more
> and dma_map_single () api is used.
> 
> Add a NULL check for the cases when it is fine even to pass
> device as NULL and add qcom_scm_is_available() check for
> cases when it is needed for DMA api's.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 88 ++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 6f14254c0c10..a1dce417e6ec 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -465,7 +465,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>  	struct qcom_scm_res res;
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);

We're doing this ternary a lot. Maybe an macro would help with
readability?

static inline struct device *scm_dev()
{
	return __scm ? __scm->dev : NULL;
}

and then we can do

ret = qcom_scm_call(scm_dev(), &desc, &res);

>  
>  	return ret ? : res.result[0];
>  }
> @@ -591,6 +591,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	/*
>  	 * During the scm call memory protection will be enabled for the meta
>  	 * data blob, so make sure it's physically contiguous, 4K aligned and
> @@ -637,6 +640,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>   */
>  void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
>  {
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	if (!ctx->ptr)
>  		return;
>  
> @@ -671,6 +677,9 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -706,6 +715,9 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -740,6 +752,9 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -776,11 +791,11 @@ bool qcom_scm_pas_supported(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> -	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
> +	if (!__qcom_scm_is_call_available(__scm ? __scm->dev : NULL, QCOM_SCM_SVC_PIL,
>  					  QCOM_SCM_PIL_PAS_IS_SUPPORTED))
>  		return false;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? false : !!res.result[0];
>  }
> @@ -840,7 +855,7 @@ int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
>  	int ret;
>  
>  
> -	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, &res);
>  	if (ret >= 0)
>  		*val = res.result[0];
>  
> @@ -859,7 +874,7 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> +	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
>  
> @@ -871,7 +886,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
>   */
>  bool qcom_scm_restore_sec_cfg_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_MP,
>  					    QCOM_SCM_MP_RESTORE_SEC_CFG);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg_available);
> @@ -889,7 +905,7 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
>  	struct qcom_scm_res res;
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? : res.result[0];
>  }
> @@ -907,7 +923,7 @@ int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
>  	struct qcom_scm_res res;
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	if (size)
>  		*size = res.result[0];
> @@ -930,7 +946,7 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
>  	};
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, NULL);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  
>  	/* the pg table has been initialized already, ignore the error */
>  	if (ret == -EPERM)
> @@ -951,7 +967,7 @@ int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_cp_pool_size);
>  
> @@ -973,7 +989,7 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>  	};
>  	struct qcom_scm_res res;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? : res.result[0];
>  }
> @@ -1038,6 +1054,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  	int ret, i, b;
>  	u64 srcvm_bits = *srcvm;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	src_sz = hweight64(srcvm_bits) * sizeof(*src);
>  	mem_to_map_sz = sizeof(*mem_to_map);
>  	dest_sz = dest_cnt * sizeof(*destvm);
> @@ -1093,7 +1112,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>   */
>  bool qcom_scm_ocmem_lock_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_OCMEM,
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_OCMEM,
>  					    QCOM_SCM_OCMEM_LOCK_CMD);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock_available);
> @@ -1120,7 +1140,7 @@ int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
>  		.arginfo = QCOM_SCM_ARGS(4),
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock);
>  
> @@ -1143,7 +1163,7 @@ int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
>  		.arginfo = QCOM_SCM_ARGS(3),
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>  
> @@ -1155,9 +1175,11 @@ EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>   */
>  bool qcom_scm_ice_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_ES,
>  					    QCOM_SCM_ES_INVALIDATE_ICE_KEY) &&
> -		__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
> +		__qcom_scm_is_call_available(__scm ?__scm->dev : NULL,
> +					     QCOM_SCM_SVC_ES,
>  					     QCOM_SCM_ES_CONFIG_SET_ICE_KEY);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ice_available);
> @@ -1184,7 +1206,7 @@ int qcom_scm_ice_invalidate_key(u32 index)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ?__scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ice_invalidate_key);
>  
> @@ -1228,6 +1250,9 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>  	dma_addr_t key_phys;
>  	int ret;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	/*
>  	 * 'key' may point to vmalloc()'ed memory, but we need to pass a
>  	 * physical address that's been properly flushed.  The sanctioned way to
> @@ -1262,7 +1287,12 @@ EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
>  bool qcom_scm_hdcp_available(void)
>  {
>  	bool avail;
> -	int ret = qcom_scm_clk_enable();
> +	int ret;
> +
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	ret = qcom_scm_clk_enable();
>  
>  	if (ret)
>  		return ret;
> @@ -1307,6 +1337,9 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
>  		return -ERANGE;
>  
> @@ -1335,7 +1368,7 @@ int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_pt_format);
>  
> @@ -1351,13 +1384,15 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>  	};
>  
>  
> -	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> +	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_qsmmu500_wait_safe_toggle);
>  
>  bool qcom_scm_lmh_dcvsh_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH);
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_LMH,
> +					    QCOM_SCM_LMH_LIMIT_DCVSH);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
> @@ -1371,7 +1406,7 @@ int qcom_scm_lmh_profile_change(u32 profile_id)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_lmh_profile_change);
>  
> @@ -1394,6 +1429,9 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL);
>  	if (!payload_buf)
>  		return -ENOMEM;
> @@ -1568,6 +1606,9 @@ int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
>  	char *name_buf;
>  	int status;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	if (app_name_len >= name_buf_size)
>  		return -EINVAL;
>  
> @@ -1638,6 +1679,9 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>  	dma_addr_t rsp_phys;
>  	int status;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	/* Map request buffer */
>  	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
>  	status = dma_mapping_error(__scm->dev, req_phys);
> -- 
> 2.43.0.254.ga26002b62827
> 
> 

Re: [PATCH v12 8/9] firmware: qcom: scm: Add check to prevent Null pointer dereference

[Mukesh Ojha]

On 2/27/2024 10:26 PM, Elliot Berman wrote:
> On Tue, Feb 27, 2024 at 09:23:07PM +0530, Mukesh Ojha wrote:
>> There are multiple place in SCM driver __scm->dev is being
>> accessed without checking if it is valid or not and all
>> not all of function needs the device but it is needed
>> for some cases when the number of argument passed is more
>> and dma_map_single () api is used.
>>
>> Add a NULL check for the cases when it is fine even to pass
>> device as NULL and add qcom_scm_is_available() check for
>> cases when it is needed for DMA api's.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/firmware/qcom/qcom_scm.c | 88 ++++++++++++++++++++++++--------
>>   1 file changed, 66 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 6f14254c0c10..a1dce417e6ec 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -465,7 +465,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>>   	struct qcom_scm_res res;
>>   	int ret;
>>   
>> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
> 
> We're doing this ternary a lot. Maybe an macro would help with
> readability?
> 
> static inline struct device *scm_dev()
> {
> 	return __scm ? __scm->dev : NULL;
> }
> 
> and then we can do
> 
> ret = qcom_scm_call(scm_dev(), &desc, &res);
> 

Sure, will apply.

-Mukesh

>>   
>>   	return ret ? : res.result[0];
>>   }
>> @@ -591,6 +591,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	/*
>>   	 * During the scm call memory protection will be enabled for the meta
>>   	 * data blob, so make sure it's physically contiguous, 4K aligned and
>> @@ -637,6 +640,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>>    */
>>   void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
>>   {
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	if (!ctx->ptr)
>>   		return;
>>   
>> @@ -671,6 +677,9 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	ret = qcom_scm_clk_enable();
>>   	if (ret)
>>   		return ret;
>> @@ -706,6 +715,9 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	ret = qcom_scm_clk_enable();
>>   	if (ret)
>>   		return ret;
>> @@ -740,6 +752,9 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	ret = qcom_scm_clk_enable();
>>   	if (ret)
>>   		return ret;
>> @@ -776,11 +791,11 @@ bool qcom_scm_pas_supported(u32 peripheral)
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> -	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
>> +	if (!__qcom_scm_is_call_available(__scm ? __scm->dev : NULL, QCOM_SCM_SVC_PIL,
>>   					  QCOM_SCM_PIL_PAS_IS_SUPPORTED))
>>   		return false;
>>   
>> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>>   
>>   	return ret ? false : !!res.result[0];
>>   }
>> @@ -840,7 +855,7 @@ int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
>>   	int ret;
>>   
>>   
>> -	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
>> +	ret = qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, &res);
>>   	if (ret >= 0)
>>   		*val = res.result[0];
>>   
>> @@ -859,7 +874,7 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
>>   		.owner = ARM_SMCCC_OWNER_SIP,
>>   	};
>>   
>> -	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
>>   
>> @@ -871,7 +886,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
>>    */
>>   bool qcom_scm_restore_sec_cfg_available(void)
>>   {
>> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
>> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
>> +					    QCOM_SCM_SVC_MP,
>>   					    QCOM_SCM_MP_RESTORE_SEC_CFG);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg_available);
>> @@ -889,7 +905,7 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
>>   	struct qcom_scm_res res;
>>   	int ret;
>>   
>> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>>   
>>   	return ret ? : res.result[0];
>>   }
>> @@ -907,7 +923,7 @@ int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
>>   	struct qcom_scm_res res;
>>   	int ret;
>>   
>> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>>   
>>   	if (size)
>>   		*size = res.result[0];
>> @@ -930,7 +946,7 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
>>   	};
>>   	int ret;
>>   
>> -	ret = qcom_scm_call(__scm->dev, &desc, NULL);
>> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>>   
>>   	/* the pg table has been initialized already, ignore the error */
>>   	if (ret == -EPERM)
>> @@ -951,7 +967,7 @@ int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
>>   		.owner = ARM_SMCCC_OWNER_SIP,
>>   	};
>>   
>> -	return qcom_scm_call(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_cp_pool_size);
>>   
>> @@ -973,7 +989,7 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
>> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>>   
>>   	return ret ? : res.result[0];
>>   }
>> @@ -1038,6 +1054,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>   	int ret, i, b;
>>   	u64 srcvm_bits = *srcvm;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	src_sz = hweight64(srcvm_bits) * sizeof(*src);
>>   	mem_to_map_sz = sizeof(*mem_to_map);
>>   	dest_sz = dest_cnt * sizeof(*destvm);
>> @@ -1093,7 +1112,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>>    */
>>   bool qcom_scm_ocmem_lock_available(void)
>>   {
>> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_OCMEM,
>> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
>> +					    QCOM_SCM_SVC_OCMEM,
>>   					    QCOM_SCM_OCMEM_LOCK_CMD);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock_available);
>> @@ -1120,7 +1140,7 @@ int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
>>   		.arginfo = QCOM_SCM_ARGS(4),
>>   	};
>>   
>> -	return qcom_scm_call(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock);
>>   
>> @@ -1143,7 +1163,7 @@ int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
>>   		.arginfo = QCOM_SCM_ARGS(3),
>>   	};
>>   
>> -	return qcom_scm_call(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>>   
>> @@ -1155,9 +1175,11 @@ EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>>    */
>>   bool qcom_scm_ice_available(void)
>>   {
>> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
>> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
>> +					    QCOM_SCM_SVC_ES,
>>   					    QCOM_SCM_ES_INVALIDATE_ICE_KEY) &&
>> -		__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
>> +		__qcom_scm_is_call_available(__scm ?__scm->dev : NULL,
>> +					     QCOM_SCM_SVC_ES,
>>   					     QCOM_SCM_ES_CONFIG_SET_ICE_KEY);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_ice_available);
>> @@ -1184,7 +1206,7 @@ int qcom_scm_ice_invalidate_key(u32 index)
>>   		.owner = ARM_SMCCC_OWNER_SIP,
>>   	};
>>   
>> -	return qcom_scm_call(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call(__scm ?__scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_ice_invalidate_key);
>>   
>> @@ -1228,6 +1250,9 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>>   	dma_addr_t key_phys;
>>   	int ret;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	/*
>>   	 * 'key' may point to vmalloc()'ed memory, but we need to pass a
>>   	 * physical address that's been properly flushed.  The sanctioned way to
>> @@ -1262,7 +1287,12 @@ EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
>>   bool qcom_scm_hdcp_available(void)
>>   {
>>   	bool avail;
>> -	int ret = qcom_scm_clk_enable();
>> +	int ret;
>> +
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>> +	ret = qcom_scm_clk_enable();
>>   
>>   	if (ret)
>>   		return ret;
>> @@ -1307,6 +1337,9 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>>   	};
>>   	struct qcom_scm_res res;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
>>   		return -ERANGE;
>>   
>> @@ -1335,7 +1368,7 @@ int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt)
>>   		.owner = ARM_SMCCC_OWNER_SIP,
>>   	};
>>   
>> -	return qcom_scm_call(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_pt_format);
>>   
>> @@ -1351,13 +1384,15 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>>   	};
>>   
>>   
>> -	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_qsmmu500_wait_safe_toggle);
>>   
>>   bool qcom_scm_lmh_dcvsh_available(void)
>>   {
>> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH);
>> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
>> +					    QCOM_SCM_SVC_LMH,
>> +					    QCOM_SCM_LMH_LIMIT_DCVSH);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>>   
>> @@ -1371,7 +1406,7 @@ int qcom_scm_lmh_profile_change(u32 profile_id)
>>   		.owner = ARM_SMCCC_OWNER_SIP,
>>   	};
>>   
>> -	return qcom_scm_call(__scm->dev, &desc, NULL);
>> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_lmh_profile_change);
>>   
>> @@ -1394,6 +1429,9 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
>>   		.owner = ARM_SMCCC_OWNER_SIP,
>>   	};
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL);
>>   	if (!payload_buf)
>>   		return -ENOMEM;
>> @@ -1568,6 +1606,9 @@ int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
>>   	char *name_buf;
>>   	int status;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	if (app_name_len >= name_buf_size)
>>   		return -EINVAL;
>>   
>> @@ -1638,6 +1679,9 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>>   	dma_addr_t rsp_phys;
>>   	int status;
>>   
>> +	if (!qcom_scm_is_available())
>> +		return -EPROBE_DEFER;
>> +
>>   	/* Map request buffer */
>>   	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
>>   	status = dma_mapping_error(__scm->dev, req_phys);
>> -- 
>> 2.43.0.254.ga26002b62827
>>
>>

Re: [PATCH v12 5/9] pinctrl: qcom: Use qcom_scm_io_rmw() function

[Linus Walleij]

On Tue, Feb 27, 2024 at 4:53 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> Use qcom_scm_io_rmw() exported function in pinctrl-msm
> driver.
>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> @Linus.Walleij,
>
> I have removed your Ack on this patch after your comment
> on https://lore.kernel.org/lkml/CACRpkdbnj3W3k=snTx3iadHWU+RNv9GY4B3O4K0hu8TY+DrK=Q@mail.gmail.com/
>
> If you agree on the current solution, please ack this again.

It's fine, I trust you guys mostly :)
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v12 8/9] firmware: qcom: scm: Add check to prevent Null pointer dereference

[Konrad Dybcio]

On 27.02.2024 16:53, Mukesh Ojha wrote:
> There are multiple place in SCM driver __scm->dev is being
> accessed without checking if it is valid or not and all
> not all of function needs the device but it is needed
> for some cases when the number of argument passed is more
> and dma_map_single () api is used.
> 
> Add a NULL check for the cases when it is fine even to pass
> device as NULL and add qcom_scm_is_available() check for
> cases when it is needed for DMA api's.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---

Most (all?) drivers calling into SCM already check is_available()
at probe time. I'm not sure returning -EPROBE_DEFER would be good
for calls outside .probe.

Konrad

Re: [PATCH v12 8/9] firmware: qcom: scm: Add check to prevent Null pointer dereference

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:07PM +0530, Mukesh Ojha wrote:
> There are multiple place in SCM driver __scm->dev is being
> accessed without checking if it is valid

Why is this a problem?

> or not and all
> not all of function needs the device but it is needed
> for some cases when the number of argument passed is more
> and dma_map_single () api is used.
> 

Why can't we just always pass NULL in these cases then?

> Add a NULL check for the cases when it is fine even to pass
> device as NULL and add qcom_scm_is_available() check for
> cases when it is needed for DMA api's.
> 

It could be argued that returning an error for this scenario could be
making things more robust. But I can see no scenario where the calling
driver would be able to react in a suitable way when getting
-EPROBE_DEFERRED back.

Our current philosophy is that the client will do probe deferral by
invoking qcom_scm_is_available() at probe time. Please provide an
adequate description of the problem that you're solving by introducing
all these checks.

And pick a return value that is appropriate for the context these
functions are being expected to be called in.


PS. Why are you extending the scope of this series? You're just making
sure that your original patches aren't getting merged.

Regards,
Bjorn

> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 88 ++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 6f14254c0c10..a1dce417e6ec 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -465,7 +465,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>  	struct qcom_scm_res res;
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? : res.result[0];
>  }
> @@ -591,6 +591,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	/*
>  	 * During the scm call memory protection will be enabled for the meta
>  	 * data blob, so make sure it's physically contiguous, 4K aligned and
> @@ -637,6 +640,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>   */
>  void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
>  {
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	if (!ctx->ptr)
>  		return;
>  
> @@ -671,6 +677,9 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -706,6 +715,9 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -740,6 +752,9 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -776,11 +791,11 @@ bool qcom_scm_pas_supported(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> -	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
> +	if (!__qcom_scm_is_call_available(__scm ? __scm->dev : NULL, QCOM_SCM_SVC_PIL,
>  					  QCOM_SCM_PIL_PAS_IS_SUPPORTED))
>  		return false;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? false : !!res.result[0];
>  }
> @@ -840,7 +855,7 @@ int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
>  	int ret;
>  
>  
> -	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, &res);
>  	if (ret >= 0)
>  		*val = res.result[0];
>  
> @@ -859,7 +874,7 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> +	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
>  
> @@ -871,7 +886,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_io_writel);
>   */
>  bool qcom_scm_restore_sec_cfg_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_MP,
>  					    QCOM_SCM_MP_RESTORE_SEC_CFG);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg_available);
> @@ -889,7 +905,7 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
>  	struct qcom_scm_res res;
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? : res.result[0];
>  }
> @@ -907,7 +923,7 @@ int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
>  	struct qcom_scm_res res;
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	if (size)
>  		*size = res.result[0];
> @@ -930,7 +946,7 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
>  	};
>  	int ret;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, NULL);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  
>  	/* the pg table has been initialized already, ignore the error */
>  	if (ret == -EPERM)
> @@ -951,7 +967,7 @@ int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_cp_pool_size);
>  
> @@ -973,7 +989,7 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>  	};
>  	struct qcom_scm_res res;
>  
> -	ret = qcom_scm_call(__scm->dev, &desc, &res);
> +	ret = qcom_scm_call(__scm ? __scm->dev : NULL, &desc, &res);
>  
>  	return ret ? : res.result[0];
>  }
> @@ -1038,6 +1054,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  	int ret, i, b;
>  	u64 srcvm_bits = *srcvm;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	src_sz = hweight64(srcvm_bits) * sizeof(*src);
>  	mem_to_map_sz = sizeof(*mem_to_map);
>  	dest_sz = dest_cnt * sizeof(*destvm);
> @@ -1093,7 +1112,8 @@ EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>   */
>  bool qcom_scm_ocmem_lock_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_OCMEM,
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_OCMEM,
>  					    QCOM_SCM_OCMEM_LOCK_CMD);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock_available);
> @@ -1120,7 +1140,7 @@ int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
>  		.arginfo = QCOM_SCM_ARGS(4),
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_lock);
>  
> @@ -1143,7 +1163,7 @@ int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
>  		.arginfo = QCOM_SCM_ARGS(3),
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>  
> @@ -1155,9 +1175,11 @@ EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>   */
>  bool qcom_scm_ice_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_ES,
>  					    QCOM_SCM_ES_INVALIDATE_ICE_KEY) &&
> -		__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
> +		__qcom_scm_is_call_available(__scm ?__scm->dev : NULL,
> +					     QCOM_SCM_SVC_ES,
>  					     QCOM_SCM_ES_CONFIG_SET_ICE_KEY);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ice_available);
> @@ -1184,7 +1206,7 @@ int qcom_scm_ice_invalidate_key(u32 index)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ?__scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ice_invalidate_key);
>  
> @@ -1228,6 +1250,9 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>  	dma_addr_t key_phys;
>  	int ret;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	/*
>  	 * 'key' may point to vmalloc()'ed memory, but we need to pass a
>  	 * physical address that's been properly flushed.  The sanctioned way to
> @@ -1262,7 +1287,12 @@ EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
>  bool qcom_scm_hdcp_available(void)
>  {
>  	bool avail;
> -	int ret = qcom_scm_clk_enable();
> +	int ret;
> +
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	ret = qcom_scm_clk_enable();
>  
>  	if (ret)
>  		return ret;
> @@ -1307,6 +1337,9 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>  	};
>  	struct qcom_scm_res res;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
>  		return -ERANGE;
>  
> @@ -1335,7 +1368,7 @@ int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_iommu_set_pt_format);
>  
> @@ -1351,13 +1384,15 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>  	};
>  
>  
> -	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> +	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_qsmmu500_wait_safe_toggle);
>  
>  bool qcom_scm_lmh_dcvsh_available(void)
>  {
> -	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH);
> +	return __qcom_scm_is_call_available(__scm ? __scm->dev : NULL,
> +					    QCOM_SCM_SVC_LMH,
> +					    QCOM_SCM_LMH_LIMIT_DCVSH);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
> @@ -1371,7 +1406,7 @@ int qcom_scm_lmh_profile_change(u32 profile_id)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(__scm ? __scm->dev : NULL, &desc, NULL);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_lmh_profile_change);
>  
> @@ -1394,6 +1429,9 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL);
>  	if (!payload_buf)
>  		return -ENOMEM;
> @@ -1568,6 +1606,9 @@ int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
>  	char *name_buf;
>  	int status;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	if (app_name_len >= name_buf_size)
>  		return -EINVAL;
>  
> @@ -1638,6 +1679,9 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>  	dma_addr_t rsp_phys;
>  	int status;
>  
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
>  	/* Map request buffer */
>  	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
>  	status = dma_mapping_error(__scm->dev, req_phys);
> -- 
> 2.43.0.254.ga26002b62827
> 

Re: [PATCH v12 2/9] firmware: qcom: scm: provide a read-modify-write function

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:01PM +0530, Mukesh Ojha wrote:
> It is possible that different bits of a secure register is used
> for different purpose and currently, there is no such available
> function from SCM driver to do that; one similar usage was pointed
> by Srinivas K. inside pinctrl-msm where interrupt configuration
> register lying in secure region and written via read-modify-write
> operation.
> 
> Export qcom_scm_io_rmw() to do read-modify-write operation on secure
> register and reuse it wherever applicable, also document scm_lock
> to convey its usage.
> 
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
> ---
>  drivers/firmware/qcom/qcom_scm.c       | 33 ++++++++++++++++++++++++++
>  include/linux/firmware/qcom/qcom_scm.h |  1 +
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 2d0ba529cf56..8f766fce5f7c 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -193,6 +193,11 @@ static void qcom_scm_bw_disable(void)
>  }
>  
>  enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
> +/*
> + * scm_lock to serialize call to query SMC convention and
> + * to atomically operate(read-modify-write) on different
> + * bits of secure register.
> + */
>  static DEFINE_SPINLOCK(scm_lock);
>  
>  static enum qcom_scm_convention __get_convention(void)
> @@ -481,6 +486,34 @@ static int qcom_scm_disable_sdi(void)
>  	return ret ? : res.result[0];
>  }
>  
> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> +	unsigned long flags;
> +	unsigned int old;
> +	unsigned int new;
> +	int ret;
> +
> +	if (!__scm)
> +		return -EPROBE_DEFER;
> +
> +	/*
> +	 * Lock to atomically do rmw operation on different bits
> +	 * of secure register
> +	 */

A spinlock does not make something globally atomic, all you have made
sure is that:
1) qcom_scm_io_rmw() can not happen concurrently with __get_convention()

The reuse of the lock makes me wonder what the use case you're having a
need to protect #1... When is rmw happening concurrently with convention
detection?

2) Two qcom_scm_io_rmw() can not happen concurrently

What happens if a separate process invokes qcom_scm_io_write() of the
same address concurrently with qcom_scm_io_rmw()?


Quite likely neither of these will happen in practice, and I'm guessing
that there will not be any caching issues etc among different calls to
qcom_scm_io_*(), and hence this spinlock serves just to complicate the
implementation.

Please add a kernel-doc comment to this function (and perhaps
qcom_scm_io_write()) and describe that we don't guarantee this operation
to happen atomically - or if you have a valid reason, document that and
it's exact limitations.


PS. I would have been perfectly happy with us not adding a rmw function.
You're adding 34 lines of code to save 2*3 lines of duplicated, easy to
understand, code.

Regards,
Bjorn

> +	spin_lock_irqsave(&scm_lock, flags);
> +	ret = qcom_scm_io_readl(addr, &old);
> +	if (ret)
> +		goto unlock;
> +
> +	new = (old & ~mask) | (val & mask);
> +
> +	ret = qcom_scm_io_writel(addr, new);
> +unlock:
> +	spin_unlock_irqrestore(&scm_lock, flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> +
>  static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>  {
>  	struct qcom_scm_desc desc = {
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index ccaf28846054..3a8bb2e603b3 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
>  
>  int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>  int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
>  
>  bool qcom_scm_restore_sec_cfg_available(void);
>  int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
> -- 
> 2.43.0.254.ga26002b62827
> 

Re: [PATCH v12 1/9] firmware: qcom: scm: Rename scm_query_lock to scm_lock

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:00PM +0530, Mukesh Ojha wrote:
> scm_query_lock is global spin lock and only used for query
> purpose with trustzone and that too for one time to get the
> convention of scm communication. It is possible that, it
> can reused for other purpose.
> 

This is not a good principle to follow for something as complex as
locking...

Regards,
Bjorn

> Rename scm_query_lock to scm_lock.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..2d0ba529cf56 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -193,7 +193,7 @@ static void qcom_scm_bw_disable(void)
>  }
>  
>  enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
> -static DEFINE_SPINLOCK(scm_query_lock);
> +static DEFINE_SPINLOCK(scm_lock);
>  
>  static enum qcom_scm_convention __get_convention(void)
>  {
> @@ -250,14 +250,14 @@ static enum qcom_scm_convention __get_convention(void)
>  
>  	probed_convention = SMC_CONVENTION_LEGACY;
>  found:
> -	spin_lock_irqsave(&scm_query_lock, flags);
> +	spin_lock_irqsave(&scm_lock, flags);
>  	if (probed_convention != qcom_scm_convention) {
>  		qcom_scm_convention = probed_convention;
>  		pr_info("qcom_scm: convention: %s%s\n",
>  			qcom_scm_convention_names[qcom_scm_convention],
>  			forced ? " (forced)" : "");
>  	}
> -	spin_unlock_irqrestore(&scm_query_lock, flags);
> +	spin_unlock_irqrestore(&scm_lock, flags);
>  
>  	return qcom_scm_convention;
>  }
> -- 
> 2.43.0.254.ga26002b62827
> 

Re: [PATCH v12 3/9] firmware: qcom: scm: Modify only the download bits in TCSR register

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:02PM +0530, Mukesh Ojha wrote:
> Crashdump collection is done based on DLOAD bits of TCSR register.
> To retain other bits, scm driver need to read the register and
> modify only the DLOAD bits, as other bits in TCSR may have their
> own significance.
> 
> Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 8f766fce5f7c..bd6bfdf2d828 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -4,6 +4,8 @@
>   */
>  
>  #include <linux/arm-smccc.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/cpumask.h>
> @@ -114,6 +116,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>  #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
>  #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
>  
> +#define QCOM_DLOAD_MASK		GENMASK(5, 4)
> +enum qcom_dload_mode {
> +	QCOM_DLOAD_NODUMP	= 0,
> +	QCOM_DLOAD_FULLDUMP	= 1,

These values are not enumerations, they represent fixed/defined values
in the interface. As such it's appropriate to use #define.

Regards,
Bjorn

> +};
> +
>  static const char * const qcom_scm_convention_names[] = {
>  	[SMC_CONVENTION_UNKNOWN] = "unknown",
>  	[SMC_CONVENTION_ARM_32] = "smc arm 32",
> @@ -531,6 +539,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>  
>  static void qcom_scm_set_download_mode(bool enable)
>  {
> +	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
>  	bool avail;
>  	int ret = 0;
>  
> @@ -540,8 +549,9 @@ static void qcom_scm_set_download_mode(bool enable)
>  	if (avail) {
>  		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>  	} else if (__scm->dload_mode_addr) {
> -		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> -				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> +		ret = qcom_scm_io_rmw(__scm->dload_mode_addr,
> +				      QCOM_DLOAD_MASK,
> +				      FIELD_PREP(QCOM_DLOAD_MASK, val));
>  	} else {
>  		dev_err(__scm->dev,
>  			"No available mechanism for setting download mode\n");
> -- 
> 2.43.0.254.ga26002b62827
> 

Re: [PATCH v12 4/9] firmware: qcom: scm: Rework dload mode availability check

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:03PM +0530, Mukesh Ojha wrote:
> QCOM_SCM_BOOT_SET_DLOAD_MODE was only valid for very older
> target and firmware and for recent targets there is dload
> mode tcsr registers available to set the download mode.
> 

I presume this implies that it will always return false, so what's the
actual problem with that? Presumably you want this because it takes
unnecessary time to make that call, if so please say so.


Content of the patch looks good.

Regards,
Bjorn

> So, it is better to keep it as fallback check instead of
> checking its availability and failing it always.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index bd6bfdf2d828..3fd89cddba3b 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -540,18 +540,16 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>  static void qcom_scm_set_download_mode(bool enable)
>  {
>  	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
> -	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_rmw(__scm->dload_mode_addr,
>  				      QCOM_DLOAD_MASK,
>  				      FIELD_PREP(QCOM_DLOAD_MASK, val));
> +	} 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.43.0.254.ga26002b62827
> 

Re: [PATCH v12 6/9] firmware: qcom: scm: Remove log reporting memory allocation failure

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:05PM +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");
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

This seems unrelated to the rest of the series, if you send independent
patches (or patch series) on their own it's easy to just pick them.

Regards,
Bjorn

> ---
>  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 3fd89cddba3b..6c252cddd44e 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -598,10 +598,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.43.0.254.ga26002b62827
> 

Re: [PATCH v12 9/9] firmware: scm: Remove redundant scm argument from qcom_scm_waitq_wakeup()

[Bjorn Andersson]

On Tue, Feb 27, 2024 at 09:23:08PM +0530, Mukesh Ojha wrote:
> Remove unused scm argument from qcom_scm_waitq_wakeup().

Is it unused or redundant? Please pick on word (the right one ;))

Regards,
Bjorn

> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  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 a1dce417e6ec..d91f5872e003 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1853,7 +1853,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;
>  
> @@ -1885,7 +1885,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.43.0.254.ga26002b62827
> 

Re: [PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Bjorn Andersson]

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
> 

Re: [PATCH v12 2/9] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

On 3/3/2024 12:39 AM, Bjorn Andersson wrote:
> On Tue, Feb 27, 2024 at 09:23:01PM +0530, Mukesh Ojha wrote:
>> It is possible that different bits of a secure register is used
>> for different purpose and currently, there is no such available
>> function from SCM driver to do that; one similar usage was pointed
>> by Srinivas K. inside pinctrl-msm where interrupt configuration
>> register lying in secure region and written via read-modify-write
>> operation.
>>
>> Export qcom_scm_io_rmw() to do read-modify-write operation on secure
>> register and reuse it wherever applicable, also document scm_lock
>> to convey its usage.
>>
>> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
>> ---
>>   drivers/firmware/qcom/qcom_scm.c       | 33 ++++++++++++++++++++++++++
>>   include/linux/firmware/qcom/qcom_scm.h |  1 +
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 2d0ba529cf56..8f766fce5f7c 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -193,6 +193,11 @@ static void qcom_scm_bw_disable(void)
>>   }
>>   
>>   enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
>> +/*
>> + * scm_lock to serialize call to query SMC convention and
>> + * to atomically operate(read-modify-write) on different
>> + * bits of secure register.
>> + */
>>   static DEFINE_SPINLOCK(scm_lock);
>>   
>>   static enum qcom_scm_convention __get_convention(void)
>> @@ -481,6 +486,34 @@ static int qcom_scm_disable_sdi(void)
>>   	return ret ? : res.result[0];
>>   }
>>   
>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
>> +{
>> +	unsigned long flags;
>> +	unsigned int old;
>> +	unsigned int new;
>> +	int ret;
>> +
>> +	if (!__scm)
>> +		return -EPROBE_DEFER;
>> +
>> +	/*
>> +	 * Lock to atomically do rmw operation on different bits
>> +	 * of secure register
>> +	 */
> 
> A spinlock does not make something globally atomic, all you have made
> sure is that:
> 1) qcom_scm_io_rmw() can not happen concurrently with __get_convention()
> 
> The reuse of the lock makes me wonder what the use case you're having a
> need to protect #1... When is rmw happening concurrently with convention
> detection?
> 
> 2) Two qcom_scm_io_rmw() can not happen concurrently
> 
> What happens if a separate process invokes qcom_scm_io_write() of the
> same address concurrently with qcom_scm_io_rmw()?

Yes, that is not protected..

> 
> 
> Quite likely neither of these will happen in practice, and I'm guessing
> that there will not be any caching issues etc among different calls to
> qcom_scm_io_*(), and hence this spinlock serves just to complicate the
> implementation.
> 
> Please add a kernel-doc comment to this function (and perhaps
> qcom_scm_io_write()) and describe that we don't guarantee this operation
> to happen atomically - or if you have a valid reason, document that and
> it's exact limitations.

Sure, that make sense !! it is possible for a call to be preempted right
before it reaches to Trust zone(TZ) and it is not being handled 
inherently from SCM driver.

To further add, qcom_scm_io_{read|write}() atomic calls to TZ which
makes sure the does not get interrupted while it is in trust zone by
disabling interrupts and it is other way with non-atomic calls.

> 
> 
> PS. I would have been perfectly happy with us not adding a rmw function.
> You're adding 34 lines of code to save 2*3 lines of duplicated, easy to
> understand, code.

I agree with you..

Global scm spin lock would have only made sense if there could be some
resources to share from secure to non-secure and here, addresses are 
specific to the client driver and lock does need to be taken by the 
client and their address can not get overwritten by other drivers.
So, we already discussed on regmap which does not fit here at scale.

Currently, we have only one place where we have secure rmw() operation
in Qualcomm msm pinctrl driver and that seems to protected 
spin_lock_irqsave(), similarly others can do the same way if there
is a chance of race on the same address.

-Mukesh

> 
> Regards,
> Bjorn
> 
>> +	spin_lock_irqsave(&scm_lock, flags);
>> +	ret = qcom_scm_io_readl(addr, &old);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	new = (old & ~mask) | (val & mask);
>> +
>> +	ret = qcom_scm_io_writel(addr, new);
>> +unlock:
>> +	spin_unlock_irqrestore(&scm_lock, flags);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
>> +
>>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>   {
>>   	struct qcom_scm_desc desc = {
>> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
>> index ccaf28846054..3a8bb2e603b3 100644
>> --- a/include/linux/firmware/qcom/qcom_scm.h
>> +++ b/include/linux/firmware/qcom/qcom_scm.h
>> @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
>>   
>>   int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>>   int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
>>   
>>   bool qcom_scm_restore_sec_cfg_available(void);
>>   int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
>> -- 
>> 2.43.0.254.ga26002b62827
>>

Re: [PATCH v12 3/9] firmware: qcom: scm: Modify only the download bits in TCSR register

[Mukesh Ojha]

On 3/3/2024 12:43 AM, Bjorn Andersson wrote:
> On Tue, Feb 27, 2024 at 09:23:02PM +0530, Mukesh Ojha wrote:
>> Crashdump collection is done based on DLOAD bits of TCSR register.
>> To retain other bits, scm driver need to read the register and
>> modify only the DLOAD bits, as other bits in TCSR may have their
>> own significance.
>>
>> Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   drivers/firmware/qcom/qcom_scm.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 8f766fce5f7c..bd6bfdf2d828 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -4,6 +4,8 @@
>>    */
>>   
>>   #include <linux/arm-smccc.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>>   #include <linux/clk.h>
>>   #include <linux/completion.h>
>>   #include <linux/cpumask.h>
>> @@ -114,6 +116,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>>   #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
>>   #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
>>   
>> +#define QCOM_DLOAD_MASK		GENMASK(5, 4)
>> +enum qcom_dload_mode {
>> +	QCOM_DLOAD_NODUMP	= 0,
>> +	QCOM_DLOAD_FULLDUMP	= 1,
> 
> These values are not enumerations, they represent fixed/defined values
> in the interface. As such it's appropriate to use #define.
> 

Thanks for giving reasoning on why it should be #define and not enum.

-Mukesh

Re: [PATCH v12 4/9] firmware: qcom: scm: Rework dload mode availability check

[Mukesh Ojha]

On 3/3/2024 12:46 AM, Bjorn Andersson wrote:
> On Tue, Feb 27, 2024 at 09:23:03PM +0530, Mukesh Ojha wrote:
>> QCOM_SCM_BOOT_SET_DLOAD_MODE was only valid for very older
>> target and firmware and for recent targets there is dload
>> mode tcsr registers available to set the download mode.
>>
> 
> I presume this implies that it will always return false, so what's the
> actual problem with that? Presumably you want this because it takes
> unnecessary time to make that call, if so please say so.

Correct, will add in commit description.

Also, __qcom_scm_is_call_available() usage legacy mode of setting DLOAD 
mode setting via command which is deprecated long back from Trust zone 
firmware also it takes or wait statically global mutex lock at the lower 
level which unnecessary adds time which is of no use.
> 
> 
> Content of the patch looks good.

Thanks.

-Mukesh

> 
> Regards,
> Bjorn
> 
>> So, it is better to keep it as fallback check instead of
>> checking its availability and failing it always.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   drivers/firmware/qcom/qcom_scm.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index bd6bfdf2d828..3fd89cddba3b 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -540,18 +540,16 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>   static void qcom_scm_set_download_mode(bool enable)
>>   {
>>   	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
>> -	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_rmw(__scm->dload_mode_addr,
>>   				      QCOM_DLOAD_MASK,
>>   				      FIELD_PREP(QCOM_DLOAD_MASK, val));
>> +	} 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.43.0.254.ga26002b62827
>>

Re: [PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Mukesh Ojha]

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

Re: [PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Pavan Kondeti]

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

Re: [PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Mukesh Ojha]

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) {
-- 
2.7.4



> 
> Thanks,
> Pavan

Re: [PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Pavan Kondeti]

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

Re: [PATCH v12 7/9] firmware: qcom: scm: Fix __scm->dev assignement

[Mukesh Ojha]

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