Re: [PATCH v2 2/9] gpio: mxc: fix wakeup_pads bit operations

[Peng Fan <peng.fan@oss.nxp.com>]

Hi Andy,
On Tue, Oct 06, 2026 at 11:36:15AM +0300, Andy Shevchenko wrote:
>On Mon, Oct 5, 2026 at 5:24 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>>
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> gpio_set_wake_irq() can be called concurrently for different pins on
>> the same port, so need to use atomic bitops when modifying wakeup_pads.
>
>so we need
>
>> Since wakeup_pads is u32 but assign_bit() operate on unsigned long
>
>operates
>
>> pointers.
>
>This sentence doesn't sound like it's finished.
>
>> On 64-bit platforms this causes an 8-byte read-modify-write
>> on a 4-byte field, corrupting the adjacent is_pad_wakeup member.
>> Change wakeup_pads to unsigned long and reorder to avoid the overlap.

wakeup_pads is a u32, while assign_bit() operates 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 member.
So change wakeup_pads to unsigned long and reorder to avoid the overlap.

>>
>> And the enable/disable path unconditionally sets/clears the wakeup_pads
>> bit even when enable_irq_wake()/disable_irq_wake() fails. Only update
>> the bit on success.
>>
>> While at here, simplify the logic by consolidating into a single
>> irq_set_irq_wake() call based on the enable parameter.
>
>...
>
>> +       if (port->irq_high && (gpio_idx >= 16))
>> +               ret = irq_set_irq_wake(port->irq_high, enable);
>> +       else
>> +               ret = irq_set_irq_wake(port->irq, enable);
>>
>
>This blank line may be removed.

sure.

>
>> -       return ret;
>> +       if (ret)
>> +               return ret;
>
>...
>
>> -       for (i = 0; i < 32; i++) {
>> -               if ((port->wakeup_pads & (1 << i))) {
>> -                       type = port->pad_type[i];
>> -                       if (enable)
>> -                               config = pad_type_map[type];
>> -                       else
>> -                               config = IMX_SCU_WAKEUP_OFF;
>> -
>> -                       if (is_imx8qm && config == IMX_SCU_WAKEUP_FALL_EDGE) {
>> -                               dev_warn_once(port->dev,
>> -                                             "No falling-edge support for wakeup on i.MX8QM\n");
>> -                               config = IMX_SCU_WAKEUP_OFF;
>> -                       }
>> +       for_each_set_bit(i, &port->wakeup_pads, 32) {
>
>This change can be done later, can it?
>(I mean in a separate patch)

ok to use a separate patch for switching to use for_each_set_bit.

Thanks
Peng

>
>> +               type = port->pad_type[i];
>> +               if (enable)
>> +                       config = pad_type_map[type];
>> +               else
>> +                       config = IMX_SCU_WAKEUP_OFF;
>>
>> -                       ret |= mxc_gpio_generic_config(port, i, config);
>> +               if (is_imx8qm && config == IMX_SCU_WAKEUP_FALL_EDGE) {
>> +                       dev_warn_once(port->dev,
>> +                                     "No falling-edge support for wakeup on i.MX8QM\n");
>> +                       config = IMX_SCU_WAKEUP_OFF;
>>                 }
>> +
>> +               ret |= mxc_gpio_generic_config(port, i, config);
>>         }
>
>-- 
>With Best Regards,
>Andy Shevchenko
>