Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The panel is enabled before it is prepared (powered on) in the bridge enable sequence, violating the DRM panel API contracts and causing potential hardware issues. - [High] Bridge `atomic_enable` and `atomic_disable` callbacks invoke sleeping functions, violating atomic context constraints. -- --- Patch [2]: [PATCH v2 02/37] drm/bridge: analogix-anx6345: Switch to atomic bridge callbacks --- commit c3398824f07e9152c87164f9494fed0231949fb3 Author: Maxime Ripard <[email protected]> drm/bridge: analogix-anx6345: Switch to atomic bridge callbacks The analogix-anx6345 bridge uses the legacy non-atomic enable and disable bridge callbacks. In order to remove the legacy bridge callback support from the DRM bridge core, switch to their atomic counterparts and add the bridge atomic state handlers. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c > b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c > index f3fe47b12edca..eba5c6dcb5ada 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c [ ... ] > @@ -580,7 +581,8 @@ static void anx6345_bridge_disable(struct drm_bridge > *bridge) > anx6345_poweroff(anx6345); > } > > -static void anx6345_bridge_enable(struct drm_bridge *bridge) > +static void anx6345_bridge_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *commit) > { > struct anx6345 *anx6345 = bridge_to_anx6345(bridge); > int err; > @@ -600,11 +602,14 @@ static void anx6345_bridge_enable(struct drm_bridge > *bridge) [Severity: High] This is a pre-existing issue, but does this function enable the panel before it is prepared? In anx6345_bridge_enable(), we have: if (anx6345->panel) drm_panel_enable(anx6345->panel); err = anx6345_start(anx6345); And anx6345_start() eventually calls anx6345_poweron() which prepares the panel: if (anx6345->panel) drm_panel_prepare(anx6345->panel); On a secondary modeset (after the device has been disabled), anx6345->powered is false. anx6345_bridge_enable() calls drm_panel_enable() which turns on the backlight. It then calls anx6345_start() which invokes anx6345_poweron() and drm_panel_prepare() to power on the LCD logic. Could this inverted enable -> prepare sequence turn on the backlight while the panel power is off, potentially causing latch-up and physically damaging the LCD logic on some displays? > } > > static const struct drm_bridge_funcs anx6345_bridge_funcs = { > + .atomic_create_state = drm_atomic_helper_bridge_create_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .attach = anx6345_bridge_attach, > .detach = anx6345_bridge_detach, > .mode_valid = anx6345_bridge_mode_valid, > - .disable = anx6345_bridge_disable, > - .enable = anx6345_bridge_enable, > + .atomic_disable = anx6345_bridge_disable, > + .atomic_enable = anx6345_bridge_enable, [Severity: High] This is a pre-existing issue, but does mapping these callbacks to atomic contexts conflict with the sleeping functions they invoke? The bridge atomic_enable and atomic_disable callbacks execute in the context of atomic commits (e.g., drm_atomic_helper_commit_tail), where sleeping is forbidden. However, anx6345_bridge_enable() and anx6345_bridge_disable() invoke hardware sequencing functions like anx6345_start(), anx6345_poweron(), and anx6345_poweroff(). For example, anx6345_start() contains: usleep_range(10000, 15000); And anx6345_poweron() contains: gpiod_set_value_cansleep(anx6345->gpiod_reset, 1); usleep_range(1000, 2000); Could calling these sleeping functions from the atomic context cause kernel panics or BUG warnings (scheduling while atomic)? > }; > > static void unregister_i2c_dummy_clients(struct anx6345 *anx6345) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
