Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Unconditional connector state allocation causes a memory leak if an HDMI bridge is present in the chain. Pre-existing issues: - [High] `devm_` allocations tied to the component device in `dw_dp_bind` cause resource leaks and permanent driver failure if `bind` fails. -- commit 47f0f730df97f46871d26a1858e6f2594ea7c383 Author: Igor Paunovic <[email protected]> drm/rockchip: dw_dp: Attach "max bpc" connector property This commit attaches the "max bpc" property to the dw-dp bridge connector with a range of 6 to 10. It also explicitly creates the connector state before drm_mode_config_reset() runs, allowing the property to be attached successfully during driver initialization. > diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > index b23efb153c9e6..8ff27353b1417 100644 > --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c [ ... ] > @@ -112,6 +112,32 @@ static int dw_dp_rockchip_bind(struct device *dev, > struct device *master, void * > "Failed to init bridge connector\n"); > } > > + /* > + * The components are bound before drm_mode_config_reset() runs, so > + * the connector has no state yet, but > + * drm_connector_attach_max_bpc_property() requires one. Create the > + * connector state first, like drmm_connector_hdmi_init() does. > + */ > + if (connector->funcs->atomic_create_state) { > + struct drm_connector_state *state; > + > + state = connector->funcs->atomic_create_state(connector); > + if (IS_ERR(state)) { > + dw_dp_unbind(dp->base); > + return PTR_ERR(state); > + } > + > + connector->state = state; [Severity: Medium] Does this unconditionally overwrite connector->state? If the display pipeline contains an HDMI bridge (e.g., a DP-to-HDMI chip), drm_bridge_connector_init() internally calls drmm_connector_hdmi_init(), which allocates and assigns connector->state. Overwriting it here seems to leak the previously allocated drm_connector_state. > + } else if (connector->funcs->reset) { > + connector->funcs->reset(connector); > + } > + > + ret = drm_connector_attach_max_bpc_property(connector, 6, 10); > + if (ret) { > + dw_dp_unbind(dp->base); > + return ret; [Severity: High] This isn't a regression introduced by this patch, but if this newly added error path is taken, does it permanently break the driver on deferred probe? The component bind callback invokes dw_dp_bind() earlier in this function, which performs devm_ allocations tied to the component device (dev) rather than the master device. For example: drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind() { ... ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq, IRQF_ONESHOT, dev_name(dev), dp); ... } Because the component device remains probed even if the bind callback fails, these devm_ resources are not freed when this error path returns. On a subsequent bind retry, dw_dp_bind() executes again. Since devm_request_threaded_irq() does not use IRQF_SHARED, it will return -EBUSY, permanently failing. > + } > + > return 0; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
