[PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[Johan Jonker via B4 Relay]

From: Hüseyin BIYIK <boogiepop@gmx.com>

The function devm_drm_bridge_alloc() is the new API for allocating DRM bridges.
This conversion was missed during the initial conversion of all bridges to
the new API.

Signed-off-by: Hüseyin BIYIK <boogiepop@gmx.com>
Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 drivers/gpu/drm/rockchip/rk3066_hdmi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
index f28d6a883e09..39c65ee07caa 100644
--- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
+++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
@@ -703,7 +703,6 @@ rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi)
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
 	hdmi->bridge.driver_private = hdmi;
-	hdmi->bridge.funcs = &rk3066_hdmi_bridge_funcs;
 	hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT |
 			   DRM_BRIDGE_OP_EDID |
 			   DRM_BRIDGE_OP_HDMI |
@@ -747,9 +746,10 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
 	int irq;
 	int ret;
 
-	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
-	if (!hdmi)
-		return -ENOMEM;
+	hdmi = devm_drm_bridge_alloc(dev, struct rk3066_hdmi, bridge,
+				     &rk3066_hdmi_bridge_funcs);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
 
 	hdmi->dev = dev;
 	hdmi->drm_dev = drm;

---
base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
change-id: 20260824-drm-e4689f65ab8c

Best regards,
-- 
Johan Jonker <jbx6244@gmail.com>

Re: [PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a Use-After-Free vulnerability on the error path of `rk3066_hdmi_bind()`. If `rk3066_hdmi_register()` fails, the code jumps to `err_disable_hclk`, skipping the encoder cleanup label `err_cleanup_hdmi`. Since the DRM encoder was added to the DRM device's list during `drm_encoder_init()` but is not removed, and the `hdmi` struct containing the encoder is freed by devres upon bind failure, the DRM core retains a dangling pointer. This will cause a Use-After-Free when the DRM core later iterates over the encoder list or during teardown.
--

--- Patch [1]: [PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API ---
commit bdad67e77f8f693946bd674b1a40ce7af3ce54c3
Author: Hüseyin BIYIK <boogiepop@gmx.com>

drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

This commit converts the rk3066_hdmi driver to use devm_drm_bridge_alloc()
for allocating DRM bridges. This conversion was missed during the initial
conversion of all bridges to the new API.

> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> index f28d6a883e099..39c65ee07caac 100644
> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
[ ... ]
> @@ -747,9 +746,10 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
>  	int irq;
>  	int ret;
>  
> -	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi)
> -		return -ENOMEM;
> +	hdmi = devm_drm_bridge_alloc(dev, struct rk3066_hdmi, bridge,
> +				     &rk3066_hdmi_bridge_funcs);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the
allocation of the hdmi struct in rk3066_hdmi_bind() inside
drivers/gpu/drm/rockchip/rk3066_hdmi.c, I noticed a potential
use-after-free on the error path.

If rk3066_hdmi_register() fails after drm_encoder_init() has been called,
the error path in rk3066_hdmi_bind() jumps to err_disable_hclk:

	ret = rk3066_hdmi_register(drm, hdmi);
	if (ret)
		goto err_disable_hclk;

This bypasses the err_cleanup_hdmi label which destroys the encoder:

err_cleanup_hdmi:
	hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder);
err_disable_hclk:
	clk_disable_unprepare(hdmi->hclk);

Since the encoder was added to the DRM device's internal lists during
drm_encoder_init(), and the devm-allocated hdmi struct will be freed when
rk3066_hdmi_bind() returns an error, does this leave a dangling pointer in
the DRM core that will cause a use-after-free when the list is iterated or
torn down?

>  
>  	hdmi->dev = dev;
>  	hdmi->drm_dev = drm;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-drm-v2-1-7f9273c6020b@gmail.com?part=1

Re: [PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[Heiko Stübner]

Am Montag, 24. August 2026, 12:21:19 Mitteleuropäische Sommerzeit schrieb Johan Jonker via B4 Relay:
> From: Hüseyin BIYIK <boogiepop@gmx.com>
> 
> The function devm_drm_bridge_alloc() is the new API for allocating DRM bridges.
> This conversion was missed during the initial conversion of all bridges to
> the new API.
> 
> Signed-off-by: Hüseyin BIYIK <boogiepop@gmx.com>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/rk3066_hdmi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> index f28d6a883e09..39c65ee07caa 100644
> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> @@ -703,7 +703,6 @@ rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi)
>  			 DRM_MODE_ENCODER_TMDS, NULL);
>  
>  	hdmi->bridge.driver_private = hdmi;
> -	hdmi->bridge.funcs = &rk3066_hdmi_bridge_funcs;
>  	hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT |
>  			   DRM_BRIDGE_OP_EDID |
>  			   DRM_BRIDGE_OP_HDMI |
> @@ -747,9 +746,10 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
>  	int irq;
>  	int ret;
>  
> -	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi)
> -		return -ENOMEM;
> +	hdmi = devm_drm_bridge_alloc(dev, struct rk3066_hdmi, bridge,
> +				     &rk3066_hdmi_bridge_funcs);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);

If I'm reading things correctly, the parts in rk3066_hdmi_register setting
hdmi->bridge.driver_private (should use the container field of struct
drm_bridge instead) and hdmi->bridge.funcs are redundant by this change?


Heiko



>  
>  	hdmi->dev = dev;
>  	hdmi->drm_dev = drm;
> 
> ---
> base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
> change-id: 20260824-drm-e4689f65ab8c
> 
> Best regards,
> 

Re: [PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[Johan Jonker]

Hi,

On 9/3/26 17:12, Heiko Stübner wrote:
> Am Montag, 24. August 2026, 12:21:19 Mitteleuropäische Sommerzeit schrieb Johan Jonker via B4 Relay:
>> From: Hüseyin BIYIK <boogiepop@gmx.com>
>>
>> The function devm_drm_bridge_alloc() is the new API for allocating DRM bridges.
>> This conversion was missed during the initial conversion of all bridges to
>> the new API.
>>
>> Signed-off-by: Hüseyin BIYIK <boogiepop@gmx.com>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  drivers/gpu/drm/rockchip/rk3066_hdmi.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
>> index f28d6a883e09..39c65ee07caa 100644
>> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
>> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
>> @@ -703,7 +703,6 @@ rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi)
>>  			 DRM_MODE_ENCODER_TMDS, NULL);
>>  

>>  	hdmi->bridge.driver_private = hdmi;

Can this be removed?
See link/comment below.


>> -	hdmi->bridge.funcs = &rk3066_hdmi_bridge_funcs;
>>  	hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT |
>>  			   DRM_BRIDGE_OP_EDID |
>>  			   DRM_BRIDGE_OP_HDMI |
>> @@ -747,9 +746,10 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
>>  	int irq;
>>  	int ret;
>>  
>> -	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
>> -	if (!hdmi)
>> -		return -ENOMEM;
>> +	hdmi = devm_drm_bridge_alloc(dev, struct rk3066_hdmi, bridge,
>> +				     &rk3066_hdmi_bridge_funcs);
>> +	if (IS_ERR(hdmi))
>> +		return PTR_ERR(hdmi);
> 

> If I'm reading things correctly, the parts in rk3066_hdmi_register setting
> hdmi->bridge.driver_private (should use the container field of struct
> drm_bridge instead) 

The driver_private line was added with this patch:

[PATCH] drm/rockchip: rk3066_hdmi: switch to drm bridge
https://lore.kernel.org/all/20250428102309.1501986-1-andyshrk@163.com/

and hdmi->bridge.funcs are redundant by this change?

Not redundant, just funcs are attached in a common function __devm_drm_bridge_alloc()

https://elixir.bootlin.com/linux/v7.2.2/source/drivers/gpu/drm/drm_bridge.c#L383

void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
			      const struct drm_bridge_funcs *funcs)
{
	void *container;
	struct drm_bridge *bridge;
	int err;

	if (!funcs) {
		dev_warn(dev, "Missing funcs pointer\n");
		return ERR_PTR(-EINVAL);
	}

	container = kzalloc(size, GFP_KERNEL);
	if (!container)
		return ERR_PTR(-ENOMEM);

	bridge = container + offset;
	INIT_LIST_HEAD(&bridge->list);
	bridge->container = container;
	bridge->funcs = funcs;
	kref_init(&bridge->refcount);

	err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
	if (err)
		return ERR_PTR(err);

	return container;
}
EXPORT_SYMBOL(__devm_drm_bridge_alloc);

===========

Other example from sti:

https://lore.kernel.org/all/ce9c6aa3-5372-468f-a4bf-5a261259e459@samsung.com/

>>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?
>>> This looks correct to me. In current code, driver_private is used to
>>> hold a pointer to the driver private struct (struct
>>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
>>> enough, no pointer is needed. With the patch applied, driver_private
>>> becomes unused.
>> Then we should remove it from the structure if it's unused.


drm/sti: hdmi: convert to devm_drm_bridge_alloc() API
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4531424d907f3983e919a7bda2b90ea0cede4f

============

This serie was missing a few bridges too many.

drm: convert all bridges to devm_drm_bridge_alloc()
https://patchwork.freedesktop.org/series/148229/

====

Please advise what changes are needed?
RK3066_hdmi is broken since somewhere 2025-05.

Johan


> 
> 
> Heiko
> 
> 
> 
>>  
>>  	hdmi->dev = dev;
>>  	hdmi->drm_dev = drm;
>>
>> ---
>> base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
>> change-id: 20260824-drm-e4689f65ab8c
>>
>> Best regards,
>>
> 
> 
> 
> 

Re: [PATCH v2] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[Heiko Stübner]

Hi Johan,

Am Donnerstag, 3. September 2026, 22:44:57 Mitteleuropäische Sommerzeit schrieb Johan Jonker:
> On 9/3/26 17:12, Heiko Stübner wrote:
> > Am Montag, 24. August 2026, 12:21:19 Mitteleuropäische Sommerzeit schrieb Johan Jonker via B4 Relay:
> >> From: Hüseyin BIYIK <boogiepop@gmx.com>
> >>
> >> The function devm_drm_bridge_alloc() is the new API for allocating DRM bridges.
> >> This conversion was missed during the initial conversion of all bridges to
> >> the new API.
> >>
> >> Signed-off-by: Hüseyin BIYIK <boogiepop@gmx.com>
> >> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> >> ---
> >>  drivers/gpu/drm/rockchip/rk3066_hdmi.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> >> index f28d6a883e09..39c65ee07caa 100644
> >> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> >> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> >> @@ -703,7 +703,6 @@ rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi)
> >>  			 DRM_MODE_ENCODER_TMDS, NULL);
> >>  
> 
> >>  	hdmi->bridge.driver_private = hdmi;
> 
> Can this be removed?
> See link/comment below.

that is what I meant. bridge.container holds the same information and
bridge.private_data is not used in the rk3066_hdmi driver at all.

So if some future function needs to access the struct rk3066_hdmi it can
do so via the container field.


> and hdmi->bridge.funcs are redundant by this change?
> 
> Not redundant, just funcs are attached in a common function __devm_drm_bridge_alloc()

that's exactly wat I meant. As they are attached already in the common
function, there is no need to do it _again_ in rk3066_hdmi_register()

> 
> Please advise what changes are needed?
> RK3066_hdmi is broken since somewhere 2025-05.

So both should be dropped.

Heiko