| 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: <7e832ea6-2036-4112-9b63-20f4475e7f8d@intel.com>
Date: Wed, 21 Aug 2024 16:06:04 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 05/14] iavf: negotiate PTP capabilities
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Wed, 21 Aug 2024 14:15:30 +0200
> 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>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 17 ++++-
> drivers/net/ethernet/intel/iavf/iavf_main.c | 60 ++++++++++++++++
> drivers/net/ethernet/intel/iavf/iavf_ptp.h | 9 +++
> drivers/net/ethernet/intel/iavf/iavf_types.h | 36 ++++++++++
> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 72 +++++++++++++++++++
> 5 files changed, 192 insertions(+), 2 deletions(-)
> create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.h
> create mode 100644 drivers/net/ethernet/intel/iavf/iavf_types.h
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index f1506b3d01ce..871431bed64a 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_types.h"
> #include <linux/bitmap.h>
>
> #define DEFAULT_DEBUG_LEVEL_SHIFT 3
> @@ -338,13 +339,16 @@ struct iavf_adapter {
> #define IAVF_FLAG_AQ_ENABLE_STAG_VLAN_INSERTION BIT_ULL(37)
> #define IAVF_FLAG_AQ_DISABLE_STAG_VLAN_INSERTION BIT_ULL(38)
> #define IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS BIT_ULL(39)
> +#define IAVF_FLAG_AQ_GET_PTP_CAPS BIT_ULL(40)
> +#define IAVF_FLAG_AQ_SEND_PTP_CMD BIT_ULL(41)
>
> /* AQ messages that must be sent after IAVF_FLAG_AQ_GET_CONFIG, in
> * order to negotiated extended capabilities.
> */
> #define IAVF_FLAG_AQ_EXTENDED_CAPS \
> (IAVF_FLAG_AQ_GET_OFFLOAD_VLAN_V2_CAPS | \
> - IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS)
> + IAVF_FLAG_AQ_GET_SUPPORTED_RXDIDS | \
> + IAVF_FLAG_AQ_GET_PTP_CAPS)
>
> /* flags for processing extended capability messages during
> * __IAVF_INIT_EXTENDED_CAPS. Each capability exchange requires
> @@ -358,12 +362,16 @@ struct iavf_adapter {
> #define IAVF_EXTENDED_CAP_RECV_VLAN_V2 BIT_ULL(1)
> #define IAVF_EXTENDED_CAP_SEND_RXDID BIT_ULL(2)
> #define IAVF_EXTENDED_CAP_RECV_RXDID BIT_ULL(3)
> +#define IAVF_EXTENDED_CAP_SEND_PTP BIT_ULL(4)
> +#define IAVF_EXTENDED_CAP_RECV_PTP BIT_ULL(5)
>
> #define IAVF_EXTENDED_CAPS \
> (IAVF_EXTENDED_CAP_SEND_VLAN_V2 | \
> IAVF_EXTENDED_CAP_RECV_VLAN_V2 | \
> IAVF_EXTENDED_CAP_SEND_RXDID | \
> - IAVF_EXTENDED_CAP_RECV_RXDID)
> + IAVF_EXTENDED_CAP_RECV_RXDID | \
> + IAVF_EXTENDED_CAP_SEND_PTP | \
> + IAVF_EXTENDED_CAP_RECV_PTP)
>
> /* Lock to prevent possible clobbering of
> * current_netdev_promisc_flags
> @@ -423,6 +431,8 @@ struct iavf_adapter {
> VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF)
> #define IAVF_RXDID_ALLOWED(a) ((a)->vf_res->vf_cap_flags & \
> VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC)
> +#define IAVF_PTP_ALLOWED(a) ((a)->vf_res->vf_cap_flags & \
> + VIRTCHNL_VF_CAP_PTP)
Bah, should've mentioned that where you introduce IAVF_RXDID_ALLOWED().
I realize that the macros added previously are indented with spaces, but
it's not sorta correct style for the kernel. Maybe you'd indent both new
macros (RXDID and PTP) with tabs? You can also break the line different
way if you want, like
#define IAVF_PTP_ALLOWED(a) \
((a)->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP)
Looks more clear than breaking it after the '&'.
> 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;
> u64 supp_rxdids;
> + struct iavf_ptp ptp;
> u16 msg_enable;
> struct iavf_eth_stats current_stats;
> struct iavf_vsi vsi;
[...]
> 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..65678e76c34f
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2024 Intel Corporation. */
> +
> +#ifndef _IAVF_PTP_H_
> +#define _IAVF_PTP_H_
> +
> +#include "iavf_types.h"
> +
> +#endif /* _IAVF_PTP_H_ */
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_types.h b/drivers/net/ethernet/intel/iavf/iavf_types.h
> new file mode 100644
> index 000000000000..6b7029a1a5a7
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/iavf/iavf_types.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2024 Intel Corporation. */
> +
> +#ifndef _IAVF_TYPES_H_
> +#define _IAVF_TYPES_H_
> +
> +#include "iavf_types.h"
> +
> +#include <linux/avf/virtchnl.h>
> +#include <linux/ptp_clock_kernel.h>
Oh well. I initially asked to introduce iavf_types.h to not bloat
iavf.h, but now types.h includes big ptp_clock_kernel.h :z
When I was reviewing PTP for idpf, I proposed to make this iavf_ptp in
iavf_adapter a pointer and allocate it dynamically, so that iavf.h
wouldn't need to include anything PTP-related at all. This way you
wouldn't need iavf_types.h.
What do you think?
> +
> +/* structure used to queue PTP commands for processing */
> +struct iavf_ptp_aq_cmd {
> + struct list_head list;
> + enum virtchnl_ops v_opcode:16;
> + u16 msglen;
> + u8 msg[] __counted_by(msglen);
> +};
> +
> +/* fields used for PTP support */
Redundant comment I'd say.
> +struct iavf_ptp {
> + wait_queue_head_t phc_time_waitqueue;
> + struct virtchnl_ptp_caps hw_caps;
> + struct ptp_clock_info info;
> + struct ptp_clock *clock;
> + struct list_head aq_cmds;
> + u64 cached_phc_time;
> + unsigned long cached_phc_updated;
> + /* Lock protecting access to the AQ command list */
> + struct mutex aq_cmd_lock;
> + struct kernel_hwtstamp_config hwtstamp_config;
> + bool initialized:1;
> + bool phc_time_ready:1;
> +};
> +
> +#endif /* _IAVF_TYPES_H_ */
[...]
> @@ -307,6 +343,38 @@ int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter)
> return 0;
> }
>
> +int iavf_get_vf_ptp_caps(struct iavf_adapter *adapter)
> +{
> + struct virtchnl_ptp_caps caps = {};
> + struct iavf_hw *hw = &adapter->hw;
> + struct iavf_arq_event_info event;
> + enum virtchnl_ops op;
> + enum iavf_status err;
> +
> + event.msg_buf = (u8 *)∩︀
> + event.buf_len = sizeof(caps);
> +
> + 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)
> + return err;
> + op = (enum virtchnl_ops)le32_to_cpu(event.desc.cookie_high);
This cast is not needed.
> + if (op == VIRTCHNL_OP_1588_PTP_GET_CAPS)
> + break;
Same comments as to one of the previous patches -- you can declare @op
inside the loop and also take into consideration that cpu_to_le32(const)
is faster than le32_to_cpu(var) on BE.
> + }
> +
> + err = le32_to_cpu(event.desc.cookie_low);
> + if (err)
> + return err;
> +
> + memcpy(&adapter->ptp.hw_caps, &caps, sizeof(caps));
> +
> + return 0;
> +}
> +
> /**
> * iavf_configure_queues
> * @adapter: adapter structure
> @@ -2423,6 +2491,10 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
> memcpy(&adapter->supp_rxdids, msg,
> min_t(u16, msglen, sizeof(adapter->supp_rxdids)));
> break;
> + case VIRTCHNL_OP_1588_PTP_GET_CAPS:
> + memcpy(&adapter->ptp.hw_caps, msg,
> + min_t(u16, msglen, sizeof(adapter->ptp.hw_caps)));
Same as to one of the previous patches -- I'd avoid partial copying and
check the msglen first to be the same as this sizeof().
> + break;
> case VIRTCHNL_OP_ENABLE_QUEUES:
> /* enable transmits */
> iavf_irq_enable(adapter, true);
Thanks,
Olek
Powered by blists - more mailing lists