Re: [PATCH v4 12/22] arm64: dts: apple: t8103-j313: Add speaker/headset jack nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ASoC: apple: macaudio: OF ref leak and devres order in macaudio_devm_release()
- [Medium] ASoC: tas2770: tas2770_set_dai_tdm_idle() treats positive return as error
- [Medium] ASoC: apple: macaudio: macaudio_be_assign_tdm() overwrites slot error
- [Low] arm64: dts: apple: t8103-j313: unused #address-cells/#size-cells in sound
--

commit dd4cb33fc5a853fd54101899a161b5d0c84f160e
Author: James Calligeros <jcalligeros99@gmail.com>

arm64: dts: apple: t8103-j313: Add speaker/headset jack nodes

This commit adds the TI TAS5770L speaker amplifier and Cirrus CS42L83
headset jack codec device tree nodes for the M1 MacBook Air (J313), along
with the sound node advertising them to the macaudio machine driver.

> diff --git a/arch/arm64/boot/dts/apple/t8103-j313.dts b/arch/arm64/boot/dts/apple/t8103-j313.dts
> index f8b2b1637b9dc..36c47734a1dc0 100644
> --- a/arch/arm64/boot/dts/apple/t8103-j313.dts
> +++ b/arch/arm64/boot/dts/apple/t8103-j313.dts
> @@ -55,4 +55,89 @@ &typec1 {

[ ... ]

> +/ {
> +	sound: sound {
> +		compatible = "apple,j313-macaudio";
> +		model = "MacBook Air J313";

[Severity: High]
As reported by Sashiko on v3 [1], when this apple,j313-macaudio node binds to
macaudio_snd_driver, does macaudio_snd_platform_probe() in
sound/soc/apple/macaudio.c leak the backend DAI link CPU and CODEC device_node
references on unbind or if devm_add_action() fails, and can the devres
registration order cause macaudio_vlimit_update() to dereference a NULL
ma->card.snd_card during unbind?

During probe, macaudio_parse_of() -> macaudio_parse_of_be_dai_link() ->
macaudio_parse_of_component() acquires device_node references via
of_parse_phandle_with_args() for the CPU and CODEC DAIs. In
macaudio_snd_platform_probe():

sound/soc/apple/macaudio.c:macaudio_snd_platform_probe() {
	...
	ret = devm_add_action(dev, macaudio_devm_release, data);
	if (ret)
		return ret;

	ret = devm_snd_soc_register_card(dev, &data->card);
	if (ret) {
		for_each_card_prelinks(&data->card, i, link) {
			snd_soc_of_put_dai_link_codecs(link);
			snd_soc_of_put_dai_link_cpus(link);
		}

		return ret;
	}
}

If devm_add_action() fails, macaudio_snd_platform_probe() returns without
releasing the DAI link CPU and CODEC device_node references. Likewise, when
probe succeeds and the device is later unbound, neither ASoC core teardown
nor macaudio_devm_release() calls snd_soc_of_put_dai_link_codecs() and
snd_soc_of_put_dai_link_cpus():

sound/soc/apple/macaudio.c:macaudio_devm_release() {
	struct macaudio_snd_data *ma = data;

	cancel_work_sync(&ma->lock_update_work);
	cancel_delayed_work_sync(&ma->lock_timeout_work);
}

In addition, because devm_add_action(dev, macaudio_devm_release, data) is
registered before devm_snd_soc_register_card(), devres LIFO teardown runs
devm_card_bind_release() -> snd_soc_unregister_card() ->
soc_cleanup_card_resources() first, which frees card->snd_card and sets it to
NULL before macaudio_devm_release() cancels lock_update_work and
lock_timeout_work. If either work item runs during teardown,
macaudio_vlimit_update() accesses ma->card.snd_card->controls_rwsem:

sound/soc/apple/macaudio.c:macaudio_vlimit_update() {
	...
	if (take_rwsem)
		down_write(&ma->card.snd_card->controls_rwsem);
}

> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;

[Severity: Low]
Will defining #address-cells and #size-cells on the sound node trigger a dtc
avoid_unnecessary_addr_size warning when building with W=1?

Neither headphone-dai-link nor speaker-dai-link uses a reg or ranges property
(and Documentation/devicetree/bindings/sound/apple,macaudio.yaml does not
define reg on child dai-link nodes despite listing #address-cells and
#size-cells in required), which causes check_avoid_unnecessary_addr_size() in
scripts/dtc/checks.c to warn under W=1.

Also, since arch/arm64/boot/dts/apple/t8103-j313.dts already has a root
/ { ... }; block at the top of the file, should the sound node be placed in
that existing root node block rather than opening a second / { ... }; block
after the phandle overrides?

> +
> +		headphone-dai-link {
> +			link-name = "Headphone Jack";
> +
> +			codec {
> +				sound-dai = <&jack_codec>;
> +			};
> +
> +			cpu {
> +				sound-dai = <&mca 2>;
> +			};
> +		};
> +
> +		speaker-dai-link {
> +			dai-tdm-idle-mode-0 = "zero";
> +			dai-tdm-idle-mode-1 = "zero";

[Severity: Medium]
When speaker-dai-link sets dai-tdm-idle-mode-0 and dai-tdm-idle-mode-1 to
"zero" for the two ti,tas5770l codecs, will tas2770_set_dai_tdm_idle() fail to
complete the TDM idle setup and cause macaudio_be_assign_tdm() to fail?

During backend initialization, macaudio_be_init() -> macaudio_be_assign_tdm()
calls snd_soc_dai_set_tdm_idle() with SND_SOC_DAI_TDM_IDLE_ZERO, which
dispatches to tas2770_set_dai_tdm_idle() in sound/soc/codecs/tas2770.c:

sound/soc/codecs/tas2770.c:tas2770_set_dai_tdm_idle() {
	...
	case SND_SOC_DAI_TDM_IDLE_ZERO:
		ret = snd_soc_component_update_bits(component, TAS2770_TDM_CFG_REG4,
						    TAS2770_TDM_CFG_REG4_TX_KEEPER,
						    TAS2770_TDM_CFG_REG4_TX_KEEPER);
		if (ret)
			return ret;

		ret = snd_soc_component_update_bits(component, TAS2770_TDM_CFG_REG4,
						    TAS2770_TDM_CFG_REG4_TX_FILL, 0);
		if (ret)
			return ret;
		break;
}

Because snd_soc_component_update_bits() returns 1 when the register value
changes, 0 when unchanged, and a negative errno on error, setting
TAS2770_TDM_CFG_REG4_TX_KEEPER after reset returns 1. Checking if (ret)
instead of if (ret < 0) causes tas2770_set_dai_tdm_idle() to return 1 early
without clearing TAS2770_TDM_CFG_REG4_TX_FILL or updating
tas2770->idle_tx_mode.

In sound/soc/apple/macaudio.c, macaudio_be_assign_tdm() then checks if (ret)
on the return value of snd_soc_dai_set_tdm_idle():

sound/soc/apple/macaudio.c:macaudio_be_assign_tdm() {
	...
		if (props->codecs[0].idle_mode) {
			ret = snd_soc_dai_set_tdm_idle(dai, props->codecs[0].idle_mask,
						       0, props->codecs[0].idle_mode, 0);
			if (ret) {
				dev_err(card->dev, "Failed to set idle props for codec\n");
				return ret;
			}

		}
}

Won't this treat the positive return value 1 as an error and log "Failed to
set idle props for codec" when initializing speaker_left and speaker_right?

[Severity: Medium]
In the same single-codec branch of macaudio_be_assign_tdm() in
sound/soc/apple/macaudio.c (used for each of the two CPU/CODEC pairs in
speaker-dai-link), can a failure from snd_soc_dai_set_tdm_slot() be
overwritten when props->codecs[0].idle_mode is set?

Looking at macaudio_be_assign_tdm():

sound/soc/apple/macaudio.c:macaudio_be_assign_tdm() {
	if (rtd->dai_link->num_codecs == 1) {
		dai = snd_soc_rtd_to_codec(rtd, 0);
		ret = snd_soc_dai_set_tdm_slot(dai, mask, 0, nslots, MACAUDIO_SLOTWIDTH);

		/*
		 * Headphones get a pass on -ENOTSUPP (see the comment
		 * around bclk_ratio value for primary FE).
		 */
		if (ret == -ENOTSUPP && props->is_headphones)
			return 0;

		if (props->codecs[0].idle_mode) {
			ret = snd_soc_dai_set_tdm_idle(dai, props->codecs[0].idle_mask,
						       0, props->codecs[0].idle_mode, 0);
			if (ret) {
				dev_err(card->dev, "Failed to set idle props for codec\n");
				return ret;
			}

		}

		return ret;
	}
}

Unlike the multi-codec loop below it (which checks if (ret) return ret;
immediately after snd_soc_dai_set_tdm_slot()), the num_codecs == 1 path does
not check ret before entering if (props->codecs[0].idle_mode), so
snd_soc_dai_set_tdm_idle() overwrites any negative error returned by
snd_soc_dai_set_tdm_slot().

[1] https://lore.kernel.org/r/20261018-macaudio-v3-11-abdff7741dea@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-macaudio-v4-0-637484efe46d@gmail.com?part=12