Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/bridge: NULL bridge->name causes drm_atomic_private_obj_init() failure - [Medium] drm/dp_tunnel: empty group->name duplicated in init_group() --
commit 4d9e18eb9a2836a0d646f48abba1d11dfdce7d31 Author: Maxime Ripard <[email protected]> drm/atomic: Add drm_private_obj name This commit adds a human-readable name field to drm_private_obj, allocated in drm_atomic_private_obj_init() and freed in drm_atomic_private_obj_fini(), and updates existing callers to pass a name. > diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c > b/drivers/gpu/drm/display/drm_dp_tunnel.c > index d29ad4116e641..391d643793eb3 100644 > --- a/drivers/gpu/drm/display/drm_dp_tunnel.c > +++ b/drivers/gpu/drm/display/drm_dp_tunnel.c > @@ -1732,6 +1732,7 @@ static bool init_group(struct drm_dp_tunnel_mgr *mgr, > struct drm_dp_tunnel_group > INIT_LIST_HEAD(&group->tunnels); > > drm_atomic_private_obj_init(mgr->dev, &group->base, > + group->name, > &tunnel_group_funcs); > > return true; [Severity: Medium] Is group->name populated when init_group() runs? In drm_dp_tunnel_mgr_create(), mgr->groups is zero-allocated via kzalloc_objs() immediately before init_group() is called on each element: drivers/gpu/drm/display/drm_dp_tunnel.c:drm_dp_tunnel_mgr_create() { mgr->groups = kzalloc_objs(*mgr->groups, max_group_count); ... for (i = 0; i < max_group_count; i++) { if (!init_group(mgr, &mgr->groups[i])) { } Because drm_atomic_private_obj_init() duplicates the passed string with kstrdup(), group->base.name is initialized to "" while group->name is only formatted later when a tunnel is added in lookup_or_alloc_group(): drivers/gpu/drm/display/drm_dp_tunnel.c:lookup_or_alloc_group() { ... snprintf(group->name, sizeof(group->name), "%d:%d:*", tunnel_group_drv_id(drv_group_id) & ((1 << DP_GROUP_ID_BITS) - 1), tunnel_group_id(drv_group_id) & ((1 << DP_USB4_DRIVER_ID_BITS) - 1)); } Won't this leave group->base.name permanently set to an empty string for every DP tunnel group? > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index e7f9e6d589272..ac65ac9416919 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -317,6 +317,7 @@ static void __drm_bridge_free(struct kref *kref) > > drm_bridge_put(bridge->next_bridge); > > + kfree(bridge->name); > kfree(bridge->container); > } > > @@ -418,6 +419,13 @@ void *__devm_drm_bridge_alloc(struct device *dev, size_t > size, size_t offset, > return ERR_PTR(-ENOMEM); > > bridge = container + offset; > + > + bridge->name = kasprintf(GFP_KERNEL, "bridge-%s", dev_name(dev)); > + if (!bridge->name) { > + kfree(container); > + return ERR_PTR(-ENOMEM); > + } > + > INIT_LIST_HEAD(&bridge->list); > bridge->container = container; > bridge->funcs = funcs; > @@ -649,7 +657,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct > drm_bridge *bridge, > goto err_reset_bridge; > } > > - drm_atomic_private_obj_init(bridge->dev, &bridge->base, > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, bridge->name, > &drm_bridge_priv_state_funcs); > > return 0; [Severity: High] What happens in drm_bridge_attach() if a bridge was not allocated via devm_drm_bridge_alloc() and therefore has bridge->name == NULL? For example, ingenic_drm_bind() allocates struct ingenic_drm_bridge via drmm_encoder_alloc() and attaches &ib->bridge: drivers/gpu/drm/ingenic/ingenic-drm-drv.c:ingenic_drm_bind() { ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder, NULL, DRM_MODE_ENCODER_DPI, NULL); ... ib->bridge.funcs = &ingenic_drm_bridge_funcs; ib->next_bridge = bridge; ret = drm_bridge_attach(encoder, &ib->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); } When bridge->name is NULL, kstrdup(name, GFP_KERNEL) in drm_atomic_private_obj_init() returns NULL and aborts with -ENOMEM before adding bridge->base to dev->mode_config.privobj_list or calling atomic_create_state() to initialize bridge->base.state: drivers/gpu/drm/drm_atomic.c:drm_atomic_private_obj_init() { obj->name = kstrdup(name, GFP_KERNEL); if (!obj->name) { ret = -ENOMEM; goto err_lock_fini; } } Because drm_bridge_attach() ignores the return value of drm_atomic_private_obj_init(), attach succeeds with bridge->base.state == NULL, causing subsequent atomic commits via drm_atomic_add_encoder_bridges() -> drm_atomic_get_bridge_state() -> drm_atomic_helper_bridge_duplicate_state() to trigger WARN_ON(!bridge->base.state) and fail with -ENOMEM. Can drm_atomic_private_obj_init() or drm_bridge_attach() handle a NULL bridge name gracefully? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
