Re: [PATCH 2/2] drm: anx7625: add I2S input stream right justified control

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Bypassing standard ALSA HDMI codec DAI format negotiation.
- [Medium] RIGHT_JUSTIFY_CTRL bit is conditionally set but never cleared.

Pre-existing issues:
- [High] Use-After-Free due to interrupt firing after workqueue destruction.
--

--- Patch [2]: [PATCH 2/2] drm: anx7625: add I2S input stream right justified control ---
commit e78186b015f668998903e293729ea71c5bebfbd8
Author: Xin Ji <[email protected]>

drm: anx7625: add I2S input stream right justified control

The default audio data stream is left justify. When the
property "analogix,audio-right-justify-enable" is detected,
the code enables right justify.

> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index fffcd6154c710..d00aa6ea7273d 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c

[ ... ]

> @@ -2083,6 +2086,12 @@ static int anx7625_audio_hw_params(struct device *dev, void *data,
>  		ret |= anx7625_write_and(ctx, ctx->i2c.tx_p2_client,
>  				AUDIO_CHANNEL_STATUS_6, ~AUDIO_LAYOUT);
>  
> +	/* Right justified */
> +	if (ctx->pdata.audio_right_justify)

[Severity: Medium]
Instead of bypassing standard ALSA HDMI codec DAI format negotiation,
should this code check the standard fmt->fmt parameter for HDMI_RIGHT_J?

When the driver receives HDMI_RIGHT_J via hdmi_codec_daifmt->fmt at the
start of anx7625_audio_hw_params(), it rejects it with -EINVAL. Relying
on a custom static DT property breaks compatibility with generic ALSA
sound cards that dynamically negotiate the DAI format.

[Severity: Medium]
Is there a missing clear operation for this bit?

If a bootloader or previous kernel execution left the RIGHT_JUSTIFY_CTRL
bit set, and ctx->pdata.audio_right_justify is false, the hardware will
remain in right-justified mode even when standard I2S is expected. Should
there be an anx7625_write_and() to clear the bit on the false path?

> +		ret |= anx7625_write_or(ctx, ctx->i2c.tx_p2_client,
> +					AUDIO_CONTROL_REGISTER,
> +					RIGHT_JUSTIFY_CTRL);
> +
>  	/* FS */
>  	switch (params->sample_rate) {
>  	case 32000:

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the driver have a
use-after-free vulnerability during unbind?

In anx7625_i2c_remove(), the workqueue is destroyed before the interrupt is
disabled:

	if (platform->pdata.intp_irq)
		destroy_workqueue(platform->workqueue);

Because the driver uses devm_request_threaded_irq(), the interrupt is
only freed after remove() returns. If an interrupt (like Hot Plug Detect)
fires after destroy_workqueue(), the ISR anx7625_intr_hpd_isr() will
attempt to use it:

	queue_work(ctx->workqueue, &ctx->work);

Should disable_irq() be called before destroying the workqueue to prevent
the ISR from dereferencing the freed workqueue pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2