Re: [RFC PATCH 8/8] ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware reset causes immediate SoundWire I/O failure due to lost enumeration.
- [High] Mismatch between declared SoundWire sink ports and WCD9378_SDCA_MAX_PORTS leaves DP8 unmapped.
- [High] Missing `.remove` callback leaves the asynchronous `boot_work` running or queued, leading to a Use-After-Free.
- [Medium] Device-specific configuration parsed from DT mutates a shared static global array.
- [Medium] The register staging and commit sequence in `wcd9378_sdca_pde_post_pmu` lacks locking, allowing concurrent operations to interleave.
--

--- Patch [8]: [RFC PATCH 8/8] ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec ---
commit e171d84f19c198b9a986581a4ba450c4b632c883
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec

This commit adds support for the Qualcomm Tambora (WCD9378) headset codec
in SDCA mode over SoundWire. It integrates static SDCA topology and port
properties transcribed from the factory ACPI tables for ARM platforms.

> diff --git a/sound/soc/codecs/wcd9378-sdca.c b/sound/soc/codecs/wcd9378-sdca.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/codecs/wcd9378-sdca.c
> @@ -0,0 +1,1202 @@
[ ... ]
> +#define WCD9378_SDCA_MAX_PORTS	7
[ ... ]
> +static int wcd9378_sdca_sdw_read_prop(struct sdw_slave *slave)
> +{
[ ... ]
> +	/* Sink ports: DP6 (HPH audio), DP7 (HPH envelope), DP8 (optimisation RX). */
> +	prop->sink_ports = BIT(6) | BIT(7) | BIT(8);
[ ... ]
> +	if (device_property_read_u32_array(dev, "qcom,port-mapping",
> +					   &slave->m_port_map[1],
> +					   WCD9378_SDCA_MAX_PORTS))

[Severity: High]
Does this array read leave DP8 unmapped? 

The driver declares support for sink port 8 above, but
WCD9378_SDCA_MAX_PORTS is 7. Will m_port_map[8] remain uninitialized
and cause the SoundWire master to map DP8 to the control port instead of
a valid data port?

> +		dev_dbg(dev, "qcom,port-mapping not found, using default\n");
> +
> +	return 0;
> +}
[ ... ]
> +static int wcd9378_sdca_hw_init(struct sdw_slave *slave)
> +{
[ ... ]
> +	if (reset) {
> +		gpiod_set_value(reset, 1);
> +		/* 20us assert per WCD9378 reset sequence */
> +		usleep_range(20, 30);
> +		gpiod_set_value(reset, 0);
> +		/* 20us settle after deassert */
> +		usleep_range(20, 30);
> +	}
> +
> +	ret = device_property_read_u32(dev, "qcom,micbias2-microvolt",
> +				       &micbias_uv);
[ ... ]
> +	ret = wcd9378_sdca_micbias_uv_to_sdca(micbias_uv);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "unsupported qcom,micbias2-microvolt %u\n",
> +				     micbias_uv);
> +	ctrl_it33_micbias_vals[0] = ret;

[Severity: Medium]
Does writing the device-specific mic bias voltage into
ctrl_it33_micbias_vals[0] mutate a shared static global array?

If multiple instances of this driver probe, could they race to overwrite
this array, causing the last probed device to dictate the mic bias
voltage for all instances?

> +
> +	/*
> +	 * Vendor SCP bank-1 shadow that clocks the TX PDM path.  SCP
> +	 * registers survive PDE cycles, so the one-time write here is
> +	 * enough for every subsequent mic capture.  The trailing
> +	 * SCP_COMMIT swaps the bank in — safe to do at hw_init since no
> +	 * stream is running that could underflow.
> +	 */
> +	ret = sdw_write_no_pm(slave, WCD9378_SCP_HOST_CLK_DIV2_CTL_B1, 0x01);

[Severity: High]
Does this immediate write fail due to lost enumeration? 

Hard-resetting the slave clears its MIPI registers, reverting it to the
unattached state. The master needs time to asynchronously detect the
unattached device and re-enumerate it. 

Will sdw_write_no_pm() (using the old dev_num) fail with a NACK or timeout
because it does not wait for re-enumeration to complete?

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "HOST_CLK_DIV2_CTL_B1: %d\n", ret);
[ ... ]
> +static int wcd9378_sdca_pde_post_pmu(struct sdw_slave *slave,
> +				       struct regmap *regmap,
> +				       unsigned int function_id,
> +				       unsigned int entity_id)
> +{
[ ... ]
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_MUTE_CH1_ADDR,
> +			   WCD9378_FU42_MUTE_UNMUTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 MUTE_CH1: %d\n", ret);
> +
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_MUTE_CH2_ADDR,
> +			   WCD9378_FU42_MUTE_UNMUTE);
[ ... ]
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_CH_VOL_CH2_LSB_ADDR,
> +			   WCD9378_FU42_CH_VOL_UNITY_BYTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 CH2_LSB: %d\n", ret);
> +
> +	ret = sdw_write_no_pm(slave, SDW_SCP_COMMIT, 0x02);

[Severity: Medium]
Can concurrent operations interleave with this sequence? 

wcd9378_sdca_pde_post_pmu() executes multiple regmap_write() calls followed
by an unprotected sdw_write_no_pm() call. Since regmap_write() releases the
regmap lock internally between calls, could an ALSA mixer control using the
same commit mechanism race with a DAPM event and commit partially staged
FU42 writes prematurely?

> +	if (ret)
> +		dev_err(&slave->dev, "FU42 SCP_COMMIT: %d\n", ret);
> +
> +	return 0;
> +}
[ ... ]
> +static struct sdw_driver wcd9378_sdca_sdw_driver = {
> +	.driver = {
> +		.name	= "wcd9378-sdca",
> +		.pm	= pm_ptr(&sdca_class_pm_ops),
> +	},
> +	.probe		= wcd9378_sdca_sdw_probe,
> +	.id_table	= wcd9378_sdca_sdw_id,
> +	.ops		= &wcd9378_sdca_sdw_ops,
> +};

[Severity: High]
Could this cause a use-after-free on driver unbind? 

wcd9378_sdca_sdw_probe() delegates to sdca_class_probe(), which allocates
drv and queues drv->boot_work. Because there is no .remove callback in
wcd9378_sdca_sdw_driver to explicitly cancel this work, won't devres free
the drv object while class_boot_work remains pending or executing?

> +module_sdw_driver(wcd9378_sdca_sdw_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm SimpleJack (Tambora/WCD9378) SDCA codec");
> +MODULE_AUTHOR("Qualcomm Technologies, Inc.");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_IMPORT_NS("SND_SOC_SDCA_CLASS");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722234221.884765-1-srinivas.kandagatla@oss.qualcomm.com?part=8