Re: [PATCH v9 05/10] drm/rockchip: analogix_dp: Enable hclk for RK3588

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] OF node reference leak from of_get_child_by_name().
- [High] Unbalanced clock disable in power down sequence due to ignored errors in power on sequence.
--

commit 0a8f503a077ab1566879dcba7ea0ac8ea7edf25c
Author: Damon Ding <damon.ding@rock-chips.com>

drm/rockchip: analogix_dp: Enable hclk for RK3588

Acquire and enable the HCLK_VO1 bus clock explicitly for the RK3588 eDP
controller. This replaces the implicit enable via the rockchip,vo-grf
phandle, aligning with updated device tree bindings and guaranteeing access.

> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 06072efd7fca3..d2af5eb29dbb5 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -311,6 +311,7 @@ static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
>  {
>  	struct device *dev = dp->dev;
>  	struct device_node *np = dev->of_node;
> +	struct clk *clk;
>  
>  	dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
>  	if (IS_ERR(dp->grf))
> @@ -327,6 +328,11 @@ static int rockchip_dp_of_probe(struct rockchip_dp_device *dp)
>  		return dev_err_probe(dev, PTR_ERR(dp->pclk),
>  				     "failed to get pclk property\n");
>  
> +	clk = devm_clk_get_optional_enabled(dev, "hclk");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "failed to get hclk property\n");
> +
>  	dp->rst = devm_reset_control_get(dev, "dp");
>  	if (IS_ERR(dp->rst))
>  		return dev_err_probe(dev, PTR_ERR(dp->rst),

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the clock
management in this file, I noticed a potential issue with the power-on error
handling.

If rockchip_dp_pre_init() fails in rockchip_dp_poweron():

static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
{
	...
	ret = rockchip_dp_pre_init(dp);
	if (ret < 0) {
		DRM_DEV_ERROR(dp->dev, "failed to dp pre init %d\n", ret);
		clk_disable_unprepare(dp->pclk);
		return ret;
	}

The core caller analogix_dp_resume() ignores the return value of power_on():

int analogix_dp_resume(struct analogix_dp_device *dp)
{
	...
	if (dp->plat_data->power_on)
		dp->plat_data->power_on(dp->plat_data);

When the device is later suspended, rockchip_dp_powerdown() unconditionally
disables the clock:

static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
{
	...
	clk_disable_unprepare(dp->pclk);

Since the clock framework tracks enable counts globally per clock core, will
this unbalanced disable decrement the enable_count of the shared clock?
Could this unexpectedly disable the clock for other peripherals and cause
hardware lockups?

[Severity: High]
This is a pre-existing issue, but I also noticed a reference leak in
rockchip_dp_drm_encoder_enable():

static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
					   struct drm_atomic_commit *state)
{
	...
	remote_port_parent = of_graph_get_remote_port_parent(endpoint.local_node);
	if (remote_port_parent) {
		if (of_get_child_by_name(remote_port_parent, "ports")) {
			remote_port = of_graph_get_remote_port(endpoint.local_node);

Since of_get_child_by_name() returns a device node with its reference
count incremented and requires a matching of_node_put(), does using the
returned pointer directly as a boolean condition cause the pointer to be
discarded? Does this leak the node reference every time the encoder is
enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527024336.191433-1-damon.ding@rock-chips.com?part=5