Re: [PATCH v2 2/2] soundwire: qcom: add EE-aware register layout for Shikra

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] IRQ handler remains active after subsystem unregistration (Use-After-Free risk).
- [Medium] Missing clock cleanup on error path in probe.
- [Medium] Missing pm_runtime_disable() in remove.
--

commit ea7f212648855ca1f1b27ca9b52687d659dc81f7
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

soundwire: qcom: add EE-aware register layout for Shikra

On SoundWire v2.0 and later hardware, FIFO, and status
registers are banked per Execution Environment (EE).
The existing driver assumes the SoundWire controller is
assigned to EE1, which is true for all currently supported
Qualcomm SoCs.

Shikra is an exception and assigns the controller to EE0.
As a result, the relevant register windows are located at
different offsets and cannot be accessed using the default
EE1 register layout.

Add an ee field to the SoC match data and introduce a
dedicated qcom,shikra-soundwire compatible with ee = 0.
The driver uses this information at probe time to select
the correct register layout, while all existing platforms
continue to use the default EE1 configuration.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 35ffffd541bd1..6e0c5fb10efad 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c

[ ... ]

> @@ -1659,6 +1711,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	prop->default_row = data->default_rows;
>  
>  	ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &ctrl->version);
> +	qcom_swrm_set_ee_register_layout(ctrl, data);
>  
>  	ret = devm_request_threaded_irq(dev, ctrl->irq, NULL,
>  					qcom_swrm_irq_handler,

[Severity: Medium]
This isn't a bug introduced by this patch, but looking at the error handling
further down in qcom_swrm_probe():

	ctrl->wake_irq = of_irq_get(dev->of_node, 1);
	if (ctrl->wake_irq > 0) {
		ret = devm_request_threaded_irq(dev, ctrl->wake_irq, NULL,
						qcom_swrm_wake_irq_handler,
						IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
						"swr_wake_irq", ctrl);
		if (ret) {
			dev_err(dev, "Failed to request soundwire wake irq\n");
			goto err_init;
		}
	}

If the devm_request_threaded_irq() for wake_irq fails, the code jumps directly
to the err_init label. Does this bypass the err_clk label, leaking the clock
reference and keeping it prepared and enabled unexpectedly?

[ ... ]

> @@ -1769,16 +1822,19 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
>  		reset_control_reset(ctrl->audio_cgcr);

[Severity: High]
This is also a pre-existing issue, but I have a question about the teardown
sequence in qcom_swrm_remove() which sits just above this function:

static void qcom_swrm_remove(struct platform_device *pdev)
{
	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);

	sdw_bus_master_delete(&ctrl->bus);
	clk_disable_unprepare(ctrl->hclk);
}

During driver unbind or module unload, this function tears down the bus and
slave devices. However, the driver uses a devm-managed IRQ handler which
remains active until devres release (after remove returns).

If a hardware interrupt fires during this window, could qcom_swrm_irq_handler()
execute and operate on the already destroyed bus and slave structures, leading
to a use-after-free or null pointer dereference?

[Severity: Medium]
This is another pre-existing issue in qcom_swrm_remove():

The qcom_swrm_probe() function sets up PM runtime:

	pm_runtime_set_autosuspend_delay(dev, 3000);
	pm_runtime_use_autosuspend(dev);
	pm_runtime_mark_last_busy(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

Should qcom_swrm_remove() call pm_runtime_disable() and related cleanup
functions like pm_runtime_dont_use_autosuspend() to balance this PM setup and
avoid leaving the device in an unbalanced state upon driver reload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-shikra_soundwire_support-v2-0-f98c9a6ea1e1@oss.qualcomm.com?part=2