RE: [PATCH net-next] r8152: use GFP_NOIO during system suspend

On Fri, Aug 21, 2026 at 08:51 PM +0800, Andrew wrote:
> On Fri, Aug 21, 2026 at 03:30:54AM +0000, Chih Kai Hsu wrote:
> > > On Mon, Aug 11, 2026 at 02:03 AM +0800, Andrew Lunn wrote:
> > > > On Mon, Aug 10, 2026 at 02:57:41PM +0800, Chih Kai Hsu wrote:
> > > > > During system suspend, memory allocation with GFP_KERNEL can
> > > > > block waiting for I/O to complete. If that I/O depends on a
> > > > > device that is itself suspended, a deadlock results.
> > > > >
> > > > > Introduce RTL8152_SYSTEM_SUSPEND flag to track when the driver
> > > > > is operating in the system suspend/resume context. Set the flag
> > > > > at the start of rtl8152_system_suspend() and clear it at the end
> > > > > of rtl8152_system_resume(), following the same pattern used by
> > > > > SELECTIVE_SUSPEND for runtime suspend.
> > > > >
> > > > > In get_registers() and set_registers(), select GFP_NOIO when the
> > > > > flag is set so that the kmalloc and kmemdup calls in those paths
> > > > > do not trigger I/O reclaim.
> > > >
> > > > Do you have a reproducer for this? Or is it theoretical?
> > > >
> > >
> > > It is theoretical.
> > >
> > > > > @@ -8693,6 +8700,9 @@ static int rtl8152_system_resume(struct
> > > > > r8152
> > > *tp)
> > > > >               usb_submit_urb(tp->intr_urb, GFP_NOIO);
> > > > >       }
> > > > >
> > > > > +     clear_bit(RTL8152_SYSTEM_SUSPEND, &tp->flags);
> > > > > +     smp_mb__after_atomic();
> > > > > +
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > @@ -8758,6 +8768,9 @@ static int rtl8152_system_suspend(struct
> > > > > r8152
> > > > > *tp)  {
> > > > >       struct net_device *netdev = tp->netdev;
> > > > >
> > > > > +     set_bit(RTL8152_SYSTEM_SUSPEND, &tp->flags);
> > > > > +     smp_mb__after_atomic();
> > > > > +
> > > > >       netif_device_detach(netdev);
> > > > >
> > > > >       if (netif_running(netdev) && test_bit(WORK_ENABLE,
> > > > > &tp->flags)) {
> > > >
> > > > It seems odd to me that every driver needs to track if it is in
> > > > suspend/resume or not. Doesn't the struct device tell you? Is
> > > > there no global
> > > state somewhere?

The function hibernate_quiet_exec() used by the driver at drivers/nvdimm/core.c
calls dpm_suspend() directly, so the driver cannot use functions such as pm_suspend_in_progress()
or system_entering_hibernation() to determine whether it is in the suspend/hibernate path now.

The ax88179 driver at drivers/net/usb/ax88179_178a.c uses a private variable, in_pm, which is
set when entering the suspend callback. After that, the driver uses ax88179_in_pm() to determine
whether the driver is in the suspend/hibernate path now. Therefore, using a per-driver flag
makes sense.

> > >
> > > I found
> > > https://elixir.bootlin.com/linux/v7.2-rc6/source/drivers/base/power/
> > > main.c#L
> > > 2343
> > > in the Linux source code.
> > >
> > > pm_restrict_gfp_mask() is called before dpm_suspend(state). It
> > > removes __GFP_IO and __GFP_FS from the global gfp_t
> > > gfp_allowed_mask. Therefore, I think the driver can use GFP_KERNEL
> > > safely. Is this correct? Could you please provide any advice?
> > >
> > >

For the same reason, hibernate_quiet_exec() calls dpm_suspend() directly,
so pm_restrict_gfp_mask() is never called. The driver should use GFP_NOIO in
this case.

> > > >
> > > >        Andrew
> > >
> > > Best,
> > > Chih-Kai
> >
> > Hi Andrew,
> >
> > Just a gentle ping on this patch.
> > Based on my previous observation, should the driver use GFP_NOIO
> explicitly?
> 
> Please try to answer the question yourself. There are lots of drivers out there.
> What do they do? What does the suspend/resume documentation say?
> 
>         Andrew

Best Regards,
Chih-Kai