Re: [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support

[Igor Paunovic]

> Does this byte-swapping logic scramble the extraction of the CTS and N
> values on little-endian architectures?

No. This is the same finding as in the v2 review, where it was already
answered, and the comment this v3 adds above the swap documents exactly
this. Two premises in the report don't match the ACR packet layout:
CTS does not live in PB0..PB2 and is not little-endian. Subpacket
byte 0 is reserved, CTS[19:16] sits in PB1[3:0], CTS[15:8] in PB2 and
CTS[7:0] in PB3 (MSB first), with N likewise in PB4..PB6. The packet
decoder stores PB0 in register bits [7:0], so the byte swap is
precisely what lines the bytes up before the 20-bit masks.

It is also the path validated on hardware: with a 44.1 kHz source the
card reports 44.1 kHz rather than falling back to the 48 kHz default,
and sample-rate switches are followed. Neither would work if the
extraction produced garbage.

> Can this lead to a use-after-free if the driver is unbound while an
> audio stream is active?

No -- the synchronization the report treats as missing is provided by
the ALSA core, and relying on it is why the v2 workaround was dropped
on Dmitry's review in the first place.

The worker is only ever armed between hw_params() and
audio_shutdown(). If a capture stream is still open when the codec
platform device is unregistered, teardown does not complete until the
stream is gone: soc_cleanup_card_resources() calls snd_card_free()
(sound/soc/soc-core.c), which blocks in wait_for_completion() until
the last file handle is closed (sound/core/init.c: "wait, until all
devices are ready for the free operation"). A disconnected card still
routes that final close through the original release path
(snd_disconnect_release() -> disconnected_f_op->release()), so
snd_pcm_release() runs soc_pcm_close() -> hdmi_codec shutdown ->
hdmirx_audio_shutdown(), which clears the streaming flag and
cancel_delayed_work_sync()s the worker.

platform_device_unregister() therefore cannot return with the worker
armed, and the devm-allocated state is freed only after remove()
returns. There is no window for the work to fire on freed memory.

Also exercised on the Orange Pi 5 Plus: unbinding the driver through
sysfs while arecord was actively capturing. The ALSA disconnect kicks
the reader out with ENODEV, the file handle is closed -- which is
where audio_shutdown() runs and cancels the worker -- and only then
does the unbind complete. A subsequent bind re-probes cleanly and the
card comes back. The kernel log stays free of warnings throughout.

Igor


On Sat, Jul 18, 2026 at 11:09 AM <sashiko-bot@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Incorrect byte-swapping logic completely scrambles the extraction of Audio Clock Regeneration (ACR) CTS and N values, breaking sample rate recovery on little-endian architectures.
> - [High] The `audio_work` delayed work is not explicitly cancelled in `hdmirx_remove()`, leading to a Use-After-Free if the driver is unbound while an audio stream is active.
> --
>
> --- Patch [2]: [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support ---
> commit b04bcd22f40b424aac824100715c46f764531875
> Author: Igor Paunovic <royalnet026@xxxxxxxxx>
>
> media: synopsys: hdmirx: add HDMI audio capture support
>
> The Synopsys DesignWare HDMI RX controller extracts the audio stream
> embedded in the incoming HDMI signal and feeds it to an on-SoC I2S
> controller. Expose it as an ALSA capture device by registering the
> generic hdmi-codec as a child of the controller.
>
> > diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> > index 25f8ca0d6d946..22f50cbf3fa7a 100644
> > --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> > +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
>
> [ ... ]
>
> > +/* Recover the incoming audio sample rate from the ACR N/CTS + TMDS clock. */
> > +static u32 hdmirx_audio_fs(struct snps_hdmirx_dev *hdmirx_dev)
> > +{
> > +     u64 tmds_clk, fs_audio = 0;
> > +     u32 acr_cts, acr_n, tmdsqpclk_freq;
> > +     u32 acr_pb3_0, acr_pb7_4;
> > +
> > +     tmdsqpclk_freq = hdmirx_readl(hdmirx_dev, CMU_TMDSQPCLK_FREQ);
> > +     hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PH2_1);
> > +     acr_pb3_0 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB3_0);
> > +     acr_pb7_4 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB7_4);
> > +     /*
> > +      * The packet decoder stores the ACR subpacket bytes with packet byte
> > +      * 0 in register bits [7:0], so byte-swap each word to line the bytes
> > +      * up: CTS is packet bytes 1-3 (PKTDEC_ACR_PB3_0) and N is packet
> > +      * bytes 4-6 (PKTDEC_ACR_PB7_4), 20 bits each.
> > +      */
> > +     acr_cts = be32_to_cpu((__force __be32)acr_pb3_0) & 0xfffff;
> > +     acr_n = (be32_to_cpu((__force __be32)acr_pb7_4) & 0x0fffff00) >> 8;
>
> [Severity: High]
> Does this byte-swapping logic scramble the extraction of the CTS and N values
> on little-endian architectures?
>
> Since the HDMI ACR packet transmits the 20-bit CTS value across PB0, PB1, and
> PB2, and because it is little-endian, applying a bitmask like
> acr_pb3_0 & 0xfffff natively yields the correct CTS value without any swaps.
>
> By byte-swapping the 32-bit register, the bytes are completely reversed.
> The subsequent bitmask extracts the lower 20 bits of the swapped word, which
> correspond to the lower nibble of PB1, all of PB2, and all of PB3, discarding
> PB0 entirely.
>
> The same applies to acr_pb7_4 to extract N. Will this regression result in
> garbage values for both acr_cts and acr_n, forcing the driver to blindly
> fall back to the default sample rate?
>
> [ ... ]
>
> > @@ -2784,6 +3052,9 @@ static void hdmirx_remove(struct platform_device *pdev)
> >       struct device *dev = &pdev->dev;
> >       struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
> >
> > +     if (hdmirx_dev->audio_pdev)
> > +             platform_device_unregister(hdmirx_dev->audio_pdev);
> > +
>
> [Severity: High]
> Can this lead to a use-after-free if the driver is unbound while an audio
> stream is active?
>
> The v3 changelog notes that the synchronous cancel for the audio_work
> delayed work was removed here under the assumption that audio_shutdown()
> handles it.
>
> However, ALSA does not synchronously invoke the PCM .shutdown callback
> when a device is unregistered if a userspace application holds the file
> descriptor open.
>
> Because hdmirx_dev is allocated using devm_kzalloc, it is freed immediately
> after hdmirx_remove() returns. Will the un-cancelled audio_work delayed work
> subsequently fire and access the freed hdmirx_dev memory?
>
> >       v4l2_debugfs_if_free(hdmirx_dev->infoframes);
> >       debugfs_remove_recursive(hdmirx_dev->debugfs_dir);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260718085728.6797-1-royalnet026@xxxxxxxxx?part=2