Re: [PATCH iwl-next v6 5/9] igc: Add support for frame preemption verification

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 2/27/25 15:01, Faizal Rahim wrote:
> This patch implements the "ethtool --set-mm" callback to trigger the
> frame preemption verification handshake.
> 
> Uses the MAC Merge Software Verification (mmsv) mechanism in ethtool
> to perform the verification handshake for igc.
> The structure fpe.mmsv is set by mmsv in ethtool and should remain
> read-only for the driver.
> 

[..]

Thank you for the series, please find some comments inline,
sorry for them being mostly nitpicks.

> ---
>   drivers/net/ethernet/intel/igc/igc.h         |  12 +-
>   drivers/net/ethernet/intel/igc/igc_base.h    |   1 +
>   drivers/net/ethernet/intel/igc/igc_defines.h |   8 +-
>   drivers/net/ethernet/intel/igc/igc_ethtool.c |  21 +++
>   drivers/net/ethernet/intel/igc/igc_main.c    |  53 ++++++-
>   drivers/net/ethernet/intel/igc/igc_tsn.c     | 147 ++++++++++++++++++-
>   drivers/net/ethernet/intel/igc/igc_tsn.h     |  51 +++++++
>   7 files changed, 288 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 22ecdac26cf4..705bd4739e3b 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -40,6 +40,10 @@ void igc_ethtool_set_ops(struct net_device *);
>   
>   #define IGC_MAX_TX_TSTAMP_REGS		4
>   
> +struct fpe_t {

please use "igc_" prefix

> @@ -2068,6 +2088,7 @@ static const struct ethtool_ops igc_ethtool_ops = {
>   	.set_rxfh		= igc_ethtool_set_rxfh,
>   	.get_ts_info		= igc_ethtool_get_ts_info,
>   	.get_channels		= igc_ethtool_get_channels,
> +	.set_mm			= igc_ethtool_set_mm,
>   	.set_channels		= igc_ethtool_set_channels,

you have picked a wierd place on this list to plug the new op

>   	.get_priv_flags		= igc_ethtool_get_priv_flags,
>   	.set_priv_flags		= igc_ethtool_set_priv_flags,


> +/**
> + * igc_enable_empty_addr_recv - Enable rx of packets with all-zeroes MAC address

Rx

> + * @adapter: Pointer to the igc_adapter structure.
> + *
> + * Frame preemption verification requires that packets with the all-zeroes
> + * MAC address are allowed to be received by IGC. This function adds the

s/IGC/the driver/?

> + * all-zeroes destination address to the list of acceptable addresses.
> + *
> + * Return: 0 on success, negative value otherwise.
> + */
> +int igc_enable_empty_addr_recv(struct igc_adapter *adapter)
> +{
> +	u8 empty[ETH_ALEN] = { };

the style prefered is = {}

> +
> +	return igc_add_mac_filter(adapter, IGC_MAC_FILTER_TYPE_DST, empty, -1);
> +}
> +
> +void igc_disable_empty_addr_recv(struct igc_adapter *adapter)
> +{
> +	u8 empty[ETH_ALEN] = { };

ditto {}

> +
> +	igc_del_mac_filter(adapter, IGC_MAC_FILTER_TYPE_DST, empty);
> +}


> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
> index f0213cfce07d..acfff3919793 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
> @@ -1,10 +1,137 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /* Copyright (c)  2019 Intel Corporation */
>   
> +#include <linux/kernel.h>

Please don't include umbrealla headers, instdead IWYU
(Include What You Use)
linux/jump_label.h etc

>   #include "igc.h"
> +#include "igc_base.h"
>   #include "igc_hw.h"
>   #include "igc_tsn.h"
>   


> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
> index 98ec845a86bf..7ba232ef37c9 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.h
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
> @@ -4,9 +4,60 @@
>   #ifndef _IGC_TSN_H_
>   #define _IGC_TSN_H_
>   
> +#define SMD_FRAME_SIZE			60
> +
> +enum igc_txd_popts_type {
> +	SMD_V = 0x01,
> +	SMD_R = 0x02

please end enums with comma (,), unless the last entry is a COUNT/MAX/
SIZE or similar

> +};
> +
> +DECLARE_STATIC_KEY_FALSE(igc_fpe_enabled);
> +
> +void igc_fpe_init(struct igc_adapter *adapter);
> +u32 igc_fpe_get_supported_frag_size(u32 user_frag_size);
>   int igc_tsn_offload_apply(struct igc_adapter *adapter);
>   int igc_tsn_reset(struct igc_adapter *adapter);
>   void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter);
>   bool igc_tsn_is_taprio_activated_by_user(struct igc_adapter *adapter);
>   
> +static inline bool igc_fpe_is_pmac_enabled(struct igc_adapter *adapter)
> +{
> +	return static_branch_unlikely(&igc_fpe_enabled) &&
> +	       adapter->fpe.mmsv.pmac_enabled;
> +}
> +
> +static inline bool igc_fpe_is_verify_or_response(union igc_adv_rx_desc *rx_desc,
> +						 unsigned int size)
> +{
> +	u32 status_error = le32_to_cpu(rx_desc->wb.upper.status_error);
> +	int smd;
> +
> +	smd = FIELD_GET(IGC_RXDADV_STAT_SMD_TYPE_MASK, status_error);
> +
> +	return ((smd == IGC_RXD_STAT_SMD_TYPE_V || smd == IGC_RXD_STAT_SMD_TYPE_R) &&
> +		size == SMD_FRAME_SIZE);

too much braces