Re: [PATCH net-next 09/12] gpio: tc956x: add TC956x/QPS615 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Function signature mismatch for `tc956x_gpio_set` causing build failures.
- [High] Sleeping in atomic context bug on PREEMPT_RT due to `can_sleep = false`.
- [Medium] Uninitialized stack variable used if `regmap_read()` fails.
- [Medium] Ignored `regmap_update_bits()` return value can cause incorrect hardware state.
- [Low] Kconfig depends on a non-existent symbol.
--

commit ff81a192de0fcb35d3c4b8a32cad777cd0bff29e
Author: Alex Elder <elder@riscstar.com>

gpio: tc956x: add TC956x/QPS615 support

This commit adds support for the GPIO controller embedded in the Toshiba
TC956x Ethernet-AVB/TSN bridge. It implements an auxiliary device driver
for the GPIO block using regmap.

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
[ ... ]
> +config GPIO_TC956X
> +	tristate "Toshiba TC956X GPIO support"
> +	depends on TOSHIBA_TC956X_PCI

Does TOSHIBA_TC956X_PCI exist in the tree at this point? Reference to a
non-existent config symbol causes silent build failures. If it is added
later in the patch series, this temporarily leaves the driver unselectable
and breaks bisectability.

[ ... ]
> diff --git a/drivers/gpio/gpio-tc956x.c b/drivers/gpio/gpio-tc956x.c
> --- /dev/null
> +++ b/drivers/gpio/gpio-tc956x.c
[ ... ]
> +static int tc956x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> +	u32 reg;
> +	u32 val;
> +
> +	if (test_bit(offset, gpio->input_only))
> +		return GPIO_LINE_DIRECTION_IN;
> +
> +	reg = offset < 32 ? GPIO_EN0_OFFSET : GPIO_EN1_OFFSET;
> +
> +	regmap_read(gpio->regmap, reg, &val);

If regmap_read() fails, does it leave the stack variable val uninitialized?
Should the return value be checked before evaluating val in the next line?

> +	if (val & BIT(offset % 32))
> +		return GPIO_LINE_DIRECTION_IN;
> +
> +	return GPIO_LINE_DIRECTION_OUT;
> +}
[ ... ]
> +static int tc956x_gpio_direction_output(struct gpio_chip *gc,
> +					unsigned int offset, int value)
> +{
> +	struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> +	u32 vreg;
> +	u32 dreg;
> +	u32 mask;
[ ... ]
> +	/* Set output value first, then direction */
> +	regmap_update_bits(gpio->regmap, vreg, mask, value ? mask : 0);

If this first regmap_update_bits() fails, should the function bail out
early? Ignoring the error and proceeding to change the direction to output
anyway could actively drive an unintended logic level onto the pin.

> +
> +	return regmap_update_bits(gpio->regmap, dreg, mask, 0);
> +}
> +
> +static int tc956x_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	u32 reg = offset < 32 ? GPIO_IN0_OFFSET : GPIO_IN1_OFFSET;
> +	struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> +	u32 val;
> +
> +	regmap_read(gpio->regmap, reg, &val);

Similar to tc956x_gpio_get_direction(), does this leave val uninitialized
and return unpredictable states if regmap_read() fails?

> +
> +	return val & BIT(offset % 32) ? 1 : 0;
> +}
> +
> +static int tc956x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)

Does the set function pointer in struct gpio_chip expect a void return
type? Assigning a function returning int could trigger an incompatible
pointer type compiler error or cause Control Flow Integrity panics at
runtime.

> +{
> +	u32 reg = offset < 32 ? GPIO_OUT0_OFFSET : GPIO_OUT1_OFFSET;
> +	struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> +	u32 mask = BIT(offset % 32);
> +
> +	return regmap_update_bits(gpio->regmap, reg, mask, value ? mask : 0);
> +}
[ ... ]
> +static int tc956x_gpio_probe(struct auxiliary_device *adev,
> +			     const struct auxiliary_device_id *id)
> +{
[ ... ]
> +	gc->base = -1;
> +	gc->ngpio = TC956X_GPIO_COUNT;
> +	gc->can_sleep = false;

Is it safe to set can_sleep to false while using a regmap? By default,
MMIO regmaps use spinlock_t for synchronization, which becomes a sleepable
rt_mutex on PREEMPT_RT kernels. Calling these GPIO methods from an atomic
context on PREEMPT_RT could cause a panic.

> +
> +	dev_set_drvdata(dev, gpio);
> +
> +	return devm_gpiochip_add_data(dev, gc, gpio);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260501155421.3329862-1-elder@riscstar.com?part=9