From: Mateusz Polchlopek <[email protected]> Date: Tue, 30 Jul 2024 05:14:59 -0400
> From: Jacob Keller <[email protected]> > > Enable support for VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC, to enable the VF > driver the ability to determine what Rx descriptor formats are > available. This requires sending an additional message during > initialization and reset, the VIRTCHNL_OP_GET_SUPPORTED_RXDIDS. This > operation requests the supported Rx descriptor IDs available from the > PF. [...] > #define ADV_RSS_SUPPORT(_a) ((_a)->vf_res->vf_cap_flags & \ > VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF) > +#define RXDID_ALLOWED(_a) ((_a)->vf_res->vf_cap_flags & \ > + VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC) 1. You need an 'IAVF_' prefix here. 2. Very bad indentation, should be #define IAVF_RXDID_ALLOWED(a) ((a)->vf_res->vf_cap_flags & RX_FLEX_DESC) Tabs + align RX_FLEX DESC as it should be. [...] (sorry I lost a piece of quote here) > + struct virtchnl_supported_rxdids supported_rxdids; Does it make sense to add a struct with only 1 field here? Maybe just add `u64 supp_rxdids`? [...] > +static u8 iavf_select_rx_desc_format(struct iavf_adapter *adapter) Const @adapter. > +{ > + u64 supported_rxdids = adapter->supported_rxdids.supported_rxdids; The variable name could be shorter :D Like just 'rxdids' or 'supp'. > + > + /* If we did not negotiate VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC, we must > + * stick with the default value of the legacy 32 byte format. > + */ > + if (!RXDID_ALLOWED(adapter)) > + return VIRTCHNL_RXDID_1_32B_BASE; > + > + /* Warn if the PF does not list support for the default legacy > + * descriptor format. This shouldn't happen, as this is the format > + * used if VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC is not supported. It is > + * likely caused by a bug in the PF implementation failing to indicate > + * support for the format. > + */ > + if (supported_rxdids & BIT(VIRTCHNL_RXDID_1_32B_BASE)) Isn't this BIT(1_32B_BASE) the same as 1_32B_BASE_M defined previously? > + dev_warn(&adapter->pdev->dev, "PF does not list support for > default Rx descriptor format\n"); 1. > if (supp & BASE) > "PF does *not* list support" Is this condition correct? > + > + return VIRTCHNL_RXDID_1_32B_BASE; This function *always* return 1_32B_BASE, is it intended? Will you add more in the subsequent patches? [...] > +static void iavf_init_send_supported_rxdids(struct iavf_adapter *adapter) > +{ > + int ret; > + > + WARN_ON(!(adapter->extended_caps & IAVF_EXTENDED_CAP_SEND_RXDID)); Please no WARN()s in drivers unless absolutely needed. This looks like a regular pci_warn() or pci_err(). Also, shouldn't this condition be handled somehow here? > + > + ret = iavf_send_vf_supported_rxdids_msg(adapter); > + if (ret == -EOPNOTSUPP) { > + /* PF does not support VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC. In this > + * case, we did not send the capability exchange message and > + * do not expect a response. > + */ > + adapter->extended_caps &= ~IAVF_EXTENDED_CAP_RECV_RXDID; > + } > + > + /* We sent the message, so move on to the next step */ > + adapter->extended_caps &= ~IAVF_EXTENDED_CAP_SEND_RXDID; [...] > +static void iavf_init_recv_supported_rxdids(struct iavf_adapter *adapter) > +{ > + int ret; > + > + WARN_ON(!(adapter->extended_caps & IAVF_EXTENDED_CAP_RECV_RXDID)); Same here. > + > + memset(&adapter->supported_rxdids, 0, > + sizeof(adapter->supported_rxdids)); > + > + ret = iavf_get_vf_supported_rxdids(adapter); > + if (ret) > + goto err; > + > + /* We've processed the PF response to the > + * VIRTCHNL_OP_GET_SUPPORTED_RXDIDS message we sent previously. > + */ > + adapter->extended_caps &= ~IAVF_EXTENDED_CAP_RECV_RXDID; > + return; Pls empty newline here before `err`. > +err: > + /* We didn't receive a reply. Make sure we try sending again when > + * __IAVF_INIT_FAILED attempts to recover. > + */ > + adapter->extended_caps |= IAVF_EXTENDED_CAP_SEND_RXDID; > + iavf_change_state(adapter, __IAVF_INIT_FAILED); > +} [...] > diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.h > b/drivers/net/ethernet/intel/iavf/iavf_txrx.h > index d7b5587aeb8e..17309d8625ac 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.h > +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.h > @@ -262,6 +262,8 @@ struct iavf_ring { > u16 next_to_use; > u16 next_to_clean; > > + u8 rxdid; /* Rx descriptor format */ You're introducing a 1-byte hole here. But I guess there's no space to fit this 1 byte. Anyway, pls check with `pahole` that the layout is fine. I remember I was packing this struct cacheline-friendly. I guess ::rxdid will be used on hotpath. I'd make it at least u16 then. > + > u16 flags; > #define IAVF_TXR_FLAGS_WB_ON_ITR BIT(0) > #define IAVF_TXR_FLAGS_ARM_WB BIT(1) [...] > +int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter) > +{ > + struct iavf_hw *hw = &adapter->hw; > + struct iavf_arq_event_info event; > + enum virtchnl_ops op; > + enum iavf_status err; > + u16 len; u32 is fine. > + > + len = sizeof(struct virtchnl_supported_rxdids); > + event.buf_len = len; > + event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL); I think it would be better to declare a pointer on the stack with __free(kfree) and then assign it to event.msg_buf only after the allocation is done. > + if (!event.msg_buf) { > + err = -ENOMEM; > + goto out; > + } > + > + while (1) { > + /* When the AQ is empty, iavf_clean_arq_element will return > + * nonzero and this loop will terminate. > + */ > + err = iavf_clean_arq_element(hw, &event, NULL); > + if (err != IAVF_SUCCESS) > + goto out_alloc; > + op = (enum virtchnl_ops)le32_to_cpu(event.desc.cookie_high); I think this cast is not needed here. > + if (op == VIRTCHNL_OP_GET_SUPPORTED_RXDIDS) > + break; > + } > + > + err = (enum iavf_status)le32_to_cpu(event.desc.cookie_low); Same here. > + if (err) > + goto out_alloc; > + > + memcpy(&adapter->supported_rxdids, event.msg_buf, > + min(event.msg_len, len)); or if (!err) memcpy() - 1 goto > +out_alloc: > + kfree(event.msg_buf); > +out: > + return err; > +} [...] Thanks, Olek
