netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Aquantia ethernet driver suspend/resume issues
@ 2023-11-26  3:04 Linus Torvalds
  2023-11-27 17:29 ` [EXT] " Igor Russkikh
  0 siblings, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2023-11-26  3:04 UTC (permalink / raw)
  To: Igor Russkikh; +Cc: Eric Dumazet, Jakub Kicinski, Paolo Abeni, Netdev

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

Ok, so this is pretty random, but I ended up replacing my main SSD
today, and decided that I'll just do a clean re-install and copy my
user data over from my old SSD. As a result of all that, my ethernet
cable ended up in a random ethernet port when I reconnected
everything, and because of the system reinstall I ended up with
suspend-at-idle on by default (which I very much don't want, but I
only noticed after it happened).

And it turns out that suspend/resume *really* doesn't work on the
Aquantia ethernet driver, which is where the cable happened to be.

First you get an allocation failure at resume:

  kworker/u256:41: page allocation failure: order:6,
mode:0x40d00(GFP_NOIO|__GFP_COMP|__GFP_ZERO),
nodemask=(null),cpuset=/,mems_allowed=0
  CPU: 58 PID: 11654 Comm: kworker/u256:41 Not tainted
  Workqueue: events_unbound async_run_entry_fn
  Call Trace:
   <TASK>
   dump_stack_lvl+0x47/0x60
   warn_alloc+0x165/0x1e0
   __alloc_pages_slowpath.constprop.0+0xcd4/0xd90
   __alloc_pages+0x32d/0x350
   __kmalloc_large_node+0x73/0x130
   __kmalloc+0xc3/0x150
   aq_ring_alloc+0x22/0xb0 [atlantic]
   aq_vec_ring_alloc+0xee/0x1a0 [atlantic]
   aq_nic_init+0x118/0x1d0 [atlantic]
   atl_resume_common+0x40/0xd0 [atlantic]
   ...

and immediately after that we get

  trying to free invalid coherent area: 000000006fb35228
  WARNING: CPU: 58 PID: 11654 at kernel/dma/remap.c:65
dma_common_free_remap+0x2d/0x40
  CPU: 58 PID: 11654 Comm: kworker/u256:41 Not tainted 6.5.6-300.fc39.x86_64 #1
  Workqueue: events_unbound async_run_entry_fn
  Call Trace:
   <TASK>
   __iommu_dma_free+0xe8/0x100
   aq_ring_alloc+0xa4/0xb0 [atlantic]
   aq_vec_ring_alloc+0xee/0x1a0 [atlantic]
   aq_nic_init+0x118/0x1d0 [atlantic]
   atl_resume_common+0x40/0xd0 [atlantic]
   ...
  atlantic 0000:44:00.0: PM: dpm_run_callback():
pci_pm_resume+0x0/0xf0 returns -12
  atlantic 0000:44:00.0: PM: failed to resume async: error -12

and now the slab cache is corrupt and the system is dead.

My *guess* is that what is going on is that when the kcalloc() failued
(because it tries to allocate a large area, and it has only been
tested at boot-time when it succeeds),  we end up doing that

  err_exit:
        if (err < 0) {
                aq_ring_free(self);
                self = NULL;
        }

but aq_ring_free() does

        kfree(self->buff_ring);

        if (self->dx_ring)
                dma_free_coherent(aq_nic_get_dev(self->aq_nic),
                                  self->size * self->dx_size, self->dx_ring,
                                  self->dx_ring_pa);

and notice how it will free the dx_ring even though it was never
allocated! I suspect dc_ring is  non-zero because it was allocated
earlier, but the suspend free'd it - but never cleared the pointer.

That "never cleared the pointer on free" is true for buff_ring too,
but the aq_ring_alloc() did

        self->buff_ring =
                kcalloc(self->size, sizeof(struct aq_ring_buff_s), GFP_KERNEL);

so when that failed, at least it re-initialized that part to NULL, so
we just had a kfree(NULL) which is fine.

Anyway, I suspect a fix for the fatal error might be something like
the attached, but I think the *root* of the problem is how the
aquantia driver tried to allocate a humongous buff_ring with kmalloc,
which really doesn't work.  You can see that "order:6", ie we're
talking an allocation > 100kB, and in low-memory situations that kind
of kmalloc space simply isn't available. It *will* fail.

Again, during boot you'll probably never see any issues. During
suspend/resume it very much does not work.

In general, suspend/resume should *not* do big memory management
things. It should probably have never free'd the old data structure,
and it most definitely cannot try to allocate a big new data structure
in resume.

To make matters worse, it looks like there's not just *one* of those
big allocations, there's multiple ones, both for RX and TX. But I
didn't look much more closely.

I don't know what the right fix is, but *one* fix would certainly be
to not tear everything down at suspend time, only to build it up again
at resume.

And please please please don't double-free things randomly (if that is
what was going on, but it does look like it was).

           Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 781 bytes --]

 drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 4de22eed099a..472c7c08bfed 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -932,11 +932,14 @@ void aq_ring_free(struct aq_ring_s *self)
 		return;
 
 	kfree(self->buff_ring);
+	self->buff_ring = NULL;
 
-	if (self->dx_ring)
+	if (self->dx_ring) {
 		dma_free_coherent(aq_nic_get_dev(self->aq_nic),
 				  self->size * self->dx_size, self->dx_ring,
 				  self->dx_ring_pa);
+		self->dx_ring = NULL;
+	}
 }
 
 unsigned int aq_ring_fill_stats_data(struct aq_ring_s *self, u64 *data)

^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2024-02-02 10:59 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-26  3:04 Aquantia ethernet driver suspend/resume issues Linus Torvalds
2023-11-27 17:29 ` [EXT] " Igor Russkikh
2023-11-27 18:02   ` Linus Torvalds
2023-11-28 17:54     ` Igor Russkikh
2023-11-28 18:37       ` Linus Torvalds
2023-11-27 22:59   ` Jakub Kicinski
2023-11-28 19:18     ` Igor Russkikh
2023-11-28 21:09       ` Jakub Kicinski
2023-11-30 12:59         ` Igor Russkikh
2024-01-21 21:05           ` Peter Waller
2024-01-23 14:58             ` Igor Russkikh
2024-01-23 15:13               ` Peter Waller
2024-01-23 21:02                 ` Peter Waller
2024-01-25 14:58                   ` Igor Russkikh
     [not found]               ` <e8739692-89ea-40e7-b966-bbb4ea5826af@pwaller.net>
2024-02-02 10:35                 ` Igor Russkikh
2024-02-02 10:59                   ` Peter Waller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).