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

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

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

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] The GPIO auxiliary device is unconditionally registered without runtime PM synchronization with the parent device, exposing unpowered hardware to I2C accesses.
> - [High] Hardcoded auxiliary device ID causes a sysfs naming collision, breaking support for multiple TC9563 controllers.
> 
> Pre-existing issues:
> - [High] Inversion of teardown order between the I2C adapter reference and devm-managed resources.
> --
> 
> commit 4ceb9152c94ce43b58577ae6c0490e74f1fcf9de
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> PCI/pwrctrl: tc9563: Add GPIO auxiliary device support
> 
> The TC9563 embeds a GPIO controller used for per-port reset signals.
> Create an auxiliary device for it so the gpio-tc9563 driver can
> register the GPIO chip and enable DT-based GPIO lookups.
> 
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > index 59ad219c26c02..2230765950b88 100644
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> [ ... ]
> > @@ -393,6 +395,69 @@ static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
> [ ... ]
> > +static int tc9563_pwrctrl_add_gpio_adev(struct tc9563_pwrctrl *tc9563)
> > +{
> > +	struct device *dev = tc9563->pwrctrl.dev;
> > +
> > +	if (!of_property_read_bool(dev->of_node, "gpio-controller") ||
> > +	    !of_property_present(dev->of_node, "#gpio-cells"))
> > +		return 0;
> > +
> > +	return tc9563_pwrctrl_adev_add(dev, TC9563_GPIO_DEV_NAME, 0,
> > +				       dev->of_node, tc9563->regmap);
>                                                                   ^
> [Severity: High]
> Does this hardcoded auxiliary device ID of 0 in
> tc9563_pwrctrl_add_gpio_adev() (in drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c)
> cause a sysfs naming collision?
> 
> If there is more than one TC9563 controller in the system, the first
> will probe and register successfully. When the second probes, won't it
> attempt to register the same name and fail with -EEXIST?

ack, I will fix it in v4.

> 
> [Severity: High]
> Will unconditionally registering the GPIO auxiliary device here expose
> it to accesses while the parent device is suspended?
> 
> The PCI subsystem manages the hardware's power state via the pwrctrl
> callbacks, powering it off when the PCI endpoint suspends. However, the
> GPIO auxiliary device remains registered and active without runtime PM
> synchronization.
> 
> When userspace or another subsystem accesses a GPIO via sysfs or libgpiod
> while the PCI link is suspended, wouldn't the resulting I2C operations
> target an unpowered chip?

I guess this is not a real issue since if pwrctrl driver runs the power_off()
callback, it will disable device regulators and so the i2c communications
triggered by gpio read/write will fail returning an error.

> 
> > +}
> > +
> [ ... ]
> > @@ -596,6 +661,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;
> > +
> 
> [Severity: High]
> This is a pre-existing issue, but could dropping the I2C adapter
> reference manually here and in tc9563_pwrctrl_remove() create a
> use-after-free risk during devm teardown?

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.

Regards,
Lorenzo

> 
> The driver acquires the I2C adapter via of_get_i2c_adapter_by_node() in
> tc9563_pwrctrl_probe() (in drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c).
> In the probe error path (and similarly in tc9563_pwrctrl_remove() during
> unbind), i2c_put_adapter() is called manually before the devm teardown
> runs.
> 
> With this patch adding the auxiliary device via devm, devm will
> unregister the GPIO auxiliary device after the adapter reference is
> dropped. If the GPIO driver performs any I2C transfers during its
> teardown, it will access the I2C adapter after its reference has been
> released.



> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904-pci-tc9563-aux-v3-0-5b1449d62ba2@oss.qualcomm.com?part=3

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