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

On 5/2/26 10:56 AM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

In version 2 of the series, the patch that defines TOSHIBA_TC956X_PCI
will come earlier than this one in the series.

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

In this case, we know we are using an MMIO-based regmap, and
since we have no clock, regmap_mmio_read() always returns 0.

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

Again, we do not expect regmap_update_bits() to return an error
for an MMIO-based regmap.

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

This should not fail, so we expect val will be initialized.

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

I might be missing something but I don't understand this comment.
In any case, the next version of the code implements the GPIO
controller using the gpio-regmap framework.

> 
>> +{
>> +	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.

Here too, even if this were an issue, the issue will move into
the gpio-regmap code with the new version of the code.

					-Alex

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