Re: [PATCH net 01/10] iavf: Fix return of set the new channel count

[Stefan Assmann <sassmann@redhat.com>]

On 2021-11-15 15:59, Tony Nguyen wrote:
> From: Mateusz Palczewski <mateusz.palczewski@intel.com>
> 
> Fixed return correct code from set the new channel count.
> Implemented by check if reset is done in appropriate time.
> This solution give a extra time to pf for reset vf in case
> when user want set new channel count for all vfs.
> Without this patch it is possible to return misleading output
> code to user and vf reset not to be correctly performed by pf.
> 
> Fixes: 5520deb15326 ("iavf: Enable support for up to 16 queues")
> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 5a359a0a20ec..136c801f5584 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -1776,6 +1776,7 @@ static int iavf_set_channels(struct net_device *netdev,
>  {
>  	struct iavf_adapter *adapter = netdev_priv(netdev);
>  	u32 num_req = ch->combined_count;
> +	int i;
>  
>  	if ((adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADQ) &&
>  	    adapter->num_tc) {
> @@ -1798,6 +1799,20 @@ static int iavf_set_channels(struct net_device *netdev,
>  	adapter->num_req_queues = num_req;
>  	adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
>  	iavf_schedule_reset(adapter);
> +
> +	/* wait for the reset is done */
> +	for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
> +		msleep(IAVF_RESET_WAIT_MS);
> +		if (adapter->flags & IAVF_FLAG_RESET_PENDING)
> +			continue;
> +		break;
> +	}
> +	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
> +		adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> +		adapter->num_active_queues = num_req;

Hi Mateusz,

I'm not sure I understand why you touch flags and num_active_queues here
even though the reset is still in progress? Shouldn't we just bail out
and report the error?

> +		return -EOPNOTSUPP;

Is this really the correct thing to report? Setting the queue count is
supported, the device is just busy, so isn't EAGAIN a better way to
express this?

  Stefan

> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.31.1
>