[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <71215655-6eab-44d6-a372-72b2814f0f80@intel.com>
Date: Tue, 30 Jul 2024 15:33:47 +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 05/14] iavf: negotiate PTP
capabilities
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 30 Jul 2024 05:15:00 -0400
> From: Jacob Keller <jacob.e.keller@...el.com>
>
> Add a new extended capabilities negotiation to exchange information from
> the PF about what PTP capabilities are supported by this VF. This
> requires sending a VIRTCHNL_OP_1588_PTP_GET_CAPS message, and waiting
> for the response from the PF. Handle this early on during the VF
> initialization.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 17 +++-
> drivers/net/ethernet/intel/iavf/iavf_main.c | 69 ++++++++++++++++
> drivers/net/ethernet/intel/iavf/iavf_ptp.h | 12 +++
> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 79 +++++++++++++++++++
> 4 files changed, 175 insertions(+), 2 deletions(-)
> create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.h
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index bc0201f6453d..8e86b0edb046 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -40,6 +40,7 @@
> #include "iavf_txrx.h"
> #include "iavf_fdir.h"
> #include "iavf_adv_rss.h"
> +#include "iavf_ptp.h"
I thought I wrote already that you need to include this file only in the
source files which need it...
> #include <linux/bitmap.h>
[...]
> @@ -423,6 +431,8 @@ struct iavf_adapter {
> VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF)
> #define RXDID_ALLOWED(_a) ((_a)->vf_res->vf_cap_flags & \
> VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC)
> +#define PTP_ALLOWED(_a) ((_a)->vf_res->vf_cap_flags & \
> + VIRTCHNL_VF_CAP_PTP)
Same as in one of the previous patches.
> struct virtchnl_vf_resource *vf_res; /* incl. all VSIs */
> struct virtchnl_vsi_resource *vsi_res; /* our LAN VSI */
> struct virtchnl_version_info pf_version;
> @@ -430,6 +440,7 @@ struct iavf_adapter {
> ((_a)->pf_version.minor == 1))
> struct virtchnl_vlan_caps vlan_v2_caps;
> struct virtchnl_supported_rxdids supported_rxdids;
> + struct iavf_ptp ptp;
Ah, I see now why you need iavf_ptp.h here.
Alternatively, it will be a good idea to create iavf_types.h and define
&iavf_ptp and other structs you may need in this file there. And then
include iavf_types.h in iavf_ptp.h as well.
> u16 msg_enable;
> struct iavf_eth_stats current_stats;
> struct iavf_vsi vsi;
> @@ -569,6 +580,8 @@ int iavf_get_vf_vlan_v2_caps(struct iavf_adapter *adapter);
> int iavf_send_vf_offload_vlan_v2_msg(struct iavf_adapter *adapter);
> int iavf_send_vf_supported_rxdids_msg(struct iavf_adapter *adapter);
> int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter);
> +int iavf_send_vf_ptp_caps_msg(struct iavf_adapter *adapter);
> +int iavf_get_vf_ptp_caps(struct iavf_adapter *adapter);
> void iavf_set_queue_vlan_tag_loc(struct iavf_adapter *adapter);
> u16 iavf_get_num_vlans_added(struct iavf_adapter *adapter);
> void iavf_irq_enable(struct iavf_adapter *adapter, bool flush);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 7edfe71fcf23..11599d62d15a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2083,6 +2083,8 @@ static int iavf_process_aq_command(struct iavf_adapter *adapter)
> return iavf_send_vf_offload_vlan_v2_msg(adapter);
> if (adapter->aq_required & IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS)
> return iavf_send_vf_supported_rxdids_msg(adapter);
> + if (adapter->aq_required & IAVF_FLAG_AQ_GET_PTP_CAPS)
> + return iavf_send_vf_ptp_caps_msg(adapter);
> if (adapter->aq_required & IAVF_FLAG_AQ_DISABLE_QUEUES) {
> iavf_disable_queues(adapter);
> return 0;
> @@ -2657,6 +2659,64 @@ static void iavf_init_recv_supported_rxdids(struct iavf_adapter *adapter)
> iavf_change_state(adapter, __IAVF_INIT_FAILED);
> }
>
> +/**
> + * iavf_init_send_ptp_caps - part of querying for extended PTP capabilities
> + * @adapter: board private structure
> + *
> + * Function processes send of the request for 1588 PTP capabilities to the PF.
> + * Must clear IAVF_EXTENDED_CAP_SEND_PTP if the message is not sent, e.g.
> + * due to the PF not negotiating VIRTCHNL_VF_PTP_CAP
> + */
> +static void iavf_init_send_ptp_caps(struct iavf_adapter *adapter)
> +{
> + int ret;
> +
> + WARN_ON(!(adapter->extended_caps & IAVF_EXTENDED_CAP_SEND_PTP));
Same as in the previous patches.
> +
> + ret = iavf_send_vf_ptp_caps_msg(adapter);
> + if (ret == -EOPNOTSUPP) {
Since you only need @ret once,
if (iavf_send_vf_ptp_caps_msg(adapter) == -EOPNOTSUPP)
> + /* PF does not support VIRTCHNL_VF_PTP_CAP. In this case, we
> + * did not send the capability exchange message and do not
> + * expect a response.
> + */
> + adapter->extended_caps &= ~IAVF_EXTENDED_CAP_RECV_PTP;
> + }
> +
> + /* We sent the message, so move on to the next step */
> + adapter->extended_caps &= ~IAVF_EXTENDED_CAP_SEND_PTP;
> +}
> +
> +/**
> + * iavf_init_recv_ptp_caps - part of querying for supported PTP capabilities
> + * @adapter: board private structure
> + *
> + * Function processes receipt of the PTP capabilities supported on this VF.
> + **/
> +static void iavf_init_recv_ptp_caps(struct iavf_adapter *adapter)
> +{
> + int ret;
> +
> + WARN_ON(!(adapter->extended_caps & IAVF_EXTENDED_CAP_RECV_PTP));
(same)
> +
> + memset(&adapter->ptp.hw_caps, 0, sizeof(adapter->ptp.hw_caps));
> +
> + ret = iavf_get_vf_ptp_caps(adapter);
> + if (ret)
> + goto err;
(same re. @ret)
> +
> + /* We've processed the PF response to the VIRTCHNL_OP_1588_PTP_GET_CAPS
> + * message we sent previously.
> + */
> + adapter->extended_caps &= ~IAVF_EXTENDED_CAP_RECV_PTP;
> + return;
> +err:
(same re. empty newline)
> + /* 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_PTP;
> + iavf_change_state(adapter, __IAVF_INIT_FAILED);
> +}
[...]
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> new file mode 100644
> index 000000000000..aee4e2da0b9a
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2024 Intel Corporation. */
> +
> +#ifndef _IAVF_PTP_H_
> +#define _IAVF_PTP_H_
> +
> +/* fields used for PTP support */
> +struct iavf_ptp {
> + struct virtchnl_ptp_caps hw_caps;
> +};
Please keep header files compilable. Here you definitely need to include
header file which defines &virtchnl_ptp_caps, but it's not included.
[...]
> +int iavf_send_vf_ptp_caps_msg(struct iavf_adapter *adapter)
> +{
> + struct virtchnl_ptp_caps hw_caps = {};
> +
> + adapter->aq_required &= ~IAVF_FLAG_AQ_GET_PTP_CAPS;
> +
> + if (!PTP_ALLOWED(adapter))
> + return -EOPNOTSUPP;
> +
> + hw_caps.caps = (VIRTCHNL_1588_PTP_CAP_READ_PHC |
> + VIRTCHNL_1588_PTP_CAP_RX_TSTAMP);
Parenthesis are redundant here.
Moreover, you can assign them right where you declare @hw_caps.
> +
> + adapter->current_op = VIRTCHNL_OP_1588_PTP_GET_CAPS;
> +
> + return iavf_send_pf_msg(adapter, VIRTCHNL_OP_1588_PTP_GET_CAPS,
> + (u8 *)&hw_caps, sizeof(hw_caps));
> +}
> +
> /**
> * iavf_validate_num_queues
> * @adapter: adapter structure
> @@ -316,6 +352,45 @@ int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter)
> return err;
> }
>
> +int iavf_get_vf_ptp_caps(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;
> +
> + len = sizeof(struct virtchnl_ptp_caps);
> + event.buf_len = len;
> + event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL);
> + 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);
> + if (op == VIRTCHNL_OP_1588_PTP_GET_CAPS)
> + break;
> + }
> +
> + err = (enum iavf_status)le32_to_cpu(event.desc.cookie_low);
> + if (err)
> + goto out_alloc;
> +
> + memcpy(&adapter->ptp.hw_caps, event.msg_buf, min(event.msg_len, len));
> +out_alloc:
> + kfree(event.msg_buf);
> +out:
> + return err;
Same as in the previous patch for the whole function.
Thanks,
Olek
Powered by blists - more mailing lists