On Thu, Sep 12, 2024 at 12:36 AM Brett Creeley <bcreeley@xxxxxxx> wrote:
Hi Brett,
Thank you so much for your review!
>
>
>
> 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@xxxxxxxxx>
> > ---
> >
> > 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?
Because It exceeds 80 characters 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;
> }
I think your suggestion is much safer!
I will use your suggestion in the v3 patch.
>
> 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>
Thanks a lot!
Taehee Yoo
