Re: [PATCH 4/5] media: i2c: it6625: add I2S audio capture DAI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: i2c: it6625: it6625_dai_shutdown() cancels active rate-settle polling
- [Medium] media: i2c: it6625: it6625_audio_refresh() in DAI ops skips V4L2 ctrl update
--

commit 1dfd81c0e8bfaba0b855fe94b881d239f6f65287
Author: Hermes Wu <Hermes.wu@ite.com.tw>

media: i2c: it6625: add I2S audio capture DAI

This commit registers an ASoC component and I2S capture DAI for the IT6625
and IT6626 HDMI-to-MIPI bridges when #sound-dai-cells is present in the
device tree. It constrains capture streams to the currently observed HDMI
audio status, stops active streams on status changes, and polls for audio
status updates and rate settling.

> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index 6f12a0e760178..f1bd42ad15eda 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c

[ ... ]

> @@ -2001,6 +2158,369 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
>  	return 0;
>  }
>  

[ ... ]

> +static void it6625_audio_refresh_work(struct work_struct *work)
> +{
> +	struct it6625 *it6625 = container_of(to_delayed_work(work),
> +					     struct it6625, audio_refresh_work);
> +	unsigned long flags;
> +	bool rearm;
> +
> +	it6625_s_ctrl_audio(&it6625->sd);
> +
> +	spin_lock_irqsave(&it6625->audio_lock, flags);
> +	/* every run uses up one re-check of a settle budget */
> +	if (it6625->audio_settle_polls)
> +		it6625->audio_settle_polls--;
> +	rearm = (it6625->audio_substream && !it6625->audio_block) ||
> +		(it6625->audio_settle_polls && it6625_audio_settling(it6625));
> +	spin_unlock_irqrestore(&it6625->audio_lock, flags);
> +
> +	if (rearm)
> +		schedule_delayed_work(&it6625->audio_refresh_work,
> +				      msecs_to_jiffies(IT6625_AUDIO_REFRESH_MS));
> +}
> +
> +static struct it6625 *dai_to_6625(struct snd_soc_dai *dai)
> +{
> +	/* the I2C client data is the V4L2 subdev */
> +	return sd_to_6625(snd_soc_dai_get_drvdata(dai));
> +}
> +
> +static int it6625_dai_startup(struct snd_pcm_substream *substream,
> +			      struct snd_soc_dai *dai)
> +{
> +	struct it6625 *it6625 = dai_to_6625(dai);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct it6625_audio_info info;
> +	unsigned long flags;
> +	u32 gen;
> +	int ret;
> +
> +	it6625_audio_refresh(it6625);

[ ... ]

> +/* also called on rollback after a successful .startup */
> +static void it6625_dai_shutdown(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct it6625 *it6625 = dai_to_6625(dai);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&it6625->audio_lock, flags);
> +	it6625->audio_block = true;
> +	spin_unlock_irqrestore(&it6625->audio_lock, flags);
> +
> +	cancel_delayed_work_sync(&it6625->audio_refresh_work);
> +	cancel_work_sync(&it6625->audio_xrun_work);
> +
> +	spin_lock_irqsave(&it6625->audio_lock, flags);
> +	it6625->audio_substream = NULL;
> +	it6625->audio_running = false;
> +	it6625->audio_block = false;
> +	spin_unlock_irqrestore(&it6625->audio_lock, flags);
> +}

[Severity: Medium]
Can it6625_dai_shutdown() prematurely stop rate-settle polling started by
it6625_audio_event()?

If an HDMI format or audio event occurs while a capture stream is running and
leaves audio present without a valid sampling rate, it6625_audio_event()
performs:

  it6625_audio_event()
    it6625_s_ctrl_audio()
      it6625_audio_refresh()
        it6625->audio_gen++;
        queue_work(system_wq, &it6625->audio_xrun_work);
    if (it6625_audio_settling(it6625)) {
        it6625->audio_settle_polls = IT6625_AUDIO_SETTLE_POLLS;
        schedule_delayed_work(&it6625->audio_refresh_work, ...);
    }

Because audio_gen was incremented, subsequent calls to it6625_dai_prepare() or
it6625_dai_hw_params() return -EPIPE, so userspace has to close the capture
stream to recover.

When userspace closes the stream, it6625_dai_shutdown() runs
cancel_delayed_work_sync(&it6625->audio_refresh_work) and does not re-schedule
audio_refresh_work after clearing audio_block even when
it6625->audio_settle_polls > 0 and it6625_audio_settling(it6625) is still
true. If the bridge raises no further interrupt once the rate settles, won't
ctrl_audio_sampling_rate and it6625->audio remain stuck at rate 0?

> +
> +static int it6625_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	if ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_CBP_CFP ||
> +	    (fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_I2S ||
> +	    (fmt & SND_SOC_DAIFMT_INV_MASK) != SND_SOC_DAIFMT_NB_NF)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int it6625_dai_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	struct it6625 *it6625 = dai_to_6625(dai);
> +	const struct it6625_audio_stream *s;
> +	unsigned long flags;
> +	int ret;
> +
> +	it6625_audio_refresh(it6625);
> +
> +	spin_lock_irqsave(&it6625->audio_lock, flags);
> +	ret = it6625_audio_validate_locked(it6625);
> +	s = &it6625->audio_accepted;
> +	if (!ret &&
> +	    (params_rate(params) != it6625_audio_fs_to_rate(s->fs_code) ||
> +	     params_channels(params) != s->channels ||
> +	     params_format(params) != SNDRV_PCM_FORMAT_S32_LE))
> +		ret = -EINVAL;
> +	spin_unlock_irqrestore(&it6625->audio_lock, flags);
> +
> +	return ret;
> +}
> +
> +static int it6625_dai_prepare(struct snd_pcm_substream *substream,
> +			      struct snd_soc_dai *dai)
> +{
> +	struct it6625 *it6625 = dai_to_6625(dai);
> +	unsigned long flags;
> +	int ret;
> +
> +	it6625_audio_refresh(it6625);
> +
> +	spin_lock_irqsave(&it6625->audio_lock, flags);
> +	ret = it6625_audio_validate_locked(it6625);
> +	spin_unlock_irqrestore(&it6625->audio_lock, flags);
> +
> +	return ret;
> +}

[Severity: Medium]
Should it6625_dai_startup(), it6625_dai_hw_params(), and it6625_dai_prepare()
call it6625_s_ctrl_audio(&it6625->sd) instead of calling
it6625_audio_refresh(it6625) directly?

Calling it6625_audio_refresh() updates it6625->audio and increments
it6625->audio_gen when a status change is observed, but skips
it6625_audio_update_ctrls().

If an audio change that raised no interrupt is observed by
it6625_dai_startup() and causes it6625_audio_check() to fail, startup returns
an error without scheduling audio_refresh_work and without updating
ctrl_audio_sampling_rate or ctrl_audio_present. Similarly, if
it6625_dai_hw_params() or it6625_dai_prepare() observes a change and returns
-EPIPE, and userspace closes the substream before audio_refresh_work runs,
it6625_dai_shutdown() cancels audio_refresh_work and leaves the v4l2 controls
out of sync with the sample stored in it6625->audio.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-upstream-it6625-alsa-i2s-v1-0-bb91e2879b01@ite.com.tw?part=4