Re: [PATCH v4 3/5] PCI/pwrctrl: tc9563: Add GPIO auxiliary device support

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 4157 bytes --]

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Devm vs manual cleanup ordering bug: Auxiliary device is unregistered via devres after the hardware is manually powered off and the I2C adapter reference is dropped.
> --
> 
> commit ee51cc5ce17159f5e9569310449014a414a8580e
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> PCI/pwrctrl: tc9563: Add GPIO auxiliary device support
>     
> The commit adds GPIO auxiliary device support to the TC9563 PCI power control
> driver, allowing a companion GPIO driver to handle per-port reset signals.
> It passes the tc9563 regmap to the auxiliary device and uses devres to
> manage unregistration.
> 
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > index 59ad219c26c02..3fb862105fa58 100644
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> [ ... ]
> > @@ -393,6 +398,77 @@ static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
> >  	return 0;
> >  }
> >  
> > +static void tc9563_pwrctrl_adev_release(struct device *dev)
> [ ... ]
> > +static int tc9563_pwrctrl_adev_add(struct device *dev, const char *name,
> > +				   struct device_node *of_node,
> > +				   void *priv_data)
> > +{
> > +	struct auxiliary_device *adev;
> [ ... ]
> > +	ret = auxiliary_device_add(adev);
> > +	if (ret) {
> > +		auxiliary_device_uninit(adev);
> > +		return ret;
> > +	}
> > +
> > +	return devm_add_action_or_reset(dev, tc9563_pwrctrl_adev_remove, adev);
> 
> [Severity: High]
> Does registering the auxiliary device unregistration to the devm cleanup phase
> cause a teardown ordering issue?
> 
> Because devm actions execute after .remove() returns, tc9563_pwrctrl_remove()
> will execute first:
> 
> static void tc9563_pwrctrl_remove(struct platform_device *pdev)
> {
> 	...
> 	tc9563_pwrctrl_power_off(&tc9563->pwrctrl);
> 	i2c_put_adapter(tc9563->adapter);
> }
> 
> Will this cause the hardware to be manually powered off and the I2C adapter
> reference dropped before the child device is unregistered?
> 
> Could this lead to I2C transactions on a powered-off device if the GPIO
> controller is accessed during the unbind window, and a potential use-after-free
> if the adapter module is unloaded while the dummy client and regmap are still
> active?

I think this is the issue already reported in v3.
I guess this can't really occur since i2c_adapter refcount is 2 after probe
(of_get_i2c_adapter_by_node()) and devm_i2c_new_dummy_device()). During
teardown phase, the first reference is released (i2c_put_adapter()) but the
second one is released just after aux device is destroyed.

> 
> > +}
> [ ... ]
> > @@ -596,6 +672,10 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
> >  	tc9563->pwrctrl.power_on = tc9563_pwrctrl_power_on;
> >  	tc9563->pwrctrl.power_off = tc9563_pwrctrl_power_off;
> >  
> > +	ret = tc9563_pwrctrl_add_gpio_adev(tc9563);
> > +	if (ret)
> > +		goto remove_i2c;
> > +
> >  	ret = devm_pci_pwrctrl_device_set_ready(dev, &tc9563->pwrctrl);
> >  	if (ret)
> >  		goto power_off;
> >  
> >  	return 0;
> >  
> >  power_off:
> >  	tc9563_pwrctrl_power_off(&tc9563->pwrctrl);
> >  remove_i2c:
> >  	i2c_put_adapter(tc9563->adapter);
> >  	return ret;
> 
> [Severity: High]
> Is the same teardown ordering regression reproduced in this error path?
> 
> If devm_pci_pwrctrl_device_set_ready() fails, the code jumps to power_off,
> which manually powers off the hardware and drops the I2C adapter reference.
> Then probe returns the error, and devres will clean up the auxiliary device
> afterward.
> 
> Could this create a race window where the child device is fully bound
> and accessible to userspace or other subsystems while the underlying hardware is
> already powered off?

For the above reason I guess this issue can't occur.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260908-pci-tc9563-aux-v4-0-c7fc7778292b@oss.qualcomm.com?part=3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]