Re: [PATCH 2/3] soundwire: qcom: add multi-master support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The primary controller unconditionally enables hardware interrupts on the dependent controller, whose IRQ handler will crash when accessing uninitialized bus structures.
- [High] The dependent controller skips PM runtime initialization, permanently leaving its interface clock enabled and defeating the device link power management.
- [High] If the primary controller's probe fails, the dependent controller is left with a dangling pointer to freed memory and inappropriately enabled hardware interrupts.
- [High] Primary controller permanently fails to probe with -EINVAL if it probes before the dependent controller has bound.
- [High] Lockless access to the peer device's driver state and drvdata during multi-master setup causes a Use-After-Free if the peer device is concurrently unbound.
- [High] Tearing down `peer_ctrl` linkage before calling `sdw_bus_master_delete()` causes data races and potential NULL pointer dereferences for ongoing bus operations.
- [High] The primary controller writes to the peer's hardware registers during probe without ensuring the peer device is runtime resumed.
--

commit 6d727a8db765ee8b093a2252b9a03525a3473f6a
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

soundwire: qcom: add multi-master support

v3.1.0+ Qualcomm SoundWire IPs can be paired to expose one logical bus with
more than two DATA lanes. This patch supports this pairing using a device
link to enforce unbind ordering and power management.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 35ffffd541bd1..a5f39868161af 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
[ ... ]
> @@ -1562,6 +1608,68 @@ static int swrm_reg_show(struct seq_file *s_file, void *data)
>  DEFINE_SHOW_ATTRIBUTE(swrm_reg);
>  #endif
>  
> +static int qcom_swrm_multi_master_setup(struct qcom_swrm_ctrl *ctrl)
> +{
> +	struct of_phandle_args args;
> +	struct qcom_swrm_ctrl *peer;
> +	struct platform_device *pdev;
> +	u32 offset;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_fixed_args(ctrl->dev->of_node,
> +					       "qcom,multi-master-peer",
> +					       1, 0, &args);
> +	if (ret)
> +		return -EINVAL;
> +
> +	pdev = of_find_device_by_node(args.np);
> +	of_node_put(args.np);
> +	if (!pdev)
> +		return -EPROBE_DEFER;
> +
> +	if (pdev->dev.driver != ctrl->dev->driver) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	peer = platform_get_drvdata(pdev);
> +	if (!peer || !peer->is_dependent) {
> +		ret = -EPROBE_DEFER;
> +		goto out;
> +	}

[Severity: High]
If the dependent controller is present in the device tree but has not yet
bound to its driver, pdev->dev.driver will be NULL. Does this strict equality
check cause the primary controller to permanently fail with -EINVAL instead of
returning -EPROBE_DEFER to wait for the dependent controller?

[Severity: High]
Additionally, does accessing pdev->dev.driver and platform_get_drvdata(pdev)
without holding device_lock(&pdev->dev) allow for a use-after-free? If the
peer device is concurrently unbound via sysfs, devres could free the
underlying memory while it is being accessed here.

[ ... ]
> @@ -1689,6 +1810,16 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		ctrl->bus.controller_id = val;
>  	}
>  
> +	if (of_property_match_string(dev->of_node, "qcom,multi-master-mode", "dependent") >= 0) {
> +		ctrl->is_dependent = true;
> +		dev_dbg(dev,
> +			"Qualcomm SoundWire multi-master dependent v%x.%x.%x registered (%u lanes)\n",
> +			(ctrl->version >> 24) & 0xff,
> +			(ctrl->version >> 16) & 0xff,
> +			ctrl->version & 0xffff, ctrl->num_lanes);
> +		return 0;
> +	}
> +

[Severity: High]
This early return bypasses all PM runtime setup for the dependent controller,
such as pm_runtime_enable(). Because its PM runtime is disabled, the PM core
considers it permanently suspended. Does this mean its swrm_runtime_suspend
callback is never invoked, permanently leaving ctrl->hclk enabled and
defeating the device link power management?

[Severity: High]
This early return also bypasses the initialization of the bus structures like
ctrl->bus.bus_lock and ctrl->bus.slaves. Later, the primary controller calls
qcom_swrm_init() on the dependent controller, which unconditionally enables
hardware interrupts by writing to SWRM_REG_INTERRUPT_CPU_EN. If the dependent
hardware raises an interrupt, could its IRQ handler crash when attempting to
lock the uninitialized mutex or traverse the uninitialized slave list?

>  	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
>  	if (ret) {
>  		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> @@ -1696,7 +1827,11 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		goto err_clk;
>  	}
>  
> +	if (ctrl->is_primary && ctrl->peer_ctrl)
> +		qcom_swrm_init(ctrl->peer_ctrl);
> +

[Severity: High]
This initiates register writes on the peer hardware without first calling
pm_runtime_get_sync() on the primary device. Since the primary controller is
not yet runtime-resumed during this phase of probe, the device link won't
have powered on the dependent controller either. Can this cause an unclocked
access exception or lost writes if the peer hardware is powered off?

[Severity: High]
If probe fails shortly after this (e.g., during qcom_swrm_register_dais()),
the error path unwinds the primary controller's structures. However, it
appears the dependent controller is left with a dangling pointer to freed
memory in peer->peer_ctrl and inappropriately enabled hardware interrupts.
Should the error path clean up the dependent controller's state?

>  	qcom_swrm_init(ctrl);
> +
>  	wait_for_completion_timeout(&ctrl->enumeration,
>  				    msecs_to_jiffies(TIMEOUT_MS));
>  	ret = qcom_swrm_register_dais(ctrl);
> @@ -1733,7 +1868,13 @@ 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);
> +	if (ctrl->is_primary && ctrl->peer_ctrl) {
> +		ctrl->peer_ctrl->peer_ctrl = NULL;
> +		ctrl->peer_ctrl = NULL;
> +	}
> +
> +	if (!ctrl->is_dependent)
> +		sdw_bus_master_delete(&ctrl->bus);
>  	clk_disable_unprepare(ctrl->hclk);
>  }

[Severity: High]
Clearing these cross-references before calling sdw_bus_master_delete() seems
to expose ongoing bus operations to data races and NULL pointer dereferences.
For example, concurrent streams calling qcom_swrm_pre_bank_switch() check
ctrl->peer_ctrl and then dereference it. Should the bus be stopped before
clearing the linkage?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789114385.git.srinivas.kandagatla@oss.qualcomm.com?part=2