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

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

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Auxiliary device teardown via devres races with manual power off in remove(), allowing I/O to powered-down hardware.
> - [Low] Missing explicit `select AUXILIARY_BUS` in Kconfig for PCI_PWRCTRL_TC9563.
> --
> 
> commit 6f8a88de67f080d18050c0ce0599dc8854f71eab
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> PCI/pwrctrl: tc9563: Add GPIO auxiliary device support
>     
> This commit adds an auxiliary device to represent the GPIO controller
> embedded in the TC9563. It passes the regmap via platform data to enable
> the gpio-tc9563 driver to register the GPIO chip.
> 
> > diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig
> > --- a/drivers/pci/pwrctrl/Kconfig
> > +++ b/drivers/pci/pwrctrl/Kconfig
> > @@ -29,6 +29,7 @@ config PCI_PWRCTRL_TC9563
> >  	select PCI_PWRCTRL
> >  	default m if ARCH_QCOM
> >  	depends on I2C
> > +	depends on GPIO_TC9563
> >  	select REGMAP_I2C
> 
> [Severity: Low]
> Since the driver now directly calls auxiliary bus symbols, should this config
> explicitly select AUXILIARY_BUS to prevent potential build failures if
> transitive dependencies change?

AUXILIARY_BUS is selected by GPIO_TC9563

> 
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> [ ... ]
> > +static int tc9563_pwrctrl_adev_add(struct device *dev, const char *name,
> > +				   struct device_node *of_node,
> > +				   void *priv_data)
> > +{
> [ ... ]
> > +	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]
> Because the auxiliary device is queued for removal via devres, won't it be
> destroyed after the driver's manual remove function executes?
> 
> If the hardware is shut down and the adapter reference is dropped in the 
> driver's manual remove path before the devres callbacks run, couldn't this
> allow I/O to powered-down hardware if another process accesses the GPIO
> sysfs interface during this teardown window?

During driver teardown phase, the aux device is destroyed first and then tc9563
device is released.

Regards,
Lorenzo

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

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