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 <[email protected]> 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/[email protected]?part=7
