| 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
| ||
|
Message-ID: <a5939151-adc6-4385-9072-ce4ff57bf67f@amd.com>
Date: Wed, 11 Sep 2024 08:36:43 -0700
From: Brett Creeley <bcreeley@....com>
To: Taehee Yoo <ap420073@...il.com>, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, edumazet@...gle.com, corbet@....net,
michael.chan@...adcom.com, netdev@...r.kernel.org, linux-doc@...r.kernel.org
Cc: ecree.xilinx@...il.com, przemyslaw.kitszel@...el.com, andrew@...n.ch,
hkallweit1@...il.com, kory.maincent@...tlin.com, ahmed.zaki@...el.com,
paul.greenwalt@...el.com, rrameshbabu@...dia.com, idosch@...dia.com,
maxime.chevallier@...tlin.com, danieller@...dia.com,
aleksander.lobakin@...el.com
Subject: Re: [PATCH net-next v2 1/4] bnxt_en: add support for rx-copybreak
ethtool command
On 9/11/2024 7:55 AM, Taehee Yoo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> The bnxt_en driver supports rx-copybreak, but it couldn't be set by
> userspace. Only the default value(256) has worked.
> This patch makes the bnxt_en driver support following command.
> `ethtool --set-tunable <devname> rx-copybreak <value> ` and
> `ethtool --get-tunable <devname> rx-copybreak`.
>
> Signed-off-by: Taehee Yoo <ap420073@...il.com>
> ---
>
> v2:
> - Define max/vim rx_copybreak value.
>
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 24 ++++++----
> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 6 ++-
> .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 47 ++++++++++++++++++-
> 3 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
<snip>
> +static void bnxt_init_ring_params(struct bnxt *bp)
> +{
> + bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
> +}
> +
> /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
> * be set on entry.
> */
> @@ -4465,7 +4470,6 @@ void bnxt_set_ring_params(struct bnxt *bp)
> rx_space = rx_size + ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) +
> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>
> - bp->rx_copy_thresh = BNXT_RX_COPY_THRESH;
> ring_size = bp->rx_ring_size;
> bp->rx_agg_ring_size = 0;
> bp->rx_agg_nr_pages = 0;
> @@ -4510,7 +4514,8 @@ void bnxt_set_ring_params(struct bnxt *bp)
> ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> } else {
> - rx_size = SKB_DATA_ALIGN(BNXT_RX_COPY_THRESH + NET_IP_ALIGN);
> + rx_size = SKB_DATA_ALIGN(bp->rx_copybreak +
> + NET_IP_ALIGN);
Tiny nit, but why did you wrap NET_IP_ALIGN to the next line?
> rx_space = rx_size + NET_SKB_PAD +
> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> }
> @@ -6424,8 +6429,8 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
> VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
> req->enables |=
> cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
> - req->jumbo_thresh = cpu_to_le16(bp->rx_copy_thresh);
> - req->hds_threshold = cpu_to_le16(bp->rx_copy_thresh);
> + req->jumbo_thresh = cpu_to_le16(bp->rx_copybreak);
> + req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
> }
> req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
> return hwrm_req_send(bp, req);
> @@ -15864,6 +15869,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> bnxt_init_l2_fltr_tbl(bp);
> bnxt_set_rx_skb_mode(bp, false);
> bnxt_set_tpa_flags(bp);
> + bnxt_init_ring_params(bp);
> bnxt_set_ring_params(bp);
> bnxt_rdma_aux_device_init(bp);
> rc = bnxt_set_dflt_rings(bp, true);
<snip>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index f71cc8188b4e..201c3fcba04e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4319,6 +4319,49 @@ static int bnxt_get_eee(struct net_device *dev, struct ethtool_keee *edata)
> return 0;
> }
>
> +static int bnxt_set_tunable(struct net_device *dev,
> + const struct ethtool_tunable *tuna,
> + const void *data)
> +{
> + struct bnxt *bp = netdev_priv(dev);
> + u32 rx_copybreak;
> +
> + switch (tuna->id) {
> + case ETHTOOL_RX_COPYBREAK:
> + rx_copybreak = *(u32 *)data;
> + if (rx_copybreak < BNXT_MIN_RX_COPYBREAK ||
> + rx_copybreak > BNXT_MAX_RX_COPYBREAK)
> + return -EINVAL;
> + if (rx_copybreak != bp->rx_copybreak) {
> + bp->rx_copybreak = rx_copybreak;
Should bp->rx_copybreak get set before closing the interface in the
netif_running case? Can changing this while traffic is running cause any
unexpected issues?
I wonder if this would be better/safer?
if (netif_running(dev)) {
bnxt_close_nic(bp, false, false);
bp->rx_copybreak = rx_copybreak;
bnxt_set_ring_params(bp);
bnxt_open_nic(bp, false, false);
} else {
bp->rx_copybreak = rx_copybreak;
}
Thanks,
Brett
> + if (netif_running(dev)) {
> + bnxt_close_nic(bp, false, false);
> + bnxt_set_ring_params(bp);
> + bnxt_open_nic(bp, false, false);
> + }
> + }
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
<snip>
Powered by blists - more mailing lists