Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The patch fails to implement its intended behavior because it uses `WARN_ON()` as a standalone statement without an early return. `WARN_ON()` evaluates the condition and emits a warning traceback but does not halt or return from execution. As a result, the operation is not disallowed, and execution falls through, continuing to overwrite the firmware node and leaving the resource leak unaddressed. --
--- Patch [1]: [PATCH v8 1/5] driver core: platform: amend the API contract for fwnode setters --- commit 0ad30205c2e10306a33e3a3823c155c95eca3ce4 Author: Bartosz Golaszewski <[email protected]> driver core: platform: amend the API contract for fwnode setters Calling platform_device_set_fwnode() for a dynamically allocated platform device whose primary firmware node is already a software node leads to a resource leak. This commit intends to disallow this scenario and warn loudly if it ever occurs. > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index a71015f1d9153..fb57d487a6c2a 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c [ ... ] > @@ -723,11 +725,20 @@ EXPORT_SYMBOL_GPL(platform_device_set_of_node); > * > * Assign a firmware node to this platform device. Internally keep track of > the > * reference count. Devices created with platform_device_alloc() must use > this > - * function instead of assigning the node manually. > + * function instead of assigning the node manually. This function must not be > + * called for a platform device that already has a software node as its > primary > + * firmware node assigned. > */ > void platform_device_set_fwnode(struct platform_device *pdev, > struct fwnode_handle *fwnode) > { > + /* > + * If we call this function for a platform device whose primary > + * firmware node is a software node, we'll never end up calling the > + * symmetric software_node_notify_remove(). There are no users for this > + * right now in the tree so just disallow it. > + */ > + WARN_ON(is_software_node(dev_fwnode(&pdev->dev))); [Severity: Medium] Does this code actually disallow the operation? Since WARN_ON() evaluates the condition and emits a warning but does not return from the function, execution will fall through and still overwrite the node, leaving the resource leak unaddressed. Would it be better to use an early return here, such as: if (WARN_ON(is_software_node(dev_fwnode(&pdev->dev)))) return; > fwnode_handle_put(pdev->dev.fwnode); > device_set_node(&pdev->dev, fwnode_handle_get(fwnode)); > } [ ... ] > @@ -739,11 +750,15 @@ EXPORT_SYMBOL_GPL(platform_device_set_fwnode); > * @dev2: device whose OF node to reuse > * > * Reuses the OF node of another device in this platform device while > - * internally keeping track of reference counting. > + * internally keeping track of reference counting. This function must not be > + * called for a platform device that already has a software node as its > primary > + * firmware node assigned. > */ > void platform_device_set_of_node_from_dev(struct platform_device *pdev, > const struct device *dev2) > { > + /* See platform_device_set_fwnode(). */ > + WARN_ON(is_software_node(dev_fwnode(&pdev->dev))); [Severity: Medium] Similar to platform_device_set_fwnode(), does this prevent the overwrite? A standalone WARN_ON() allows execution to continue and overwrite pdev->dev.fwnode. > device_set_of_node_from_dev(&pdev->dev, dev2); > pdev->dev.fwnode = of_fwnode_handle(pdev->dev.of_node); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260716-swnode-remove-on-dev-unreg-v8-0-5c2b8cc38...@oss.qualcomm.com?part=1
