From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down Date: Sat, 16 May 2026 13:45:36 +1000 Message-ID: In-Reply-To: <20260512103153.8861-1-rmxpzlb@gmail.com> References: <20260512103153.8861-1-rmxpzlb@gmail.com> <20260512103153.8861-1-rmxpzlb@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Commit message:** Well-written. Includes the panic trace, clear root caus= e analysis, and describes the three logical changes (guard, decouple, balan= ce AMD bit). The Fixes tag and Cc: stable are appropriate. **Change 1 =E2=80=94 Guard in `clear_audio_infoframe` (lines 889-893):** ```c if (hdmi->tmds_char_rate) dw_hdmi_qp_mod(hdmi, 0, PKTSCHED_ACR_TX_EN | PKTSCHED_AMD_TX_EN | PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN, PKTSCHED_PKT_EN); ``` This is the core fix. When `tmds_char_rate =3D=3D 0`, the PHY is down and r= egister access is unsafe, so we skip it. The function still returns 0, whic= h is fine =E2=80=94 the framework doesn't act on error returns from `clear_= infoframe` callbacks, and there's nothing useful to do when the PHY is alre= ady off. No locking concern: `tmds_char_rate` is set to 0 in `atomic_disable` and th= e audio shutdown path is the only late caller, so reading it without a lock= is acceptable for this guard-check pattern (a stale non-zero read would ju= st do a harmless register write before the PHY goes down). **Change 2 =E2=80=94 Decouple `write_audio_infoframe` from `clear_audio_inf= oframe` (lines 992-995):** ```c dw_hdmi_qp_mod(hdmi, 0, PKTSCHED_ACR_TX_EN | PKTSCHED_AMD_TX_EN | PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN, PKTSCHED_PKT_EN); ``` Previously `write_audio_infoframe` called `clear_audio_infoframe` to disabl= e packets before re-programming them. Now it inlines the `dw_hdmi_qp_mod` c= all directly, avoiding the `tmds_char_rate` guard that would be incorrect i= n the write path (where the PHY **is** up). This is the right design =E2=80= =94 the write path is only called when audio is being configured, so the PH= Y must be active. All other `write_*_infoframe` functions (AVI, HDMI, HDR_D= RM, SPD) also call their corresponding clear functions directly without a g= uard, so this is consistent in spirit even though the implementation is now= inlined rather than calling through. **Change 3 =E2=80=94 Adding `PKTSCHED_AMD_TX_EN` to the clear mask:** The original `clear_audio_infoframe` cleared `ACR`, `AUDS`, and `AUDI` but = not `AMD`. However, `write_audio_infoframe` enables all four (`ACR`, `AUDI`= , `AMD` at line 1014, plus `AUDS` at line 1019). The clear path was missing= the `AMD` disable, meaning repeated write/clear cycles would leave `PKTSCH= ED_AMD_TX_EN` set. Adding it to both the clear function and the inline clea= r in write restores proper enable/disable balance. This is a genuine second= ary bug fix. **Minor observations:** 1. **Consistency with other clear functions:** As noted above, `clear_avi_i= nfoframe`, `clear_hdmi_infoframe`, `clear_hdr_drm_infoframe`, and `clear_sp= d_infoframe` don't guard on `tmds_char_rate`. If those can ever be called a= fter PHY shutdown (e.g., through framework infoframe management during conn= ector teardown), they would have the same crash. The author might want to i= nvestigate whether follow-up patches are needed there, though this is out o= f scope for the immediate crash fix. 2. **Race window:** There's a theoretical race where `atomic_disable` sets = `tmds_char_rate =3D 0` and then disables the PHY, and between those two ope= rations another CPU could read `tmds_char_rate !=3D 0` and proceed with the= register write. However, `tmds_char_rate` is set to 0 **before** `phy.ops-= >disable()` at lines 792-794, so the window is actually safe =E2=80=94 the = PHY is still up when the rate is zeroed. The truly dangerous case (PHY alre= ady down, rate not yet zeroed) cannot happen with the current ordering. **Overall: Clean, well-scoped fix. The three changes are logically coherent= and the commit message explains the rationale clearly. Recommend acceptanc= e.** --- Generated by Claude Code Patch Reviewer