Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Memory leak of the dynamically allocated edid buffer in ptn3460_edid_read on the success and drm_edid_alloc failure paths. - [High] Lockless concurrent access to the bridge's power state (ptn_bridge->enabled) causes a TOCTOU race and hardware state mismatch. -- --- Patch [14]: [PATCH v2 14/37] drm/bridge: nxp-ptn3460: Switch to atomic bridge callbacks --- commit 3ac8d8062ac1622a61e5a518f71eee0beb4385a3 Author: Maxime Ripard <[email protected]> drm/bridge: nxp-ptn3460: Switch to atomic bridge callbacks The nxp-ptn3460 bridge uses the legacy non-atomic pre_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/nxp-ptn3460.c > b/drivers/gpu/drm/bridge/nxp-ptn3460.c > index 7acb11f16dc19..a9b4ac2defc99 100644 > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c [ ... ] > @@ -163,7 +165,7 @@ static const struct drm_edid *ptn3460_edid_read(struct > drm_bridge *bridge, > int ret; > > power_off = !ptn_bridge->enabled; > - ptn3460_pre_enable(&ptn_bridge->bridge); > + ptn3460_pre_enable(&ptn_bridge->bridge, NULL); [Severity: High] This is a pre-existing issue, but does lockless access to ptn_bridge->enabled create a race condition with asynchronous atomic commits? If ptn3460_edid_read() starts while the display is off, it saves power_off = true. If an asynchronous DRM atomic commit worker simultaneously invokes ptn3460_pre_enable(), it sets ptn_bridge->enabled = true and powers up the hardware. > > edid = kmalloc(EDID_LENGTH, GFP_KERNEL); > if (!edid) { > @@ -182,7 +184,7 @@ static const struct drm_edid *ptn3460_edid_read(struct > drm_bridge *bridge, > > out: > if (power_off) > - ptn3460_disable(&ptn_bridge->bridge); > + ptn3460_disable(&ptn_bridge->bridge, NULL); [Severity: High] This isn't a bug introduced by this patch, but does this leak the dynamically allocated edid buffer? drm_edid_alloc() duplicates the data into a new container but does not take ownership of the original pointer. When ptn3460_edid_read() reaches this out block and returns, the edid buffer allocated via kmalloc() earlier is not freed. Since this path can be triggered repeatedly via the DRM_IOCTL_MODE_GETCONNECTOR ioctl, could this eventually lead to memory exhaustion? Additionally, following up on the race condition mentioned above, if power_off was evaluated as true before a concurrent atomic commit, this path will call ptn3460_disable(). That function locklessly modifies ptn_bridge->enabled = false and powers down the hardware, causing the display to go blank against the atomic commit's expectations. > > return drm_edid; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
