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

[Wojciech Drewek <wojciech.drewek@intel.com>]

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@linux.intel.com>
>> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.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,
>>   };
>>   
>>   /**
>