Re: [PATCH] firmware: qcom: scm: Allow devicetree-less probe

[Wasim Nazir <quic_wasimn@quicinc.com>]

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 <guptarud@gmail.com>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/
> Link: https://lore.kernel.org/all/CAA8EJpqSKbKJ=y0LAigGdj7_uk+5mezDgnzV5XEzwbxRJgpN1w@mail.gmail.com/
> Suggested-by: Bartosz Golaszewski <brgl@bgdev.pl>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   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/20240920181317.391918-1-quic_wasimn@quicinc.com/
> +			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,