netdev - Re: [Intel-wired-lan] [PATCH iwl-next] ice: Implement ethtool reset support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d498996b-50af-461f-bb8c-03531ae7a2cc@intel.com>
Date: Wed, 31 Jul 2024 14:14:28 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
CC: "edumazet@...gle.com" <edumazet@...gle.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, Simon Horman <horms@...nel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ice: Implement ethtool reset
 support



On 31.07.2024 10:22, Kitszel, Przemyslaw wrote:
> On 7/30/24 12:51, Wojciech Drewek wrote:
>> Enable ethtool reset support. Each ethtool reset
>> type is mapped to the CVL reset type:
> 
> not CVL, perhaps "device" or "E810"

right

> 
>> ETH_RESET_MAC - ICE_RESET_CORER
>> ETH_RESET_ALL - ICE_RESET_GLOBR
>> ETH_RESET_DEDICATED - ICE_RESET_PFR
>>
>> Multiple reset flags are not supported.
>> Calling any reset type on port representor triggers VF reset.
>>
>> Command example:
>> GLOBR:
>> $ ethtool --reset enp1s0f0np0 all
>> CORER:
>> $ ethtool --reset enp1s0f0np0 mac
>> PFR:
>> $ ethtool --reset enp1s0f0np0 dedicated
>> VF reset:
>> $ ethtool --reset $port_representor mac
>>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>> Reviewed-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 64 ++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> index 39d2652c3ee1..00b8ac3f1dff 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> @@ -4794,6 +4794,68 @@ static void ice_get_ts_stats(struct net_device *netdev,
>>   	ts_stats->lost = ptp->tx_hwtstamp_timeouts;
>>   }
>>   
>> +/**
>> + * ice_ethtool_reset - triggers a given type of reset
>> + * @dev: network interface device structure
>> + * @flags: set of reset flags
>> + *
>> + * Note that multiple reset flags are not supported
>> + */
>> +static int ice_ethtool_reset(struct net_device *dev, u32 *flags)
>> +{
>> +	struct ice_netdev_priv *np = netdev_priv(dev);
>> +	struct ice_pf *pf = np->vsi->back;
>> +	enum ice_reset_req reset;
>> +
>> +	switch (*flags) {
>> +	case ETH_RESET_MAC:
>> +		*flags &= ~ETH_RESET_MAC;
> 
> this line is equivalent to:
> *flags = 0;
> 
>> +		reset = ICE_RESET_CORER;
>> +		break;
>> +	case ETH_RESET_ALL:
>> +		*flags &= ~ETH_RESET_ALL;
> 
> ditto
> 
>> +		reset = ICE_RESET_GLOBR;
>> +		break;
>> +	case ETH_RESET_DEDICATED:
>> +		*flags &= ~ETH_RESET_DEDICATED;
> 
> ditto
> you could just move *flags = 0; after the switch statement

makes sense

> 
>> +		reset = ICE_RESET_PFR;
>> +		break;
>> +	default:
>> +		netdev_info(dev, "Unsupported set of ethtool flags, multiple flags are not supported");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	ice_schedule_reset(pf, reset);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ice_repr_ethtool_reset - triggers a VF reset
>> + * @dev: network interface device structure
>> + * @flags: set of reset flags
>> + *
>> + * VF associated with the given port representor will be reset
>> + * Any type of reset will trigger VF reset
> 
> why not to support just one type of reset here?
> (that would left us with future option of different behavior on
> different reset type requested)

Agree, I'll use dedicated here since it is similar case to PF reset

> 
>> + */
>> +static int ice_repr_ethtool_reset(struct net_device *dev, u32 *flags)
>> +{
>> +	struct ice_repr *repr = ice_netdev_to_repr(dev);
>> +	struct ice_vf *vf;
>> +
>> +	if (repr->type != ICE_REPR_TYPE_VF)
>> +		return -EOPNOTSUPP;
>> +
>> +	vf = repr->vf;
>> +
>> +	if (ice_check_vf_ready_for_cfg(vf))
>> +		return -EBUSY;
>> +
>> +	*flags = 0;
>> +
>> +	return ice_reset_vf(vf, ICE_VF_RESET_VFLR | ICE_VF_RESET_LOCK);
>> +}
>> +
>>   static const struct ethtool_ops ice_ethtool_ops = {
>>   	.cap_rss_ctx_supported  = true,
>>   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>> @@ -4829,6 +4891,7 @@ static const struct ethtool_ops ice_ethtool_ops = {
>>   	.nway_reset		= ice_nway_reset,
>>   	.get_pauseparam		= ice_get_pauseparam,
>>   	.set_pauseparam		= ice_set_pauseparam,
>> +	.reset			= ice_ethtool_reset,
>>   	.get_rxfh_key_size	= ice_get_rxfh_key_size,
>>   	.get_rxfh_indir_size	= ice_get_rxfh_indir_size,
>>   	.get_rxfh		= ice_get_rxfh,
>> @@ -4885,6 +4948,7 @@ static const struct ethtool_ops ice_ethtool_repr_ops = {
>>   	.get_strings		= ice_repr_get_strings,
>>   	.get_ethtool_stats      = ice_repr_get_ethtool_stats,
>>   	.get_sset_count		= ice_repr_get_sset_count,
>> +	.reset			= ice_repr_ethtool_reset,
>>   };
>>   
>>   /**
>