On Fri, Sep 20, 2024 at 11:43:17PM GMT, Wasim Nazir wrote:
> Avoid NULL pointer dereference while using any qcom SCM calls.
> Add macro to easy the check at each SCM calls.
>
We already have a way to deal with this in the general case. Client
drivers should call qcom_scm_is_available() before calling the scm
interface.
Unfortunately your commit message makes it impossible to know if you're
referring to a case where this wasn't done, or isn't possible, or if
you've hit a bug.
> Changes in v2:
> - Cleanup in commit-message
This goes below the '---', by the diffstat. I don't know why you don't
have a diffstat, please figure out how to make your patches looks like
everyone else's.
>
> Signed-off-by: Wasim Nazir <quic_wasimn@xxxxxxxxxxx>
>
> diff --git a/drivers/firmware/qcom/qcom_scm-legacy.c b/drivers/firmware/qcom/qcom_scm-legacy.c
> index 029e6d117cb8..3247145a6583 100644
> --- a/drivers/firmware/qcom/qcom_scm-legacy.c
> +++ b/drivers/firmware/qcom/qcom_scm-legacy.c
> @@ -148,6 +148,9 @@ int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
> __le32 *arg_buf;
> const __le32 *res_buf;
>
> + if (!dev)
> + return -EPROBE_DEFER;
-EPROBE_DEFER only makes sense to the caller during probe. In all other
cases this is an invalid error value.
> +
> cmd = kzalloc(PAGE_ALIGN(alloc_len), GFP_KERNEL);
> if (!cmd)
> return -ENOMEM;
[..]
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
[..]
> @@ -387,7 +397,7 @@ static int qcom_scm_set_boot_addr(void *entry, const u8 *cpu_bits)
> desc.args[0] = flags;
> desc.args[1] = virt_to_phys(entry);
>
> - return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
> + return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
I don't think you understand why this is written the way it is.
> }
>
[..]
> @@ -1986,6 +2113,13 @@ static int qcom_scm_probe(struct platform_device *pdev)
> /* Let all above stores be available after this */
> smp_store_release(&__scm, scm);
>
> + __scm->reset.ops = &qcom_scm_pas_reset_ops;
> + __scm->reset.nr_resets = 1;
> + __scm->reset.of_node = pdev->dev.of_node;
> + ret = devm_reset_controller_register(&pdev->dev, &__scm->reset);
> + if (ret)
> + return ret;
> +
Why did this move?
Regards,
Bjorn
> irq = platform_get_irq_optional(pdev, 0);
> if (irq < 0) {
> if (irq != -ENXIO)
> --
> 2.46.1
>
