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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20211116070712.nijmfavryimbpzdn@p1>
Date:   Tue, 16 Nov 2021 08:07:12 +0100
From:   Stefan Assmann <sassmann@...hat.com>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, kuba@...nel.org,
        Mateusz Palczewski <mateusz.palczewski@...el.com>,
        netdev@...r.kernel.org,
        Grzegorz Szczurek <grzegorzx.szczurek@...el.com>,
        Konrad Jankowski <konrad0.jankowski@...el.com>
Subject: Re: [PATCH net 01/10] iavf: Fix return of set the new channel count

On 2021-11-15 15:59, Tony Nguyen wrote:
> From: Mateusz Palczewski <mateusz.palczewski@...el.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@...el.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Tested-by: Konrad Jankowski <konrad0.jankowski@...el.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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
>