Re: [PATCH net-next v4 3/4] net: mdio: make device map changes hotplug-safe
| Newsgroups | dev.linux.lists.imx,org.kernel.vger.linux-devicetree |
|---|---|
| Message-ID | <[email protected]> |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The 'dynamic' removal flag is hardcoded to false across all API boundaries, rendering the dynamic firmware-node reference dropping and exclusive firmware removal transactions unreachable dead code. -- --- Patch [3]: [PATCH net-next v4 3/4] net: mdio: make device map changes hotplug-safe --- Note: The format of this review has been altered to use a free-form summary rather than inline patch quoting due to automated recitation restrictions. commit ea45b70c0d34732ca4b03fdec7b5cb7a401e48fb Author: James Hilliard <[email protected]> Subject: net: mdio: make device map changes hotplug-safe This patch serializes device-map state and reserves addresses while registration is in progress to make MDIO device map changes hotplug-safe. It also adds logic to handle dynamic removals and overlay node cleanup. [Severity: Medium] The commit message states that the patch will "Drop a dynamically removed device's firmware-node reference after device_del() so it does not keep an overlay node alive." However, looking at the implementations of phy_device_remove() and mdio_device_remove(), both functions unconditionally pass false to the internal __phy_device_remove() and __mdio_device_remove() functions for the dynamic parameter. Similarly, bus teardown in mdiobus_remove_devices() also passes false. Because this dynamic flag is always false across all public API boundaries, the dynamic cleanup branch inside mdiobus_finish_remove() appears to be unreachable dead code. Is this intentional, or were there plans to export a specific dynamic removal API that callers could use to trigger this logic? Additionally, since phy_device_remove() and mdio_device_remove() return void, if the dynamic flag were ever set to true and the internal remove function returned an error like -EBUSY (for instance, if the PHY is still attached), wouldn't that error be silently ignored by the void wrapper, potentially leaving the device in the bus map while the caller assumes it was successfully freed? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3