Re: [PATCH v3 8/9] ASoC: loongson: Add headphone jack detection and DAPM routing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Double-free of the hp-det GPIO descriptor on probe failure or driver unbind.
- [High] Global state mutation via static global instance structures.
- [Low] Misleading DAPM event handler name and widget array typos due to copy-pasting.
--

commit 96bfdeee5a35a70f217e0f245943a82eb9a7ce3b
Author: Binbin Zhou <zhoubinbin@loongson.cn>

ASoC: loongson: Add headphone jack detection and DAPM routing

Extend the Loongson audio machine driver with jack detection, DAPM
widgets support, enabling proper switching between headphones and
speakers on the Loongson-2K0300 ATK-DL2K0300B board.

> diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
> index 0e63cbcad57ac..25cd12eab4b1f 100644
> --- a/sound/soc/loongson/loongson_card.c
> +++ b/sound/soc/loongson/loongson_card.c

[ ... ]

> +static int tegra_machine_event(struct snd_soc_dapm_widget *w,
> +			       struct snd_kcontrol *k, int event)
> +{

[Severity: Low]
This isn't a bug, but should this function be renamed to match the loongson
namespace instead of keeping the tegra_ prefix from a copied driver?

> +	struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
> +	struct loongson_card_data *priv = snd_soc_card_get_drvdata(card);
> +
> +	if (!snd_soc_dapm_widget_name_cmp(w, "Speaker"))
> +		gpiod_set_value_cansleep(priv->gpiod_spkr_en,
> +					 SND_SOC_DAPM_EVENT_ON(event));

[ ... ]

> +static const struct snd_soc_dapm_widget loongson_aosc_dapm_widgets[] = {

[Severity: Low]
This isn't a bug, but there is a typo here (aosc instead of asoc).

> +	SND_SOC_DAPM_HP("Headphone", tegra_machine_event),
> +	SND_SOC_DAPM_SPK("Speaker", tegra_machine_event),
> +};
> +
> +/* Headphones Jack */
> +
> +static struct snd_soc_jack loongson_asoc_hp_jack;
> +
> +static struct snd_soc_jack_pin loongson_asoc_hp_jack_pins[] = {

[ ... ]

> +static struct snd_soc_jack_gpio loongson_asoc_hp_jack_gpio = {
> +	.name = "Headphones detection",
> +	.report = SND_JACK_HEADPHONE,
> +	.debounce_time = 150,
> +};

[Severity: High]
Will using a static global instance for snd_soc_jack_gpio cause cross-device
state corruption?

The ASoC core function snd_soc_jack_add_gpios() takes ownership of this
struct and initializes a delayed_work structure inside it. If multiple
instances of this driver probe concurrently or across unbind/rebind cycles,
multiple driver instances might mutate and re-initialize the same global
static struct and its workqueue list head.

Could this be moved to the instance-specific loongson_card_data instead?

> +
> +static int loongson_asoc_machine_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_card *card = rtd->card;
> +	struct loongson_card_data *ls_priv = snd_soc_card_get_drvdata(card);
> +	int ret = 0;

[ ... ]

> @@ -135,16 +229,35 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data)
>  
>  static int loongson_card_parse_of(struct loongson_card_data *data)
>  {
> -	struct device_node *cpu, *codec;
>  	struct snd_soc_card *card = &data->snd_card;
> +	struct device_node *cpu, *codec;
>  	struct device *dev = card->dev;
>  	int ret, i;
>  
> +	data->gpiod_hp_det = devm_gpiod_get_optional(dev, "hp-det", GPIOD_IN);

[Severity: High]
Will this devm-managed GPIO descriptor trigger a double-free on driver unbind
or probe deferral?

When devm_gpiod_get_optional() requests the GPIO, it registers a devres
cleanup action. Later in loongson_asoc_machine_init():

loongson_asoc_machine_init() {
    ...
    loongson_asoc_hp_jack_gpio.desc = ls_priv->gpiod_hp_det;
    ret = snd_soc_jack_add_gpios(&loongson_asoc_hp_jack, 1,
                                 &loongson_asoc_hp_jack_gpio);
    ...
}

The snd_soc_jack_add_gpios() call registers a second overlapping devres action
that unconditionally frees the GPIO descriptor. When the device is unbound,
won't both cleanup actions attempt to free the same gpio_device, leading to
a Use-After-Free?

> +	if (IS_ERR(data->gpiod_hp_det))
> +		return PTR_ERR(data->gpiod_hp_det);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782439646.git.zhoubinbin@loongson.cn?part=8