| 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: <5cb7a09d-2e67-46b9-b5a7-4cc4cdfb6354@intel.com>
Date: Tue, 30 Jul 2024 15:19:29 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, Jacob Keller
<jacob.e.keller@...el.com>, Wojciech Drewek <wojciech.drewek@...el.com>,
Simon Horman <horms@...nel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v8 04/14] iavf: add support for
negotiating flexible RXDID format
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 30 Jul 2024 05:14:59 -0400
> From: Jacob Keller <jacob.e.keller@...el.com>
>
> 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
Powered by blists - more mailing lists