Re: [PATCH v2 14/37] drm/bridge: nxp-ptn3460: Switch to atomic bridge callbacks

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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/20260707-drm-all-atomic-bridges-v2-0-21d03cbca446@kernel.org?part=14