netdev - Re: [RFC net-next 08/22] eth: bnxt: support setting size of agg buffers via ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izMH55e9hD3dC7zy_eTVf+PRgOGunsuidtY+yW3-2jO-jw@mail.gmail.com>
Date: Wed, 23 Apr 2025 14:00:01 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, 
	donald.hunter@...il.com, sdf@...ichev.me, dw@...idwei.uk, 
	asml.silence@...il.com, ap420073@...il.com, jdamato@...tly.com, 
	dtatulea@...dia.com, michael.chan@...adcom.com
Subject: Re: [RFC net-next 08/22] eth: bnxt: support setting size of agg
 buffers via ethtool

On Mon, Apr 21, 2025 at 3:28 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> bnxt seems to be able to aggregate data up to 32kB without any issue.
> The driver is already capable of doing this for systems with higher
> order pages. While for systems with 4k pages we historically preferred
> to stick to small buffers because they are easier to allocate, the
> zero-copy APIs remove the allocation problem. The ZC mem is
> pre-allocated and fixed size.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 ++-
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 21 ++++++++++++++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 158b8f96f50c..1723909bde77 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -758,7 +758,8 @@ struct nqe_cn {
>  #define BNXT_RX_PAGE_SHIFT PAGE_SHIFT
>  #endif
>
> -#define BNXT_RX_PAGE_SIZE (1 << BNXT_RX_PAGE_SHIFT)
> +#define BNXT_MAX_RX_PAGE_SIZE  (1 << 15)
> +#define BNXT_RX_PAGE_SIZE      (1 << BNXT_RX_PAGE_SHIFT)
>
>  #define BNXT_MAX_MTU           9500
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 48dd5922e4dd..956f51449709 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -835,6 +835,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
>         ering->rx_jumbo_pending = bp->rx_agg_ring_size;
>         ering->tx_pending = bp->tx_ring_size;
>
> +       kernel_ering->rx_buf_len_max = BNXT_MAX_RX_PAGE_SIZE;
> +       kernel_ering->rx_buf_len = bp->rx_page_size;
>         kernel_ering->hds_thresh_max = BNXT_HDS_THRESHOLD_MAX;
>  }
>
> @@ -862,6 +864,21 @@ static int bnxt_set_ringparam(struct net_device *dev,
>                 return -EINVAL;
>         }
>
> +       if (!kernel_ering->rx_buf_len)  /* Zero means restore default */
> +               kernel_ering->rx_buf_len = BNXT_RX_PAGE_SIZE;
> +
> +       if (kernel_ering->rx_buf_len != bp->rx_page_size &&
> +           !(bp->flags & BNXT_FLAG_CHIP_P5_PLUS)) {
> +               NL_SET_ERR_MSG_MOD(extack, "changing rx-buf-len not supported");
> +               return -EINVAL;
> +       }
> +       if (!is_power_of_2(kernel_ering->rx_buf_len) ||
> +           kernel_ering->rx_buf_len < BNXT_RX_PAGE_SIZE ||
> +           kernel_ering->rx_buf_len > BNXT_MAX_RX_PAGE_SIZE) {
> +               NL_SET_ERR_MSG_MOD(extack, "rx-buf-len out of range, or not power of 2");

Where does the power of 2 limitation come from? bnxt itself? Or some mp issue?

dmabuf mp can do non-power of 2 rx_buf_len, I think. I haven't tested
recently. It may be good to only validate here what bnxt can't do at
all, and let a later check in the pp/mp let us know if the mp doesn't
like the size.


-- 
Thanks,
Mina