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