Re: [PATCH RFC v2 02/24] drm/connector: add drm_connector_hdmi_dynamic_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/connector: resource leak on dynamic HDMI init failure due to dead code
- [Low] drm/connector: incorrect kernel-doc function name for drm_connector_hdmi_dynamic_init
--

commit 7682dffb42f83d7a2155ddd8d9ec5ae0bcb7425d
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/connector: add drm_connector_hdmi_dynamic_init()

In preparation for adding hotpluggable bridges into the
drm_bridge_connector, we need connectors to be created dynamically, both
regular connectors and HDMI ones. drm_connector_init() already has a
dynamic variant, add one for HDMI connectors too.

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index f5bd08640d251..f264cab0d184e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -708,6 +708,48 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drmm_connector_hdmi_init);
>  
> +/**
> + * drmm_connector_hdmi_init - Init a preallocated dynamic HDMI connector

[Severity: Low]
This isn't a bug, but the kernel-doc header lists the function name as
drmm_connector_hdmi_init instead of drm_connector_hdmi_dynamic_init. Could
this cause kernel-doc validation warnings?

> + * @dev: DRM device
> + * @connector: A pointer to the HDMI connector to init

[ ... ]

> +int drm_connector_hdmi_dynamic_init(struct drm_device *dev,
> +				    struct drm_connector *connector,
> +				    const struct drm_connector_funcs *funcs,
> +				    const struct drm_connector_hdmi_funcs *hdmi_funcs,
> +				    int connector_type,
> +				    struct i2c_adapter *ddc)
> +{
> +	int ret;
> +
> +	ret = drm_connector_hdmi_sanity_checks(dev, connector, hdmi_funcs, connector_type);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_connector_dynamic_init(dev, connector, funcs, connector_type, ddc);
> +	if (ret)
> +		return ret;
> +
> +	if (ret)
> +		drm_connector_cleanup(connector);

[Severity: Medium]
Is this second if (ret) check dead code?

Since ret would have caused an early return in the previous check, this
drm_connector_cleanup(connector) will never run. 

Does this leak resources (like the IDA allocation and connector name) if
drm_connector_hdmi_initialize() fails on the next line? Should the cleanup
instead happen on the return path if drm_connector_hdmi_initialize() 
encounters a failure?

> +
> +	return drm_connector_hdmi_initialize(dev, connector, hdmi_funcs);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_dynamic_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-drm-bridge-hotplug-v2-0-8e34986dcb68@bootlin.com?part=2