Re: [PATCH v7 5/6] remoteproc: qcom: pas: Add late attach support for subsystems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Returning `-EINVAL` from `attach()` to trigger a fallback causes `rproc_add()` to fail and aborts the driver probe entirely.
- [High] Calling `rproc_report_crash()` before returning an error from `attach()` causes a deterministic Use-After-Free of the `rproc` object.
- [Medium] `qcom_sysmon_shutdown_irq_state()` passes a potentially negative IRQ number to `irq_get_irqchip_state()`, causing spurious warnings.
--

commit 2eb49833f76c6090fc2bf0848fb2114975777ac0
Author: Jingyi Wang <jingyi.wang@oss.qualcomm.com>

remoteproc: qcom: pas: Add late attach support for subsystems

This patch introduces late attach support for subsystems that are brought
out of reset by bootloaders. It adds a qcom_pas_attach() function to check
the subsystem state via SMP2P bits, allowing the driver to attach, report a
crash, or fall back to an offline state if the subsystem is not ready.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 808e9609988d3..8a0bb4b2e71c0 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -507,6 +508,67 @@ static unsigned long qcom_pas_panic(struct rproc *rproc)
>  	return qcom_q6v5_panic(&pas->q6v5);
>  }
>  
> +static int qcom_pas_attach(struct rproc *rproc)
> +{
> +	struct qcom_pas *pas = rproc->priv;
> +	bool ready_state;
> +	bool crash_state;
> +	bool stop_state;
> +	int ret;
> +
> +	pas->q6v5.handover_issued = true;
> +	enable_irq(pas->q6v5.handover_irq);
> +
> +	pas->q6v5.running = true;
> +	ret = irq_get_irqchip_state(pas->q6v5.fatal_irq,
> +				    IRQCHIP_STATE_LINE_LEVEL, &crash_state);
> +	if (ret)
> +		goto disable_running;
> +
> +	if (crash_state) {
> +		dev_err(pas->dev, "Subsystem has crashed before driver probe\n");
> +		rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> +		ret = -EINVAL;
> +		goto disable_running;

[Severity: High]
Can this sequence cause a use-after-free of the rproc object?

If crash_state is true, rproc_report_crash() asynchronously queues
&rproc->crash_handler onto the rproc_recovery_wq workqueue. This function
then immediately returns -EINVAL.

Because .attach() runs synchronously during driver probe via rproc_add(),
the returned error causes rproc_add() to fail, and the devres framework will
automatically free the rproc structure. 

The remoteproc core does not cancel the pending crash work during device
teardown, so when the workqueue later executes rproc_crash_handler_work(),
will it dereference the freed rproc memory?

> +	}
> +
> +	ret = irq_get_irqchip_state(pas->q6v5.stop_irq,
> +				    IRQCHIP_STATE_LINE_LEVEL, &stop_state);
> +	if (ret)
> +		goto disable_running;
> +
> +	if (stop_state || qcom_sysmon_shutdown_irq_state(pas->sysmon)) {
> +		dev_info(pas->dev, "Subsystem found stop state set. Falling back to start.\n");
> +		goto unroll_attach;
> +	}
> +
> +	ret = irq_get_irqchip_state(pas->q6v5.ready_irq,
> +				    IRQCHIP_STATE_LINE_LEVEL, &ready_state);
> +	if (ret)
> +		goto disable_running;
> +
> +	if (unlikely(!ready_state)) {
> +		/*
> +		 * The bootloader may not support early boot, mark the state as
> +		 * RPROC_OFFLINE so that the PAS driver can load the firmware and
> +		 * start the remoteproc.
> +		 */
> +		dev_err(pas->dev, "Failed to get subsystem ready interrupt\n");
> +		goto unroll_attach;
> +	}
> +
> +	return 0;
> +
> +unroll_attach:
> +	pas->rproc->state = RPROC_OFFLINE;
> +	ret = -EINVAL;

[Severity: High]
Does returning an error code here break the intended fallback mechanism?

If desc->auto_boot and desc->early_boot are true, rproc_add() will
synchronously call rproc_trigger_auto_boot(), which calls qcom_pas_attach().

If this function returns -EINVAL, the error propagates back to rproc_add(),
which interprets it as a fatal initialization failure, aborting the driver
probe entirely instead of enabling a normal firmware load:

drivers/remoteproc/remoteproc_core.c:rproc_add() {
    ...
	if (rproc->auto_boot) {
		ret = rproc_trigger_auto_boot(rproc);
		if (ret < 0)
			goto rproc_remove_dev;
	}
    ...
}

> +disable_running:
> +	disable_irq(pas->q6v5.handover_irq);
> +	pas->q6v5.running = false;
> +
> +	return ret;
> +}
[ ... ]
> diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
> index 913e3b750a869..a0830a48b1f40 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c
[ ... ]
> @@ -736,6 +736,25 @@ bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon)
>  }
>  EXPORT_SYMBOL_GPL(qcom_sysmon_shutdown_acked);
>  
> +bool qcom_sysmon_shutdown_irq_state(struct qcom_sysmon *sysmon)
> +{
> +	bool shutdown_state;
> +	int ret;
> +
> +	if (!sysmon)
> +		return false;
> +
> +	ret = irq_get_irqchip_state(sysmon->shutdown_irq,
> +				    IRQCHIP_STATE_LINE_LEVEL, &shutdown_state);

[Severity: Medium]
Will this cause spurious warnings if the optional shutdown IRQ is missing?

If the optional shutdown-ack interrupt is omitted from the device tree,
of_irq_get_byname() returns -ENODATA, which is stored in sysmon->shutdown_irq
but safely ignored during initialization.

Later, this function passes sysmon->shutdown_irq directly to
irq_get_irqchip_state(), which expects an unsigned int. The IRQ subsystem
will fail to find the corresponding IRQ descriptor for the invalid number and
return -EINVAL, causing this to incorrectly log a spurious warning.

> +	if (ret) {
> +		dev_warn(sysmon->dev, "failed to get shutdown_state: %d\n", ret);
> +		return false;
> +	}
> +
> +	return shutdown_state;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-knp-soccp-v7-0-1ec7bb5c9fec@oss.qualcomm.com?part=5