On Fri, 2026-06-26 at 14:44 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> New issues:
> - [High] Audio state is permanently broken upon reconnect if a full
> modeset does not occur.
> - [Low] Spurious ELD notifications with an invalid pin (-1) during
> normal CRTC disable sequence.
>
> Pre-existing issues:
> - [Critical] Data race between ELD notification and audio component
> unbind leads to Use-After-Free.
> --
>
> commit 0c399aeba0fb63f7bdb4374bcb0fb67d5cd679bb
> Author: Nícolas F. R. A. Prado <[email protected]>
>
> drm/amd/display: Always notify the HDA driver on connector disconnect
>
> This patch modifies the display detect logic to call
> amdgpu_dm_audio_eld_notify() when a sink is disconnected. This
> ensures the
> HDA driver is notified even if no subsequent atomic commit occurs.
>
> > diff --git
> > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_audio.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_audio.c
> > index 13c9a9d145baa..c9da19335eacf 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_audio.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_audio.c
> > @@ -168,7 +168,7 @@ void amdgpu_dm_audio_fini(struct amdgpu_device
> > *adev)
> > }
> > EXPORT_IF_KUNIT(amdgpu_dm_audio_fini);
> >
> > -STATIC_IFN_KUNIT void amdgpu_dm_audio_eld_notify(struct
> > amdgpu_device *adev, int pin)
> > +void amdgpu_dm_audio_eld_notify(struct amdgpu_device *adev, int
> > pin)
> > {
> > struct drm_audio_component *acomp = adev-
> > >dm.audio_component;
> >
>
> [Severity: Critical]
> This is a pre-existing issue, but can this lead to a use-after-free
> if the
> HDA module is unloaded concurrently?
>
> Looking at amdgpu_dm_audio_eld_notify(), the acomp pointer and its
> audio_ops
> are checked and dereferenced without holding any locks:
>
> if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify) {
>
> Concurrently, if the HDA module is unloaded,
> amdgpu_dm_audio_component_unbind() clears these component fields. If
> the
> notify function is preempted after its check, could it dereference
> the
> freed audio_ops?
As mentioned this is unrelated to this patch.
>
> [ ... ]
>
> > diff --git
> > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> > index 300ee26f26ff0..9286f86a20cd7 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
>
> [ ... ]
>
> > @@ -601,6 +603,13 @@ void amdgpu_dm_update_connector_after_detect(
> > /* Set CP to DESIRED if it was ENABLED, so we can
> > re-enable it again on hotplug */
> > if (connector->state->content_protection ==
> > DRM_MODE_CONTENT_PROTECTION_ENABLED)
> > connector->state->content_protection =
> > DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > +
> > + mutex_lock(&adev->dm.audio_lock);
> > + inst = aconnector->audio_inst;
> > + aconnector->audio_inst = -1;
> > + mutex_unlock(&adev->dm.audio_lock);
> > + if (inst != -1)
> > + amdgpu_dm_audio_eld_notify(adev, inst);
>
> [Severity: High]
> Does wiping aconnector->audio_inst to -1 on disconnect permanently
> break
> audio upon reconnect if a full modeset does not occur?
>
> If a display is reconnected and userspace triggers an atomic commit
> that does
> not require a full modeset (like a fastset), amdgpu_dm_commit_audio()
> skips
> the connector because drm_atomic_crtc_needs_modeset() is false.
>
> In that scenario, does audio_inst remain -1, preventing the HDA
> driver from
> ever being notified of the new ELD?
I couldn't find a real use case scenario where this might happen. I
tested turning the display off/on and disconnecting/reconnecting the
HDMI cable on multiple compositors (kwin, mutter, sway, weston,
gamescope) and in all cases a full modeset atomic commit was applied.
The only exception was when disabling the use of the display in the
compositor, but then the audio driver disables the audio devices
anyway, so this wouldn't be an issue.
>
> [Severity: Low]
> Also, does setting audio_inst to -1 here cause spurious ELD
> notifications
> with an invalid pin during a normal CRTC disable sequence?
>
> If a display is disconnected, audio_inst becomes -1. Later, when a
> modeset
> disables the CRTC, amdgpu_dm_commit_audio() reads the -1 and
> unconditionally
> calls amdgpu_dm_audio_eld_notify() with the invalid pin:
>
> amdgpu_dm_commit_audio() {
> ...
> inst = aconnector->audio_inst;
> aconnector->audio_inst = -1;
> mutex_unlock(&adev->dm.audio_lock);
>
> amdgpu_dm_audio_eld_notify(adev, inst);
> }
>
> Does this violate the cross-subsystem API by logging a bogus debug
> message
> and passing an invalid index to the HDA driver?
Upon testing it looks like calling amdgpu_dm_audio_eld_notify() with -1
already happens today. Browsing through the code looks like this is
mostly handled as a no-op, except for nvhdmi_port2pin() and
atihdmi_port2pin() which should probably be patched to handle that
case. I can send a separate patch for that.
Thanks,
Nícolas
>
> > }
> >
> > update_subconnector_property(aconnector);
