Re: [PATCH 3/3] ASoC: codecs: wcd9378: add TX/capture codec driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Modifying a static global array during probe prevents multiple device instances from working correctly.
- [High] `pm_runtime_enable()` is called after `component_master_add_with_match()`, causing component probe to fail synchronously.
- [High] Missing bounds check in `wcd9378_tx_mode_put()` allows out-of-bounds enum values to be written and retrieved.
- [High] Inconsistent SoundWire bus clock scaling calculation between `wcd9378_bus_config()` and `wcd9378_swr_clk_indicate()`.
- [High] Destroying `micb_lock` in the component unbind callback leads to a use-after-destroy upon component re-bind.
- [Medium] Off-by-one error when calculating the SoundWire channel count causes incorrect bandwidth allocation.
- [Medium] Missing High-Pass Filter (HPF) init pulse configuration for ADC2 when routed via SmartJACK.
- [Medium] Concurrent ALSA control puts cause read-modify-write data races on shared driver channel maps.
- [Medium] Using asynchronous `pm_runtime_put()` in `wcd9378_unbind()` causes a race condition where the TX SoundWire slave device may not be properly suspended before removal.
--

commit 336c5ad4fab99f2ac788ce17fc541b077bfc53c9
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
Subject: ASoC: codecs: wcd9378: add TX/capture codec driver

This patch adds the TX/capture driver for the Qualcomm WCD9378 audio codec
over SoundWire. It introduces analog core configuration and SDCA-style 
function blocks for SmartMIC and SmartJACK sequencers.

> diff --git a/sound/soc/codecs/wcd9378-sdw.c b/sound/soc/codecs/wcd9378-sdw.c
> --- a/sound/soc/codecs/wcd9378-sdw.c
> +++ b/sound/soc/codecs/wcd9378-sdw.c
[ ... ]
> +int wcd9378_sdw_hw_params(struct wcd9378_sdw_priv *wcd,
> +			  struct snd_pcm_substream *substream,
> +			  struct snd_pcm_hw_params *params,
> +			  struct snd_soc_dai *dai)
> +{
> +	struct sdw_port_config port_config[WCD9378_MAX_SWR_PORTS];
> +	unsigned long ch_mask;
> +	int i, j;
> +
> +	wcd->sconfig.ch_count = 1;

[Severity: Medium]
Does initializing the channel count to 1 here cause an off-by-one error?
It looks like if there are two active channels, the count will become 3,
which might lead to overallocating SoundWire bus bandwidth.

[ ... ]
> +static int wcd9378_bus_config(struct sdw_slave *slave,
> +			      struct sdw_bus_params *params)
> +{
> +	u8 scale;
> +
> +	switch (params->curr_dr_freq) {
> +	case 4800000:
> +		scale = WCD9378_SWRS_CLK_SCALE_DIV4;
> +		break;
> +	case 9600000:
> +	default:
> +		scale = WCD9378_SWRS_CLK_SCALE_DIV2;
> +		break;
> +	}

[Severity: High]
Is there a calculation mismatch between wcd9378_bus_config() and 
wcd9378_swr_clk_indicate()?

Here, a 9.6MHz double-rate frequency gets WCD9378_SWRS_CLK_SCALE_DIV2.
However, in wcd9378_swr_clk_indicate() in wcd9378.c, the bus clock is 
calculated by dividing the DR frequency by 2, resulting in 4.8MHz. That 
function then evaluates 4800000 < 9600000 and assigns 
WCD9378_SWRS_CLK_SCALE_DIV4 instead. 

Could this stall the sequencer if the configurations conflict?

[ ... ]
> +static int wcd9378_sdw_probe(struct sdw_slave *pdev,
> +			     const struct sdw_device_id *id)
> +{
[ ... ]
> +	if (wcd->is_tx) {
> +		pdev->prop.source_ports = GENMASK(WCD9378_MAX_TX_SWR_PORTS, 1);
> +		pdev->prop.src_dpn_prop = wcd9378_dpn_prop;
> +		wcd->ch_info = &wcd9378_sdw_tx_ch_info[0];
> +
> +		for (i = 0; i < master_ch_mask_size; i++)
> +			wcd->ch_info[i].master_ch_mask = WCD9378_SWRM_CH_MASK(master_ch_mask[i]);

[Severity: High]
Will this loop permanently modify the global static array wcd9378_sdw_tx_ch_info?
Since the master_ch_mask is written directly to the shared array, probing a 
second instance of this codec could overwrite the configuration for all active
devices.

> diff --git a/sound/soc/codecs/wcd9378.c b/sound/soc/codecs/wcd9378.c
> --- a/sound/soc/codecs/wcd9378.c
> +++ b/sound/soc/codecs/wcd9378.c
[ ... ]
> +static const struct wcd9378_smp_fn wcd9378_smp_jack_adc2 = {
> +	.usage_reg = WCD9378_SMP_JACK_IT31_USAGE,
> +	.micb_reg = WCD9378_SMP_JACK_IT31_MICB,
> +	.req_reg = WCD9378_SMP_JACK_PDE34_REQ_PS,
> +	.act_reg = WCD9378_SMP_JACK_PDE34_ACT_PS,
> +};

[Severity: Medium]
Should this struct also include the hpf_reg and hpf_mask fields?
When ADC2 is fed from AMIC2 via SmartJACK, the missing filter configuration
means the sequencer enable path will read zeroes, skipping the
WCD9378_ANA_TX_CH2_HPF2_INIT pulse and potentially leaving a DC offset.

[ ... ]
> +static int wcd9378_connect_port(struct wcd9378_sdw_priv *wcd, u8 port_idx,
> +				u8 ch_id, bool enable)
> +{
[ ... ]
> +	if (enable) {
> +		port_config->ch_mask |= ch_mask;
> +		wcd->master_channel_map[mstr_port_num] |= mstr_ch_mask;
> +	} else {
> +		port_config->ch_mask &= ~ch_mask;
> +		wcd->master_channel_map[mstr_port_num] &= ~mstr_ch_mask;
> +	}

[Severity: Medium]
Can concurrent ALSA control operations corrupt the channel masks?
Since wcd9378_set_swr_port() is reachable via ALSA put callbacks, multiple
userspace threads toggling switches might cause a read-modify-write data
race on port_config->ch_mask and wcd->master_channel_map.

[ ... ]
> +static int wcd9378_tx_mode_put(struct snd_kcontrol *kcontrol,
> +			       struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	u32 mode_val = ucontrol->value.enumerated.item[0];
> +
> +	if (mode_val == wcd9378->tx_mode[adc])
> +		return 0;
> +
> +	wcd9378->tx_mode[adc] = mode_val;

[Severity: High]
Could this store an out-of-bounds enum value?
Without a check verifying that mode_val is less than ARRAY_SIZE(tx_mode_mux_text),
userspace can write an invalid integer here. This value is later exposed via
wcd9378_tx_mode_get(), which might crash userspace audio servers when used as
an array index.

[ ... ]
> +static int wcd9378_soc_codec_probe(struct snd_soc_component *component)
> +{
[ ... ]
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		return ret;

[Severity: High]
Will this pm_runtime_resume_and_get() call fail with -EACCES?
It looks like component_master_add_with_match() in wcd9378_probe() triggers
the component bind synchronously if all slaves are present, which in turn
calls wcd9378_soc_codec_probe() here. Because pm_runtime_enable() hasn't
been called yet in the master probe, the disable depth is still > 0,
causing the resume to abort audio initialization.

[ ... ]
> +static void wcd9378_unbind(struct device *dev)
> +{
> +	struct wcd9378_priv *wcd9378 = dev_get_drvdata(dev);
> +
> +	snd_soc_unregister_component(dev);
> +	pm_runtime_put(wcd9378->txdev);

[Severity: Medium]
Should this be pm_runtime_put_sync() instead?
Using the asynchronous pm_runtime_put() queues a suspend operation that might
be cancelled immediately when the driver core calls pm_runtime_disable() upon
device removal, leaving the TX SoundWire slave device powered on.

> +	device_link_remove(dev, wcd9378->txdev);
> +	device_link_remove(dev, wcd9378->rxdev);
> +	device_link_remove(wcd9378->rxdev, wcd9378->txdev);
> +	component_unbind_all(dev, wcd9378);
> +	mutex_destroy(&wcd9378->micb_lock);

[Severity: High]
Does destroying micb_lock here prevent a successful re-bind?
The component framework allows unbinding and rebinding without fully removing
the master platform device. Since mutex_init() is only called in wcd9378_probe(),
a subsequent wcd9378_bind() will leave the destroyed mutex uninitialized, leading
to undefined behavior or a crash when wcd9378_micbias_control() tries to lock it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706192229.144137-1-jorijnvdgraaf@catcrafts.net?part=3