firmware: qcom: scm: Allow devicetree-less probe

Commit Message

Elliot Berman Sept. 20, 2024, 6:01 p.m. UTC

Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
missing the SCM node. Users of the SCM device assume the device is
present and the driver also assumes it has probed. This can lead to
unanticipated crashes when there isn't an SCM device. All Qualcomm
Technologies, Inc. SoCs use SCM to communicate with firmware, so create
the platform device if it's not present in the devicetree.

Tested that SCM node still probes on:
 - sm8650-qrd with the SCM DT node still present
 - sm845-mtp with the SCM DT node still present
 - sm845-mtp with the node removed

Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
Reported-by: Rudraksha Gupta <[e-mail]>
Closes: https://lore.kernel.org/lkml/[e-mail]/
Link: https://lore.kernel.org/all/CAA8EJpqSKbKJ=[e-mail]/
Suggested-by: Bartosz Golaszewski <[e-mail]>
Signed-off-by: Elliot Berman <[e-mail]>
---
 drivers/firmware/qcom/qcom_scm.c | 75 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 9 deletions(-)


---
base-commit: 2adcf3941db724e1750da7094c34431d9b6b7fcb
change-id: 20240917-scm-pdev-bc8db85fad05

Best regards,

Comments

Wasim Nazir Sept. 20, 2024, 6:41 p.m. UTC | #1

On 9/20/2024 11:31 PM, Elliot Berman wrote:
> Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
> missing the SCM node. Users of the SCM device assume the device is
> present and the driver also assumes it has probed. This can lead to
> unanticipated crashes when there isn't an SCM device. All Qualcomm
> Technologies, Inc. SoCs use SCM to communicate with firmware, so create
> the platform device if it's not present in the devicetree.
> 
> Tested that SCM node still probes on:
>   - sm8650-qrd with the SCM DT node still present
>   - sm845-mtp with the SCM DT node still present
>   - sm845-mtp with the node removed
> 
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <[e-mail]>
> Closes: https://lore.kernel.org/lkml/[e-mail]/
> Link: https://lore.kernel.org/all/CAA8EJpqSKbKJ=[e-mail]/
> Suggested-by: Bartosz Golaszewski <[e-mail]>
> Signed-off-by: Elliot Berman <[e-mail]>
> ---
>   drivers/firmware/qcom/qcom_scm.c | 75 +++++++++++++++++++++++++++++++++++-----
>   1 file changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..842ba490cd37 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1954,10 +1954,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	init_completion(&scm->waitq_comp);
>   	mutex_init(&scm->scm_bw_lock);
>   
> -	scm->path = devm_of_icc_get(&pdev->dev, NULL);
> -	if (IS_ERR(scm->path))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(scm->path),
> -				     "failed to acquire interconnect path\n");
> +	if (pdev->dev.of_node) {
> +		scm->path = devm_of_icc_get(&pdev->dev, NULL);
> +		if (IS_ERR(scm->path))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(scm->path),
> +					"failed to acquire interconnect path\n");
> +	}
>   
>   	scm->core_clk = devm_clk_get_optional(&pdev->dev, "core");
>   	if (IS_ERR(scm->core_clk))
> @@ -2012,10 +2014,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>   	if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled") || !download_mode)
>   		qcom_scm_disable_sdi();
>   
> -	ret = of_reserved_mem_device_init(__scm->dev);
> -	if (ret && ret != -ENODEV)
> -		return dev_err_probe(__scm->dev, ret,
> -				     "Failed to setup the reserved memory region for TZ mem\n");
> +	if (pdev->dev.of_node) {
> +		ret = of_reserved_mem_device_init(__scm->dev);
> +		if (ret && ret != -ENODEV)
> +			return dev_err_probe(__scm->dev, ret,
> +					"Failed to setup the reserved memory region for TZ mem\n");
> +	}
>   
>   	ret = qcom_tzmem_enable(__scm->dev);
>   	if (ret)
> @@ -2068,6 +2072,11 @@ static const struct of_device_id qcom_scm_dt_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
>   
> +static const struct platform_device_id qcom_scm_id_table[] = {
> +	{ .name = "qcom-scm" },
> +	{}
> +};
> +
>   static struct platform_driver qcom_scm_driver = {
>   	.driver = {
>   		.name	= "qcom_scm",
> @@ -2076,11 +2085,59 @@ static struct platform_driver qcom_scm_driver = {
>   	},
>   	.probe = qcom_scm_probe,
>   	.shutdown = qcom_scm_shutdown,
> +	.id_table = qcom_scm_id_table,
>   };
>   
> +static bool is_qcom_machine(void)
> +{
> +	struct device_node *np __free(device_node) = NULL;
> +	struct property *prop;
> +	const char *name;
> +
> +	np = of_find_node_by_path("/");
> +	if (!np)
> +		return false;
> +
> +	of_property_for_each_string(np, "compatible", prop, name)
> +		if (!strncmp("qcom,", name, 5))
Is this limitation updated in dt-schema also? This static check in code 
might cause unwanted issues.

Instead can we use this simple check method? I am ok to do some 
refinement if needed.
https://lore.kernel.org/all/[e-mail]/
> +			return true;
> +
> +	return false;
> +}
> +
>   static int __init qcom_scm_init(void)
>   {
> -	return platform_driver_register(&qcom_scm_driver);
> +	struct device_node *np __free(device_node) = NULL;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	ret = platform_driver_register(&qcom_scm_driver);
> +	if (ret)
> +		return ret;
> +
> +	/* Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
> +	 * missing the SCM node. Find out if we don't have a SCM node *and*
> +	 * we are a Qualcomm-compatible SoC. If yes, then create a platform
> +	 * device for the SCM driver. Assume scanning the root compatible for
> +	 * "qcom," vendor prefix will be faster than searching for the
> +	 * SCM DT node.
> +	 */
> +	if (!is_qcom_machine())
> +		return 0;
> +
> +	np = of_find_matching_node_and_match(NULL, qcom_scm_dt_match, NULL);
> +	if (np)
> +		return 0;
> +
> +	pdev = platform_device_alloc(qcom_scm_id_table[0].name, PLATFORM_DEVID_NONE);
> +	if (!pdev)
> +		return -ENOMEM;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		platform_device_put(pdev);
> +
> +	return ret;
>   }
>   subsys_initcall(qcom_scm_init);
>   
> 
> ---
> base-commit: 2adcf3941db724e1750da7094c34431d9b6b7fcb
> change-id: 20240917-scm-pdev-bc8db85fad05
> 
> Best regards,

Elliot Berman Sept. 20, 2024, 7:09 p.m. UTC | #2

On Sat, Sep 21, 2024 at 12:11:39AM +0530, Wasim Nazir wrote:
> 
> 
> On 9/20/2024 11:31 PM, Elliot Berman wrote:
> > Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
> > missing the SCM node. Users of the SCM device assume the device is
> > present and the driver also assumes it has probed. This can lead to
> > unanticipated crashes when there isn't an SCM device. All Qualcomm
> > Technologies, Inc. SoCs use SCM to communicate with firmware, so create
> > the platform device if it's not present in the devicetree.
> > 
> > Tested that SCM node still probes on:
> >   - sm8650-qrd with the SCM DT node still present
> >   - sm845-mtp with the SCM DT node still present
> >   - sm845-mtp with the node removed
> > 
> > Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> > Reported-by: Rudraksha Gupta <[e-mail]>
> > Closes: https://lore.kernel.org/lkml/[e-mail]/
> > Link: https://lore.kernel.org/all/CAA8EJpqSKbKJ=[e-mail]/
> > Suggested-by: Bartosz Golaszewski <[e-mail]>
> > Signed-off-by: Elliot Berman <[e-mail]>
> > ---
> >   drivers/firmware/qcom/qcom_scm.c | 75 +++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 66 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 10986cb11ec0..842ba490cd37 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -1954,10 +1954,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >   	init_completion(&scm->waitq_comp);
> >   	mutex_init(&scm->scm_bw_lock);
> > -	scm->path = devm_of_icc_get(&pdev->dev, NULL);
> > -	if (IS_ERR(scm->path))
> > -		return dev_err_probe(&pdev->dev, PTR_ERR(scm->path),
> > -				     "failed to acquire interconnect path\n");
> > +	if (pdev->dev.of_node) {
> > +		scm->path = devm_of_icc_get(&pdev->dev, NULL);
> > +		if (IS_ERR(scm->path))
> > +			return dev_err_probe(&pdev->dev, PTR_ERR(scm->path),
> > +					"failed to acquire interconnect path\n");
> > +	}
> >   	scm->core_clk = devm_clk_get_optional(&pdev->dev, "core");
> >   	if (IS_ERR(scm->core_clk))
> > @@ -2012,10 +2014,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
> >   	if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled") || !download_mode)
> >   		qcom_scm_disable_sdi();
> > -	ret = of_reserved_mem_device_init(__scm->dev);
> > -	if (ret && ret != -ENODEV)
> > -		return dev_err_probe(__scm->dev, ret,
> > -				     "Failed to setup the reserved memory region for TZ mem\n");
> > +	if (pdev->dev.of_node) {
> > +		ret = of_reserved_mem_device_init(__scm->dev);
> > +		if (ret && ret != -ENODEV)
> > +			return dev_err_probe(__scm->dev, ret,
> > +					"Failed to setup the reserved memory region for TZ mem\n");
> > +	}
> >   	ret = qcom_tzmem_enable(__scm->dev);
> >   	if (ret)
> > @@ -2068,6 +2072,11 @@ static const struct of_device_id qcom_scm_dt_match[] = {
> >   };
> >   MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
> > +static const struct platform_device_id qcom_scm_id_table[] = {
> > +	{ .name = "qcom-scm" },
> > +	{}
> > +};
> > +
> >   static struct platform_driver qcom_scm_driver = {
> >   	.driver = {
> >   		.name	= "qcom_scm",
> > @@ -2076,11 +2085,59 @@ static struct platform_driver qcom_scm_driver = {
> >   	},
> >   	.probe = qcom_scm_probe,
> >   	.shutdown = qcom_scm_shutdown,
> > +	.id_table = qcom_scm_id_table,
> >   };
> > +static bool is_qcom_machine(void)
> > +{
> > +	struct device_node *np __free(device_node) = NULL;
> > +	struct property *prop;
> > +	const char *name;
> > +
> > +	np = of_find_node_by_path("/");
> > +	if (!np)
> > +		return false;
> > +
> > +	of_property_for_each_string(np, "compatible", prop, name)
> > +		if (!strncmp("qcom,", name, 5))
> Is this limitation updated in dt-schema also? This static check in code
> might cause unwanted issues.

arm/qcom.yaml describes the requirement that Qualcomm SoCs would always
have a "qcom," prefixed compatible.

> 
> Instead can we use this simple check method? I am ok to do some refinement
> if needed.
> https://lore.kernel.org/all/[e-mail]/

We'd like to make sure that SCM device is instead probed in time.
Returning error isn't preferred as most (all?) users won't retry
later.

> > +			return true;
> > +
> > +	return false;
> > +}
> > +
> >   static int __init qcom_scm_init(void)
> >   {
> > -	return platform_driver_register(&qcom_scm_driver);
> > +	struct device_node *np __free(device_node) = NULL;
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	ret = platform_driver_register(&qcom_scm_driver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
> > +	 * missing the SCM node. Find out if we don't have a SCM node *and*
> > +	 * we are a Qualcomm-compatible SoC. If yes, then create a platform
> > +	 * device for the SCM driver. Assume scanning the root compatible for
> > +	 * "qcom," vendor prefix will be faster than searching for the
> > +	 * SCM DT node.
> > +	 */
> > +	if (!is_qcom_machine())
> > +		return 0;
> > +
> > +	np = of_find_matching_node_and_match(NULL, qcom_scm_dt_match, NULL);
> > +	if (np)
> > +		return 0;
> > +
> > +	pdev = platform_device_alloc(qcom_scm_id_table[0].name, PLATFORM_DEVID_NONE);
> > +	if (!pdev)
> > +		return -ENOMEM;
> > +
> > +	ret = platform_device_add(pdev);
> > +	if (ret)
> > +		platform_device_put(pdev);
> > +
> > +	return ret;
> >   }
> >   subsys_initcall(qcom_scm_init);
> > 
> > ---
> > base-commit: 2adcf3941db724e1750da7094c34431d9b6b7fcb
> > change-id: 20240917-scm-pdev-bc8db85fad05
> > 
> > Best regards,

Stephan Gerhold Sept. 20, 2024, 7:45 p.m. UTC | #3

On Fri, Sep 20, 2024 at 11:01:40AM -0700, Elliot Berman wrote:
> Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
> missing the SCM node. Users of the SCM device assume the device is
> present and the driver also assumes it has probed. This can lead to
> unanticipated crashes when there isn't an SCM device. All Qualcomm
> Technologies, Inc. SoCs use SCM to communicate with firmware, so create
> the platform device if it's not present in the devicetree.
> 
> Tested that SCM node still probes on:
>  - sm8650-qrd with the SCM DT node still present
>  - sm845-mtp with the SCM DT node still present
>  - sm845-mtp with the node removed
> 
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <[e-mail]>
> Closes: https://lore.kernel.org/lkml/[e-mail]/
> Link: https://lore.kernel.org/all/CAA8EJpqSKbKJ=[e-mail]/
> Suggested-by: Bartosz Golaszewski <[e-mail]>
> Signed-off-by: Elliot Berman <[e-mail]>

Do we actually need this patch?

We have a simple patch already that fixes the reported regression [1].

And as I explained in my reply to that series [2], the root cause is not
the lack of /scm node in the DT, but the time when the SCM call is made
during the kernel boot process. qcom_scm_set_cold_boot_addr() is called
in arch/arm/mach-qcom/platsmp.c before any drivers bind to devices in
the DT. We would need an early_initcall() to run early enough before
initializing SMP, but I haven't found any examples that the
device/driver model is actually functional at that point.

I think applying the simple one line fix from Bartosz [1] should be
sufficient to restore all functionality that worked before the SCM
allocator changes.

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/
[2]: https://lore.kernel.org/linux-arm-msm/[e-mail]/

Elliot Berman Sept. 20, 2024, 8:23 p.m. UTC | #4

On Fri, Sep 20, 2024 at 09:45:37PM +0200, Stephan Gerhold wrote:
> On Fri, Sep 20, 2024 at 11:01:40AM -0700, Elliot Berman wrote:
> > Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
> > missing the SCM node. Users of the SCM device assume the device is
> > present and the driver also assumes it has probed. This can lead to
> > unanticipated crashes when there isn't an SCM device. All Qualcomm
> > Technologies, Inc. SoCs use SCM to communicate with firmware, so create
> > the platform device if it's not present in the devicetree.
> > 
> > Tested that SCM node still probes on:
> >  - sm8650-qrd with the SCM DT node still present
> >  - sm845-mtp with the SCM DT node still present
> >  - sm845-mtp with the node removed
> > 
> > Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> > Reported-by: Rudraksha Gupta <[e-mail]>
> > Closes: https://lore.kernel.org/lkml/[e-mail]/
> > Link: https://lore.kernel.org/all/CAA8EJpqSKbKJ=[e-mail]/
> > Suggested-by: Bartosz Golaszewski <[e-mail]>
> > Signed-off-by: Elliot Berman <[e-mail]>
> 
> Do we actually need this patch?
> 
> We have a simple patch already that fixes the reported regression [1].
> 
> And as I explained in my reply to that series [2], the root cause is not
> the lack of /scm node in the DT, but the time when the SCM call is made
> during the kernel boot process. qcom_scm_set_cold_boot_addr() is called
> in arch/arm/mach-qcom/platsmp.c before any drivers bind to devices in
> the DT. We would need an early_initcall() to run early enough before
> initializing SMP, but I haven't found any examples that the
> device/driver model is actually functional at that point.
> 
> I think applying the simple one line fix from Bartosz [1] should be
> sufficient to restore all functionality that worked before the SCM
> allocator changes.

Ah, I had missed seeing [2]! I am happy with Bartosz's existing patch. I
had thought the discussion in [3] was still relevant.

Thanks,
Elliot

> [1]: https://lore.kernel.org/linux-arm-msm/[e-mail]/
> [2]: https://lore.kernel.org/linux-arm-msm/[e-mail]/

[3]: https://lore.kernel.org/all/CAA8EJpqSKbKJ=[e-mail]/

Patch

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 10986cb11ec0..842ba490cd37 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1954,10 +1954,12 @@  static int qcom_scm_probe(struct platform_device *pdev)
 	init_completion(&scm->waitq_comp);
 	mutex_init(&scm->scm_bw_lock);
 
-	scm->path = devm_of_icc_get(&pdev->dev, NULL);
-	if (IS_ERR(scm->path))
-		return dev_err_probe(&pdev->dev, PTR_ERR(scm->path),
-				     "failed to acquire interconnect path\n");
+	if (pdev->dev.of_node) {
+		scm->path = devm_of_icc_get(&pdev->dev, NULL);
+		if (IS_ERR(scm->path))
+			return dev_err_probe(&pdev->dev, PTR_ERR(scm->path),
+					"failed to acquire interconnect path\n");
+	}
 
 	scm->core_clk = devm_clk_get_optional(&pdev->dev, "core");
 	if (IS_ERR(scm->core_clk))
@@ -2012,10 +2014,12 @@  static int qcom_scm_probe(struct platform_device *pdev)
 	if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled") || !download_mode)
 		qcom_scm_disable_sdi();
 
-	ret = of_reserved_mem_device_init(__scm->dev);
-	if (ret && ret != -ENODEV)
-		return dev_err_probe(__scm->dev, ret,
-				     "Failed to setup the reserved memory region for TZ mem\n");
+	if (pdev->dev.of_node) {
+		ret = of_reserved_mem_device_init(__scm->dev);
+		if (ret && ret != -ENODEV)
+			return dev_err_probe(__scm->dev, ret,
+					"Failed to setup the reserved memory region for TZ mem\n");
+	}
 
 	ret = qcom_tzmem_enable(__scm->dev);
 	if (ret)
@@ -2068,6 +2072,11 @@  static const struct of_device_id qcom_scm_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
 
+static const struct platform_device_id qcom_scm_id_table[] = {
+	{ .name = "qcom-scm" },
+	{}
+};
+
 static struct platform_driver qcom_scm_driver = {
 	.driver = {
 		.name	= "qcom_scm",
@@ -2076,11 +2085,59 @@  static struct platform_driver qcom_scm_driver = {
 	},
 	.probe = qcom_scm_probe,
 	.shutdown = qcom_scm_shutdown,
+	.id_table = qcom_scm_id_table,
 };
 
+static bool is_qcom_machine(void)
+{
+	struct device_node *np __free(device_node) = NULL;
+	struct property *prop;
+	const char *name;
+
+	np = of_find_node_by_path("/");
+	if (!np)
+		return false;
+
+	of_property_for_each_string(np, "compatible", prop, name)
+		if (!strncmp("qcom,", name, 5))
+			return true;
+
+	return false;
+}
+
 static int __init qcom_scm_init(void)
 {
-	return platform_driver_register(&qcom_scm_driver);
+	struct device_node *np __free(device_node) = NULL;
+	struct platform_device *pdev;
+	int ret;
+
+	ret = platform_driver_register(&qcom_scm_driver);
+	if (ret)
+		return ret;
+
+	/* Some devicetrees representing Qualcomm Technologies, Inc. SoCs are
+	 * missing the SCM node. Find out if we don't have a SCM node *and*
+	 * we are a Qualcomm-compatible SoC. If yes, then create a platform
+	 * device for the SCM driver. Assume scanning the root compatible for
+	 * "qcom," vendor prefix will be faster than searching for the
+	 * SCM DT node.
+	 */
+	if (!is_qcom_machine())
+		return 0;
+
+	np = of_find_matching_node_and_match(NULL, qcom_scm_dt_match, NULL);
+	if (np)
+		return 0;
+
+	pdev = platform_device_alloc(qcom_scm_id_table[0].name, PLATFORM_DEVID_NONE);
+	if (!pdev)
+		return -ENOMEM;
+
+	ret = platform_device_add(pdev);
+	if (ret)
+		platform_device_put(pdev);
+
+	return ret;
 }
 subsys_initcall(qcom_scm_init);