Re: [PATCH v6 7/9] drm/vc4: use drmm_writeback_connector_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch converts the writeback connector to use DRM-managed initialization (`drmm_writeback_connector_init`) but leaves the manual cleanup in `vc4_txp_unbind`, causing a double-free and NULL pointer dereference during device teardown.
- [Medium] The commit message claims a change to encoder allocation that is entirely absent from the patch.
--

commit 2e74a8f54aeae971a4434e6be6dd5849a3cfa4ff
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/vc4: use drmm_writeback_connector_init()

The driver uses drm_writeback_connector_init() instead of its drmm
counterpart, but it doesn't perform the job queue cleanup. The
drmm_writeback_connector_init() function ensures the proper cleanup of
the job queue.

> Use drmm_plain_encoder_alloc() to allocate simple encoder and
> drmm_writeback_connector_init() in order to initialize writeback
> connector instance.

[Severity: Medium]
This isn't a bug, but the commit message mentions using
drmm_plain_encoder_alloc(), yet this function isn't used anywhere in the patch.

The encoder remains embedded in struct vc4_txp and continues to be initialized
via drmm_encoder_init() in vc4_txp_bind().

Could the commit message be updated to match the actual code?

> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 66b6f2acf8626..3fd89fccfa10a 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
[ ... ]
> @@ -602,10 +601,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  
>  	drm_connector_helper_add(&txp->connector.base,
>  				 &vc4_txp_connector_helper_funcs);
> -	ret = drm_writeback_connector_init_with_encoder(drm, &txp->connector,
> -							encoder,
> -							&vc4_txp_connector_funcs,
> -							drm_fmts, ARRAY_SIZE(drm_fmts));
> +	ret = drmm_writeback_connector_init(drm, &txp->connector,
> +					    &vc4_txp_connector_funcs,
> +					    encoder,
> +					    drm_fmts, ARRAY_SIZE(drm_fmts));

[Severity: High]
Since we are moving to drmm_writeback_connector_init(), which automatically
handles cleanup by registering a managed action, is it still safe to keep
the manual drm_connector_cleanup() call in vc4_txp_unbind()?

drivers/gpu/drm/vc4/vc4_txp.c:vc4_txp_unbind() {
	struct vc4_txp *txp = dev_get_drvdata(dev);

	drm_connector_cleanup(&txp->connector.base);
}

During teardown, vc4_txp_unbind() will manually clean up the connector,
but the drmm-managed action will later attempt to clean it up again when
the drm_device is released.

Does this cause a double-free or NULL pointer dereference on device teardown?

>  	if (ret)
>  		return ret;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-wb-drop-encoder-v6-0-9f3a54f81310@oss.qualcomm.com?part=7