Re: [PATCH 7/9] gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: mxc: sign extension in field_get() breaks edge flipping for pins 15/31
--

commit afaa23e01f6d59d21c3b7e082aaa50a10fe5daeb
Author: Peng Fan <peng.fan@nxp.com>

gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get

This commit introduces MXC_ICR_REG and MXC_ICR_MASK macros to replace
open-coded shift and mask arithmetic in the driver. It then updates
gpio_set_irq_type() and mxc_flip_edge() to use the new macros alongside
field_prep() and field_get().

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index a3274be7126a1..18ff33a0abfb7 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c

[ ... ]

> @@ -139,6 +140,9 @@ static struct mxc_gpio_hwdata imx35_gpio_hwdata = {
>  #define GPIO_INT_FALL_EDGE	(port->hwdata->fall_edge)
>  #define GPIO_INT_BOTH_EDGES	0x4
>  
> +#define MXC_ICR_REG(gpio)	(GPIO_ICR1 + (((gpio) & 0x10) >> 2))
> +#define MXC_ICR_MASK(gpio)	(0x3 << (((gpio) & 0xf) << 1))

[Severity: High]
Does this literal 0x3 evaluate to a signed integer?

If the mask is calculated for pin 15 or 31, the shift will be 30 bits.
Shifting the signed integer 0x3 by 30 bits produces a negative signed
integer (0xC0000000). Would using an unsigned literal like 0x3U prevent
this?

[ ... ]

> @@ -231,16 +234,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  {
>  	void __iomem *reg = port->base;
> -	u32 bit, val;
> +	u32 val;
>  	int edge;
>  
>  	guard(gpio_generic_lock_irqsave)(&port->gen_gc);
>  
> -	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
> -	bit = gpio & 0xf;
> +	reg += MXC_ICR_REG(gpio);
>  	val = readl(reg);
> -	edge = (val >> (bit << 1)) & 3;
> -	val &= ~(0x3 << (bit << 1));
> +	edge = field_get(MXC_ICR_MASK(gpio), val);

[Severity: High]
Does this field_get() result in sign-extension corruption when reading
pins 15 or 31?

Because MXC_ICR_MASK(gpio) evaluates to a signed integer, the internal
__field_get() macro casts the register value to typeof(mask), which is
signed. When it performs the right shift (>> 30) to extract the field,
it will do an arithmetic right shift, causing the extracted 2-bit field
to sign-extend into a negative value (e.g., 0x80000000 >> 30 becomes -2
instead of 2).

> +	val &= ~MXC_ICR_MASK(gpio);
>  	if (edge == GPIO_INT_HIGH_LEV) {
>  		edge = GPIO_INT_LOW_LEV;
>  		pr_debug("mxc: switch GPIO %d to low trigger\n", gpio);
> @@ -252,7 +254,7 @@ static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  		       gpio, edge);
>  		return;

[Severity: High]
Could the sign-extended negative value trigger this error path?

If edge evaluates to a negative number like -2, it will fail the
validation checks for GPIO_INT_HIGH_LEV and GPIO_INT_LOW_LEV. This
regression would cause IRQ_TYPE_EDGE_BOTH interrupts on pins 15 and 31
to abort here on the first edge, failing to invert the polarity and
causing subsequent interrupts to be missed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-gpio-mxc-cleanup-v1-0-dad728ce27f2@nxp.com?part=7