Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/rockchip: wrong unlocked list in rockchip_drm_match_remove() --
commit e68a6141bb89fc9c57189db5bdc14a46b7da846f Author: Wentao Liang <[email protected]> drm/rockchip: use the safe iterator when removing device links This commit changes rockchip_drm_match_remove() to use list_for_each_entry_safe() when walking dev->links.consumers and calling device_link_del(). > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index 3099408e9d050..9edfe4db93934 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -356,9 +356,9 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node > *ep) > > static void rockchip_drm_match_remove(struct device *dev) > { > - struct device_link *link; > + struct device_link *link, *tmp; > > - list_for_each_entry(link, &dev->links.consumers, s_node) > + list_for_each_entry_safe(link, tmp, &dev->links.consumers, s_node) > device_link_del(link); [Severity: High] Does walking dev->links.consumers in rockchip_drm_match_remove() actually remove the stateless device links created by rockchip_drm_match_add(), or does it walk the wrong list without holding device_links_lock? In rockchip_drm_match_add(), the links are created with the master device dev as the consumer and each sub-driver device d as the supplier: device_link_add(dev, d, DL_FLAG_STATELESS); In drivers/base/core.c:device_link_add(), s_node is added to supplier->links.consumers (which is d->links.consumers) and c_node is added to consumer->links.suppliers (which is dev->links.suppliers): list_add_tail_rcu(&link->s_node, &supplier->links.consumers); list_add_tail_rcu(&link->c_node, &consumer->links.suppliers); Because dev is the consumer rather than the supplier, none of the links created by rockchip_drm_match_add() are on dev->links.consumers, so they are all leaked when rockchip_drm_platform_probe() fails or defers and when rockchip_drm_platform_remove() runs. In addition, dev->links.consumers and dev->links.suppliers are internal to the driver core and protected by device_links_write_lock(). Because device_link_del() only acquires and releases that lock inside each call, iterating dev->links outside drivers/base/core.c leaves the list traversal and tmp pointer unserialized against concurrent device link changes. If any managed links (such as fw_devlink links) are present on the list, calling device_link_del() on them will also trigger the WARN in device_link_put_kref(): static void device_link_put_kref(struct device_link *link) { if (device_link_test(link, DL_FLAG_STATELESS)) kref_put(&link->kref, __device_link_del); else if (!device_is_registered(link->consumer)) __device_link_del(&link->kref); else WARN(1, "Unable to drop a managed device link reference\n"); } Should rockchip_drm_match_remove() instead iterate the sub-driver devices and call the exported device_link_remove(dev, d) helper (or track the struct device_link pointers returned by device_link_add())? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
