From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
To: Lorenz Brun <lorenz@brun.one>
Cc: Igor Russkikh <irusskikh@marvell.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Manuel Ullmann <labre@posteo.de>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: atlantic: keep rings across suspend/resume
Date: Thu, 12 Dec 2024 10:54:02 +0100 [thread overview]
Message-ID: <Z1qyuioi7N1WYEW4@mev-dev.igk.intel.com> (raw)
In-Reply-To: <20241212023946.3979643-1-lorenz@brun.one>
On Thu, Dec 12, 2024 at 03:39:24AM +0100, Lorenz Brun wrote:
> The rings are order-6 allocations which tend to fail on suspend due to
> fragmentation. As memory is kept during suspend/resume, we don't need to
> reallocate them.
>
> This does not touch the PTP rings which, if enabled, still reallocate.
> Fixing these is harder as the whole structure is reinitialized.
>
> Fixes: cbe6c3a8f8f4 ("net: atlantic: invert deep par in pm functions, preventing null derefs")
> Signed-off-by: Lorenz Brun <lorenz@brun.one>
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_main.c | 4 ++--
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 7 ++++---
> drivers/net/ethernet/aquantia/atlantic/aq_nic.h | 2 +-
> drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 4 ++--
> drivers/net/ethernet/aquantia/atlantic/aq_vec.c | 10 ++++++++++
> 5 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index c1d1673c5749..cd3709ba7229 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> @@ -84,7 +84,7 @@ int aq_ndev_open(struct net_device *ndev)
>
> err_exit:
> if (err < 0)
> - aq_nic_deinit(aq_nic, true);
> + aq_nic_deinit(aq_nic, true, false);
Only my suggestion:
Instead of passing another boolean to the function you can have:
aq_nic_deinit(...)
{
always without free
}
aq_nic_deinit_and_free(...)
{
aq_nic_deinit(...);
free
}
It may be easier to read.
>
> return err;
> }
> @@ -95,7 +95,7 @@ int aq_ndev_close(struct net_device *ndev)
> int err = 0;
>
> err = aq_nic_stop(aq_nic);
> - aq_nic_deinit(aq_nic, true);
> + aq_nic_deinit(aq_nic, true, false);
>
> return err;
> }
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index fe0e3e2a8117..a6324ae88acf 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -1422,7 +1422,7 @@ void aq_nic_set_power(struct aq_nic_s *self)
> }
> }
>
> -void aq_nic_deinit(struct aq_nic_s *self, bool link_down)
Will be nice to have a kernel-doc.
> +void aq_nic_deinit(struct aq_nic_s *self, bool link_down, bool keep_rings)
> {
> struct aq_vec_s *aq_vec = NULL;
> unsigned int i = 0U;
> @@ -1433,7 +1433,8 @@ void aq_nic_deinit(struct aq_nic_s *self, bool link_down)
> for (i = 0U; i < self->aq_vecs; i++) {
> aq_vec = self->aq_vec[i];
> aq_vec_deinit(aq_vec);
> - aq_vec_ring_free(aq_vec);
> + if (!keep_rings)
> + aq_vec_ring_free(aq_vec);
> }
>
> aq_ptp_unregister(self);
> @@ -1499,7 +1500,7 @@ void aq_nic_shutdown(struct aq_nic_s *self)
> if (err < 0)
> goto err_exit;
> }
> - aq_nic_deinit(self, !self->aq_hw->aq_nic_cfg->wol);
> + aq_nic_deinit(self, !self->aq_hw->aq_nic_cfg->wol, false);
> aq_nic_set_power(self);
>
> err_exit:
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
> index ad33f8586532..f0543a5cc087 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
> @@ -189,7 +189,7 @@ int aq_nic_get_regs(struct aq_nic_s *self, struct ethtool_regs *regs, void *p);
> int aq_nic_get_regs_count(struct aq_nic_s *self);
> u64 *aq_nic_get_stats(struct aq_nic_s *self, u64 *data);
> int aq_nic_stop(struct aq_nic_s *self);
> -void aq_nic_deinit(struct aq_nic_s *self, bool link_down);
> +void aq_nic_deinit(struct aq_nic_s *self, bool link_down, bool keep_rings);
> void aq_nic_set_power(struct aq_nic_s *self);
> void aq_nic_free_hot_resources(struct aq_nic_s *self);
> void aq_nic_free_vectors(struct aq_nic_s *self);
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
> index 43c71f6b314f..1015eab5ee50 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
> @@ -390,7 +390,7 @@ static int aq_suspend_common(struct device *dev)
> if (netif_running(nic->ndev))
> aq_nic_stop(nic);
>
> - aq_nic_deinit(nic, !nic->aq_hw->aq_nic_cfg->wol);
> + aq_nic_deinit(nic, !nic->aq_hw->aq_nic_cfg->wol, true);
> aq_nic_set_power(nic);
>
> rtnl_unlock();
> @@ -426,7 +426,7 @@ static int atl_resume_common(struct device *dev)
>
> err_exit:
> if (ret < 0)
> - aq_nic_deinit(nic, true);
> + aq_nic_deinit(nic, true, false);
>
> rtnl_unlock();
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> index 9769ab4f9bef..3b51d6ee0812 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> @@ -132,6 +132,16 @@ int aq_vec_ring_alloc(struct aq_vec_s *self, struct aq_nic_s *aq_nic,
> unsigned int i = 0U;
> int err = 0;
>
> + if (self && self->tx_rings == aq_nic_cfg->tcs && self->rx_rings == aq_nic_cfg->tcs) {
> + /* Correct rings already allocated, nothing to do here */
> + return 0;
Is the same number of Tx/Rx always enough to say that the vector is the
same? It has more additinal data in the structure.
> + } else if (self && (self->tx_rings > 0 || self->rx_rings > 0)) {
> + /* Allocated rings are different, free rings and reallocate */
> + pr_notice("%s: cannot reuse rings, have %d, need %d, reallocating", __func__,
> + self->tx_rings, aq_nic_cfg->tcs);
> + aq_vec_ring_free(self);
> + }
> +
> for (i = 0; i < aq_nic_cfg->tcs; ++i) {
> const unsigned int idx_ring = AQ_NIC_CFG_TCVEC2RING(aq_nic_cfg,
> i, idx);
Thanks,
Michal
> --
> 2.44.1
next prev parent reply other threads:[~2024-12-12 9:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-12 2:39 [PATCH net] net: atlantic: keep rings across suspend/resume Lorenz Brun
2024-12-12 9:54 ` Michal Swiatkowski [this message]
2024-12-12 17:20 ` Andrew Lunn
2024-12-13 17:00 ` Lorenz Brun
2024-12-13 1:21 ` Jakub Kicinski
2024-12-13 13:58 ` Simon Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z1qyuioi7N1WYEW4@mev-dev.igk.intel.com \
--to=michal.swiatkowski@linux.intel.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=irusskikh@marvell.com \
--cc=kuba@kernel.org \
--cc=labre@posteo.de \
--cc=linux-kernel@vger.kernel.org \
--cc=lorenz@brun.one \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).