Re: [PATCH v1 2/2] soundwire: qcom: add EE-aware register layout and cpu selection

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race on `ctrl->intr_mask` between threaded interrupt handler and PM runtime callbacks.
- [High] Missing NULL pointer validation for `data` from `of_device_get_match_data()`, leading to a NULL pointer dereference crash.
--

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

soundwire: qcom: add EE-aware register layout and cpu selection

This commit adds support for selecting the SoundWire execution environment
from the device tree and adjusts register windows based on the EE value.
For v2.0+ hardware, the IRQ, FIFO, and status register layout is offset by
the EE window stride.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 3d8f5a81eff19..f4b8ff77bc2f8 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c

[ ... ]

> @@ -1545,7 +1578,22 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	data = of_device_get_match_data(dev);
> +	ctrl->ee = SWRM_EE_CPU;
> +	ret = of_property_read_u32(dev->of_node, "qcom,swr-master-ee-val", &ctrl->ee);
> +	if (ret)
> +		ret = of_property_read_u32(dev->of_node, "qcom,ee", &ctrl->ee);
> +	if (ret)
> +		ctrl->ee = SWRM_EE_CPU;
> +	if (ctrl->ee > SWRM_MAX_EE) {
> +		dev_warn(dev, "invalid SoundWire EE %u, using EE%u\n",
> +			 ctrl->ee, SWRM_EE_CPU);
> +		ctrl->ee = SWRM_EE_CPU;
> +	}
>  	ctrl->max_reg = data->max_reg;

[Severity: High]
This isn't a bug introduced by this patch, but could this dereference of
data->max_reg cause a NULL pointer crash?

If the device is bound to the driver manually via sysfs (e.g., by writing to
driver_override), the platform bus falls back to matching by name rather than
the device tree.

In this case, dev->of_node is missing, and of_device_get_match_data() returns
NULL.

Does this data pointer need to be validated before being dereferenced?

> +	/*
> +	 * Defer EE register window selection until HW version is known.
> +	 * For v2.0+ the IRQ/FIFO window is EE-banked.
> +	 */
>  	ctrl->reg_layout = data->reg_layout;

[ ... ]

> @@ -1733,16 +1782,19 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
>  		reset_control_reset(ctrl->audio_cgcr);
>  
>  		if (ctrl->version == SWRM_VERSION_1_7_0) {
> -			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, SWRM_EE_CPU);
> +			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, ctrl->ee);
>  			ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL,
> -					SWRM_MCP_BUS_CLK_START << SWRM_EE_CPU);
> +					SWRM_MCP_BUS_CLK_START << ctrl->ee);
>  		} else if (ctrl->version >= SWRM_VERSION_2_0_0) {
> -			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, SWRM_EE_CPU);
> -			ctrl->reg_write(ctrl, SWRM_V2_0_CLK_CTRL,
> +			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, ctrl->ee);
> +			ctrl->reg_write(ctrl, SWRM_V2_0_CLK_CTRL +
> +					((int)ctrl->ee - SWRM_EE_CPU) *
> +					SWRM_V2_REG_EE_STRIDE,
>  					SWRM_V2_0_CLK_CTRL_CLK_START);
>  		} else {
>  			ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
>  		}
> +
>  		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
>  			SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET);
>  

[Severity: High]
This is a pre-existing issue, but could there be a data race on ctrl->intr_mask
between the threaded interrupt handler and PM runtime callbacks?

Just below this code in swrm_runtime_resume(), ctrl->intr_mask is modified
locklessly:

    ctrl->intr_mask |= SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;

At the same time, the threaded IRQ handler performs its own lockless
read-modify-write on ctrl->intr_mask:

drivers/soundwire/qcom.c:qcom_swrm_irq_handler() {
    ...
    ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
    ...
}

Both execute in process context without synchronization, and the PM runtime
worker can be scheduled via wake_irq independently.

If these run concurrently on different CPUs, could this result in a
read-modify-write race where an update is lost?

If an update is lost, could an interrupt be unintentionally left unmasked,
potentially causing a persistent IRQ storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608175345.3118060-1-mohammad.rafi.shaik@oss.qualcomm.com?part=2