| 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: <e66525d1-3f4c-45cc-909f-1a9665d4db97@intel.com>
Date: Mon, 26 Aug 2024 14:43:54 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...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
On 21.08.2024 16:06, Alexander Lobakin wrote:
> 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 '&'.
sure
>
>> 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?
I can try make it happen
>
>> +
>> +/* 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.
Agree
>
>> +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.
As in previous patch I'll use iavf_poll_virtchnl_msg. After that
your comments won't apply
>
>> + }
>> +
>> + 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().
Sure
>
>> + break;
>> case VIRTCHNL_OP_ENABLE_QUEUES:
>> /* enable transmits */
>> iavf_irq_enable(adapter, true);
>
> Thanks,
> Olek
Powered by blists - more mailing lists