Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Sleeping functions called in atomic context during bridge enable - [High] Early returns in `atomic_enable` cause unbalanced PM counter and unpowered hardware access in `atomic_disable` --
commit 0eae00dcaa6988008a1dca84233d19bc97469ed0 Author: Biju Das <[email protected]> drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder Adds support for the RZ/G3L LVDS encoder driver operating in single-link mode with 4 lanes. It supports pixel clocks from 25 to 87 MHz and shares peripherals with MIPI-DSI. > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c > b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c > new file mode 100644 > index 0000000000000..6100888ea7284 > --- /dev/null > +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c [ ... ] > @@ -0,0 +1,285 @@ > +static void rzg3l_lvds_atomic_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ > + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); > + const struct drm_bridge_state *bridge_state; > + u32 fmt; > + > + /* Get the LVDS format from the bridge state. */ > + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); > + if (WARN_ON(!bridge_state)) > + return; [Severity: High] If drm_atomic_get_new_bridge_state() returns NULL and takes this early return, will rzg3l_lvds_atomic_disable() later execute unconditionally and access unpowered hardware registers or cause a runtime PM usage counter underflow? Since atomic_enable returns void, the DRM core might still consider the bridge enabled. > + > + switch (bridge_state->output_bus_cfg.format) { > + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: > + fmt = RZG3L_LVDS_MODE_JEIDA; > + break; > + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: > + fmt = RZG3L_LVDS_MODE_VESA; > + break; > + default: > + fmt = RZG3L_LVDS_MODE_VESA; > + dev_warn(lvds->dev, "Unsupported bus fmt 0x%04x\n", > + bridge_state->output_bus_cfg.format); > + break; > + } > + > + if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) > + return; [Severity: High] Similarly, if pm_runtime_get_sync() fails, returning early here leaves the device unpowered. When rzg3l_lvds_atomic_disable() is called during teardown, could this lead to a bus fault from accessing unpowered MMIO registers and an unbalanced pm_runtime_put() counter? [Severity: High] Also, since atomic_enable callbacks run in an atomic commit context where sleeping is forbidden, is it safe to use pm_runtime_get_sync() here? This function blocks and sleeps while waiting for the device to resume, which might cause a scheduling while atomic regression. > + > + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, > + LVDS_0_PHY_CH_EN_BGR, LVDS_0_PHY_CH_EN_BGR); > + fsleep(20); > + > + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, > + LVDS_0_PHY_CH_EN_LDO, LVDS_0_PHY_CH_EN_LDO); > + fsleep(10); [Severity: High] Because this is an atomic commit path, delay functions that can sleep like fsleep() are unsafe. Could these fsleep() calls (which map to usleep_range() or msleep()) lead to scheduling while atomic bugs? Would udelay() or ndelay() be more appropriate for these hardware delays? > + > + regmap_write(lvds->regmap, LVDS_CMN, LVDS_CMN_RST_PHY0_SEL); > + regmap_update_bits(lvds->regmap, LVDS_0_CTL_OFFSET, > + LVDS_0_CTL_FMT_SEL0_MSK, > + FIELD_PREP(LVDS_0_CTL_FMT_SEL0_MSK, fmt)); > + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, > + LVDS_0_PHY_CH_IO_EN0_MSK, LVDS_0_PHY_CH_IO_EN0); > + regmap_write(lvds->regmap, LVDS_CMN, > + LVDS_CMN_RST_PHY0_SEL | LVDS_CMN_PHY_RESET); > + fsleep(100); [Severity: High] Could this fsleep(100) call also sleep in this atomic context? > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
