| Message ID | 20261003-gpio-mxc-cleanup-v1-2-dad728ce27f2@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mxc: bug fixes and probe cleanup | expand |
On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > Replace the open-coded BIT() / mask operations with the atomic > set_bit() / clear_bit() / test_bit() API. Atomic variants are required > because gpio_set_wake_irq() can be called concurrently for different > pins on the same port - irq_set_irq_wake() only holds the per-IRQ > descriptor lock, not a per-port lock, so concurrent modification of > different bits in wakeup_pads is possible. > > However wakeup_pads field is typed as u32 but accessed via set_bit() / > clear_bit() / test_bit() which operate on unsigned long pointers. On > 64-bit platforms this causes an 8-byte read-modify-write on a 4-byte > field, corrupting the adjacent is_pad_wakeup field. > > Change wakeup_pads from u32 to unsigned long to match the bitops API > width requirements. > > Also fix the disable path to only clear the wakeup_pads bit when > disable_irq_wake() succeeds, matching the enable path which already > checks the return value. Previously, a failed disable_irq_wake() would > still clear the bit, causing the driver to lose track of the wakeup > source. The above is too verbose, try to squeeze it to the point. > Also fix a latent signed-shift bug: the old (1 << i) expression in > mxc_gpio_set_pad_wakeup() has implementation-defined behavior when > i == 31, since 1 is a signed int. Too many words for a simple (non-critical) update. ... > struct mxc_gpio_port { > u32 both_edges; > struct mxc_gpio_reg_saved gpio_saved_reg; > bool power_off; > - u32 wakeup_pads; > + unsigned long wakeup_pads; > bool is_pad_wakeup; > u32 pad_type[32]; > const struct mxc_gpio_hwdata *hwdata; While at it, run `pahole` and update the arrangement (of the members you touched here) accordingly. ... > static int gpio_set_wake_irq(struct irq_data *d, u32 enable) > ret = enable_irq_wake(port->irq_high); > else > ret = enable_irq_wake(port->irq); > - port->wakeup_pads |= BIT(gpio_idx); > + if (!ret) > + set_bit(gpio_idx, &port->wakeup_pads); > } else { > if (port->irq_high && (gpio_idx >= 16)) > ret = disable_irq_wake(port->irq_high); > else > ret = disable_irq_wake(port->irq); > - port->wakeup_pads &= ~BIT(gpio_idx); > + if (!ret) > + clear_bit(gpio_idx, &port->wakeup_pads); > } > > return ret; Instead do the following after the if (enable) {} else {} block, namely if (ret) return ret; assign_bit(..., enable) return 0; ... > static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable) > for (i = 0; i < 32; i++) { > - if ((port->wakeup_pads & (1 << i))) { > + if (test_bit(i, &port->wakeup_pads)) { Instead just start using for_each_set_bits() from bitops.h. > type = port->pad_type[i]; > if (enable) > config = pad_type_map[type];
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index e05f276a50e8..8a755ac1af83 100644 --- a/drivers/gpio/gpio-mxc.c +++ b/drivers/gpio/gpio-mxc.c @@ -71,7 +71,7 @@ struct mxc_gpio_port { u32 both_edges; struct mxc_gpio_reg_saved gpio_saved_reg; bool power_off; - u32 wakeup_pads; + unsigned long wakeup_pads; bool is_pad_wakeup; u32 pad_type[32]; const struct mxc_gpio_hwdata *hwdata; @@ -330,13 +330,15 @@ static int gpio_set_wake_irq(struct irq_data *d, u32 enable) ret = enable_irq_wake(port->irq_high); else ret = enable_irq_wake(port->irq); - port->wakeup_pads |= BIT(gpio_idx); + if (!ret) + set_bit(gpio_idx, &port->wakeup_pads); } else { if (port->irq_high && (gpio_idx >= 16)) ret = disable_irq_wake(port->irq_high); else ret = disable_irq_wake(port->irq); - port->wakeup_pads &= ~BIT(gpio_idx); + if (!ret) + clear_bit(gpio_idx, &port->wakeup_pads); } return ret; @@ -599,7 +601,7 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable) }; for (i = 0; i < 32; i++) { - if ((port->wakeup_pads & (1 << i))) { + if (test_bit(i, &port->wakeup_pads)) { type = port->pad_type[i]; if (enable) config = pad_type_map[type];