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

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 [off-list ref]

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.

[ ... ]

[ ... ]

+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?

[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?

+}
+

[ ... ]

[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?

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