Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Subdevice cleanup unconditionally unregisters all children, prematurely removing MFD cell devices and bypassing `mfd_remove_devices_fn()`. - [Medium] Manual assignment of `pdev->dev.platform_data` leads to invalid `kfree()`. - [Medium] Missing IRQ domain and IRQ descriptors cleanup on teardown and error paths. -- commit 0df04ec4b22ac2adb3b77bf67e6a1339f5fd4523 Author: Bartosz Golaszewski <[email protected]> mfd: tps6586: use platform_device_set_of_node() This commit encapsulates the assignment of the OF node for dynamically allocated platform devices by using the platform_device_set_of_node() helper. This change prepares for reworking the reference counting logic for platform devices. > diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c > index f5f8054466033..3cfd2f02b62f3 100644 > --- a/drivers/mfd/tps6586x.c > +++ b/drivers/mfd/tps6586x.c > @@ -397,7 +397,7 @@ static int tps6586x_add_subdevs(struct tps6586x *tps6586x, > > pdev->dev.parent = tps6586x->dev; > pdev->dev.platform_data = subdev->platform_data; [Severity: Medium] This is a pre-existing issue, but does this manual assignment of platform_data at line 399 in tps6586x_add_subdevs() cause problems during device release? When the platform device is unregistered, platform_device_release() unconditionally calls kfree(pdev->dev.platform_data). If subdev->platform_data was statically allocated or part of devm_kzalloc memory, could this corrupt the heap? > - pdev->dev.of_node = of_node_get(subdev->of_node); > + platform_device_set_of_node(pdev, subdev->of_node); > > ret = platform_device_add(pdev); > if (ret) { [Severity: High] This isn't a bug introduced by this patch, but does the cleanup in tps6586x_remove_subdevs() interfere with MFD core cleanup? drivers/mfd/tps6586x.c:tps6586x_remove_subdevs() { ... return device_for_each_child(tps6586x->dev, NULL, __remove_subdev); } drivers/mfd/tps6586x.c:__remove_subdev() { ... platform_device_unregister(to_platform_device(dev)); } Since MFD cells added by mfd_add_devices() are children of tps6586x->dev, this unconditional unregistering of all children appears to bypass mfd_remove_devices_fn(). Could blindly casting dev to struct platform_device also be unsafe if any child is not a platform device? [Severity: Medium] This is a pre-existing issue, but does the driver leak IRQ descriptors and the IRQ domain during removal or on error paths? In the removal path, tps6586x_i2c_remove() in drivers/mfd/tps6586x.c cleans up other resources but misses IRQ cleanup: drivers/mfd/tps6586x.c:tps6586x_i2c_remove() { struct tps6586x *tps6586x = i2c_get_clientdata(client); tps6586x_remove_subdevs(tps6586x); mfd_remove_devices(tps6586x->dev); } Additionally, if irq_domain_create_simple() or request_threaded_irq() fail in tps6586x_irq_init(): drivers/mfd/tps6586x.c:tps6586x_irq_init() { ... if (!tps6586x->irq_domain) { dev_err(tps6586x->dev, "Failed to create IRQ domain\n"); return -ENOMEM; } ... } The error paths exit without freeing the previously allocated descriptors or domain. Can this cause resource leakage on cold paths? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
