Re: [PATCH] gpio: cgbc: Add init_valid_mask() operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] gpio: cgbc: init_valid_mask clobbers firmware reserved ranges
- [Medium] gpio: cgbc: uninitialized stack variable due to missing short response check
--

commit b1d359743f45922b03a802e12705b29714b272cd
Author: Thomas Richard (congatec GmbH) <thomas.richard@bootlin.com>

gpio: cgbc: Add init_valid_mask() operation

This commit adds the init_valid_mask() operation to the cgbc gpio driver
to mark as valid only the GPIOs which are usable, excluding those
reconfigured by the BIOS as alternate functions.

> diff --git a/drivers/gpio/gpio-cgbc.c b/drivers/gpio/gpio-cgbc.c
> index 84b5ed3c6e22b..1b603d8eccdaa 100644
> --- a/drivers/gpio/gpio-cgbc.c
> +++ b/drivers/gpio/gpio-cgbc.c

[ ... ]

> @@ -146,6 +147,30 @@ static int cgbc_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  		return GPIO_LINE_DIRECTION_IN;
>  }
>  
> +static int cgbc_gpio_init_valid_mask(struct gpio_chip *chip,
> +				     unsigned long *valid_mask,
> +				     unsigned int ngpios)
> +{
> +	struct cgbc_gpio_data *gpio = gpiochip_get_data(chip);
> +	struct cgbc_device_data *cgbc = gpio->cgbc;
> +	int ret;
> +	u8 val;
> +
> +	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_ENA_RD, 0, 0, &val);

[Severity: Medium]
If the hardware returns a short response, could val remain uninitialized here?

Looking at cgbc_command() in drivers/mfd/cgbc-core.c, if the hardware returns
an empty payload (istatus == 0) but a valid checksum, it returns 0 (success)
without writing to the output buffer:

	case CGBC_STATUS_DATA_READY:
		if (istatus > data_size)
			istatus = data_size;
		for (i = 0; i < istatus; i++) {
			_data[i] = ioread8(cgbc->io_cmd + CGBC_CMD_DATA + ((i + 1) % 4));
			...

If istatus is 0, this loop is skipped, val is untouched, and the uninitialized
stack variable is used to populate the valid mask.

> +	if (ret)
> +		return ret;
> +
> +	*valid_mask = val;

[Severity: Medium]
Does this direct assignment clobber firmware-reserved ranges?

The gpiolib core clears reserved bits via gpiochip_apply_reserved_ranges()
before calling the driver's init_valid_mask() callback. By using a direct
assignment instead of a bitwise operation (e.g., *valid_mask &= val), any
hardware lines explicitly reserved by the board firmware will be incorrectly
marked as valid again.

> +
> +	ret = cgbc_gpio_cmd(cgbc, CGBC_GPIO_CMD_ENA_RD, 1, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	*valid_mask |= (val << 8);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-cgbc-gpio-valid-mask-v1-1-938202276e26@bootlin.com?part=1