Re: [PATCH v3 12/20] mfd: tps6586: use platform_device_set_of_node()

[sashiko-bot@kernel.org]

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 <bartosz.golaszewski@oss.qualcomm.com>
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/20260706-pdev-fwnode-ref-v3-0-1ff028e33779@oss.qualcomm.com?part=12