As usage of ethtool private flags is deprecated, what should we use from now on?

As usage of ethtool private flags is deprecated, what should we use from now on?

[Sokolowski, Jan]

Hello netdev!

So, as recently I've been trying to upstream a patch that would introduce a new private flag to i40e driver,
I've received a note that, according to this reply from Jakub Kicinski (https://lore.kernel.org/netdev/20230207215643.43f76bdd@kernel.org/),
the use of private flags is deprecated and is something that will not be accepted by upstream anymore. 
As such flags are an easy way to flip driver-specific behavior switches,
and in the future there would be more patches written to allow end-users to change how the driver works,
it appears that a new way forward is required.

Therefore, do you have any tips on what should private flags be replaced with? 

Regards
Jan

-----Original Message-----
From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> 
Sent: Monday, March 13, 2023 9:35 PM
To: Sokolowski, Jan <jan.sokolowski@intel.com>; e1000-patches@eclists.intel.com; Keller, Jacob E <jacob.e.keller@intel.com>; Lobakin, Aleksander <aleksander.lobakin@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Raczynski, Piotr <piotr.raczynski@intel.com>
Subject: Re: [e1000-patches] [PATCH net-next v3 2/2] i40e: add mdd-auto-reset-vf private flag

On 3/10/2023 6:51 AM, Jan Sokolowski wrote:
> Since VF RX MDD events should disable the queue, add ethtool
> private flag mdd-auto-reset-vf to configure VF reset
> to re-enable the queue. This can be used by a system's administrator
> to select the desired level of security in running VF's.

I think this still needs better justification on the private flag. 
Maintainer has already ciriticized our use of private flags so we need 
to give good justification.

"
Intel keeps trickling in private flags to adjust the behavior of legacy
SR-IOV NDOs. No documentation, no well understood semantics.
"
https://lore.kernel.org/netdev/20230207215643.43f76bdd@kernel.org/

I think commit should provide full justification as Jake suggested 
previously:
"
If we want to try and add this flag I think we need to do so openly, and
clearly explain the reasons. Include information about how we're
aligning with another implementation (ice), explain the
security-critical functionality, and probably as RFC with some potential
explanation of alternatives considered.
"
https://eclists.intel.com/sympa/arc/e1000-patches/2023-02/msg00104.html

IMO the MDD reporting (minus the reference to mdd-auto-reset-vfs) should 
be in patch one. If you are breaking out individual MDD events in patch 
one, why not add the reporting as well? If this private flag/patch is 
not accepted, it justifies us splitting the stats out as well as gives 
us the printing functionality without the need for this patch).

Thanks,
Tony

> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> ---
> v2: Replaced custom ratelimiting to printk_ratelimit
> v3: Reworded commit message.
> ---
>   drivers/net/ethernet/intel/i40e/i40e.h        |  2 +-
>   .../net/ethernet/intel/i40e/i40e_ethtool.c    |  1 +
>   drivers/net/ethernet/intel/i40e/i40e_main.c   | 75 ++++++++++++++++---
>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  2 +
>   4 files changed, 70 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index 6e310a539467..72bd45c4f9ba 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -603,7 +603,7 @@ struct i40e_pf {
>    *   in abilities field of i40e_aq_set_phy_config structure
>    */
>   #define I40E_FLAG_TOTAL_PORT_SHUTDOWN_ENABLED	BIT(27)
> -
> +#define I40E_FLAG_MDD_AUTO_RESET_VF		BIT(28)
>   	struct i40e_client_instance *cinst;
>   	bool stat_offsets_loaded;
>   	struct i40e_hw_port_stats stats;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 4934ff58332c..1ed1df620ae4 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -457,6 +457,7 @@ static const struct i40e_priv_flags i40e_gstrings_priv_flags[] = {
>   	I40E_PRIV_FLAG("base-r-fec", I40E_FLAG_BASE_R_FEC, 0),
>   	I40E_PRIV_FLAG("vf-vlan-pruning",
>   		       I40E_FLAG_VF_VLAN_PRUNING, 0),
> +	I40E_PRIV_FLAG("mdd-auto-reset-vf", I40E_FLAG_MDD_AUTO_RESET_VF, 0),
>   };
>   
>   #define I40E_PRIV_FLAGS_STR_LEN ARRAY_SIZE(i40e_gstrings_priv_flags)
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 9bab68f0a618..f00d8b674687 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -11150,6 +11150,52 @@ static void i40e_handle_reset_warning(struct i40e_pf *pf, bool lock_acquired)
>   	i40e_reset_and_rebuild(pf, false, lock_acquired);
>   }
>   
> +/**
> + * i40e_print_vf_rx_mdd_event - print VF Rx malicious driver detect event
> + * @vf: pointer to the VF structure
> + */
> +static void i40e_print_vf_rx_mdd_event(struct i40e_pf *pf, struct i40e_vf *vf)
> +{
> +	dev_err_ratelimited(&pf->pdev->dev, "%lld Rx Malicious Driver Detection events detected on PF %d VF %d MAC %pm. mdd-auto-reset-vfs=%s\n",
> +		vf->mdd_rx_events.count,
> +		pf->hw.pf_id,
> +		vf->vf_id,
> +		vf->default_lan_addr.addr,
> +		(I40E_FLAG_MDD_AUTO_RESET_VF & pf->flags) ? "on" : "off");
> +}
> +
> +/**
> + * i40e_print_vfs_mdd_events - print VFs malicious driver detect event
> + * @pf: pointer to the PF structure
> + *
> + * Called from i40e_handle_mdd_event to rate limit and print VFs MDD events.
> + */
> +static void i40e_print_vfs_mdd_events(struct i40e_pf *pf)
> +{
> +	struct i40e_vf *vf;
> +	unsigned int i;
> +
> +	for (i = 0; i < pf->num_alloc_vfs; i++) {
> +		vf = &pf->vf[i];
> +		/* only print Rx MDD event message if there are new events */
> +		if (vf->mdd_rx_events.count != vf->mdd_rx_events.last_printed) {
> +			vf->mdd_rx_events.last_printed = vf->mdd_rx_events.count;
> +			i40e_print_vf_rx_mdd_event(pf, vf);
> +		}
> +
> +		/* only print Tx MDD event message if there are new events */
> +		if (vf->mdd_tx_events.count != vf->mdd_tx_events.last_printed) {
> +			vf->mdd_tx_events.last_printed = vf->mdd_tx_events.count;
> +			dev_err_ratelimited(&pf->pdev->dev, "%lld Tx Malicious Driver Detection events detected on PF %d VF %d MAC %pM.\n",
> +				vf->mdd_tx_events.count,
> +				pf->hw.pf_id,
> +				vf->vf_id,
> +				vf->default_lan_addr.addr);
> +		}
> +	}
> +}
> +
> +
>   /**
>    * i40e_handle_mdd_event
>    * @pf: pointer to the PF structure
> @@ -11164,8 +11210,13 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   	u32 reg;
>   	int i;
>   
> -	if (!test_bit(__I40E_MDD_EVENT_PENDING, pf->state))
> +	if (!test_and_clear_bit(__I40E_MDD_EVENT_PENDING, pf->state)) {
> +		/* Since the VF MDD event logging is rate limited, check if
> +		 * there are pending MDD events.
> +		 */
> +		i40e_print_vfs_mdd_events(pf);
>   		return;
> +	}
>   
>   	/* find what triggered the MDD event */
>   	reg = rd32(hw, I40E_GL_MDET_TX);
> @@ -11221,10 +11272,6 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   		if (reg & I40E_VP_MDET_TX_VALID_MASK) {
>   			wr32(hw, I40E_VP_MDET_TX(i), 0xFFFF);
>   			vf->mdd_tx_events.count++;
> -			dev_info(&pf->pdev->dev, "TX driver issue detected on VF %d\n",
> -				 i);
> -			dev_info(&pf->pdev->dev,
> -				 "Use PF Control I/F to re-enable the VF\n");
>   			set_bit(I40E_VF_STATE_DISABLED, &vf->vf_states);
>   		}
>   
> @@ -11232,11 +11279,19 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   		if (reg & I40E_VP_MDET_RX_VALID_MASK) {
>   			wr32(hw, I40E_VP_MDET_RX(i), 0xFFFF);
>   			vf->mdd_rx_events.count++;
> -			dev_info(&pf->pdev->dev, "RX driver issue detected on VF %d\n",
> -				 i);
> -			dev_info(&pf->pdev->dev,
> -				 "Use PF Control I/F to re-enable the VF\n");
>   			set_bit(I40E_VF_STATE_DISABLED, &vf->vf_states);
> +
> +			if (pf->flags & I40E_FLAG_MDD_AUTO_RESET_VF) {
> +				/* VF MDD event counters will be cleared by
> +				 * reset, so print the event prior to reset.
> +				 */
> +				i40e_print_vf_rx_mdd_event(pf, vf);
> +				i40e_vc_notify_vf_reset(vf);
> +				/* Allow VF to process pending reset notification */
> +				msleep(20);
> +
> +				i40e_reset_vf(vf, false);
> +			}
>   		}
>   	}
>   
> @@ -11246,6 +11301,8 @@ static void i40e_handle_mdd_event(struct i40e_pf *pf)
>   	reg |=  I40E_PFINT_ICR0_ENA_MAL_DETECT_MASK;
>   	wr32(hw, I40E_PFINT_ICR0_ENA, reg);
>   	i40e_flush(hw);
> +
> +	i40e_print_vfs_mdd_events(pf);
>   }
>   
>   /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> index 9e701ff09e37..c3bae16cc679 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
> @@ -75,6 +75,8 @@ struct i40e_vm_mac {
>   
>   struct i40e_mdd_vf_events {
>   	u64 count; /* total count of Rx|Tx events */
> +	/* count number of the last printed event */
> +	u64 last_printed;
>   };
>   
>   /* VF information structure */
> 
> 
> ------------------------------------------------------------------------------
> e1000-patches mailing list:   e1000-patches@eclists.intel.com
> 

Re: As usage of ethtool private flags is deprecated, what should we use from now on?

[Andrew Lunn]

On Tue, May 09, 2023 at 11:15:57AM +0000, Sokolowski, Jan wrote:
> Hello netdev!
> 
> So, as recently I've been trying to upstream a patch that would introduce a new private flag to i40e driver,
> I've received a note that, according to this reply from Jakub Kicinski (https://lore.kernel.org/netdev/20230207215643.43f76bdd@kernel.org/),
> the use of private flags is deprecated and is something that will not be accepted by upstream anymore. 
> As such flags are an easy way to flip driver-specific behavior switches,
> and in the future there would be more patches written to allow end-users to change how the driver works,
> it appears that a new way forward is required.

Hi Jan

Is your device 'special'. Does it do things which no other device
does? Why is it doing something which no other device does? Surely if
it is useful, others will copy it. If nobody is going to copy it, then
you have to wonder if it has any value, and why should we care about
it.

We much prefer generic solutions to problem which can be used for a
number of devices. So please consider if this a generic configuration
knob which multiple drivers could make use of? If it is, add a well
defined, documented API for it, using netlink, as part of ethtool or
iproute2, etc. And then watch others coping the idea into their
driver, making use of the API you just added. Everybody benefits.

	Andrew

Re: As usage of ethtool private flags is deprecated, what should we use from now on?

[Jakub Kicinski]

On Tue, 9 May 2023 11:15:57 +0000 Sokolowski, Jan wrote:
> So, as recently I've been trying to upstream a patch that would
> introduce a new private flag to i40e driver, I've received a note
> that, according to this reply from Jakub Kicinski
> (https://lore.kernel.org/netdev/20230207215643.43f76bdd@kernel.org/),
> the use of private flags is deprecated and is something that will not
> be accepted by upstream anymore. 

It has little to do with ethtool private flags being deprecated 
and a lot to do with how you were trying to use them. Andrew 
explains it well.

Maybe we should have called them "my device is weird flags", 
so people don't think they are "do whatever you want" flags :(