netdev - Re: [PATCH net 2/2] sfc: do not initialize non existing queues with efx_separate_tx

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]

Date:   Fri, 13 May 2022 12:07:23 +0100
From:   Martin Habets <habetsm.xilinx@...il.com>
To:     Íñigo Huguet <ihuguet@...hat.com>
Cc:     ecree.xilinx@...il.com, amaftei@...arflare.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org,
        Tianhao Zhao <tizhao@...hat.com>
Subject: Re: [PATCH net 2/2] sfc: do not initialize non existing queues with
 efx_separate_tx_channels

On Wed, May 11, 2022 at 02:59:41PM +0200, Íñigo Huguet wrote:
> If efx_separate_tx_channels is used, some error messages and backtraces
> are shown in the logs (see below). This is because during channels
> start, all queues in the channels are init asumming that they exist, but
> they might not if efx_separate_tx_channels is used: some channels only
> have RX queues and others only have TX queues.

Thanks for reporting this. At first glance I suspect there may be more callers
of efx_for_each_channel_tx_queue() which is why it is not yet working for you
even with this fix.
Probably we need to fix those macros themselves.

I'm having a closer look, but it will take some time.

Martin

> 
> Avoid that by checking if the channel has TX, RX or both queues.
> However, even with this patch the NIC is unusable when using
> efx_separate_tx_channels, so there are more problems that I've not
> identified. These messages are still shown at probe time many times:
>  sfc 0000:03:00.0 (unnamed net_device) (uninitialized): MC command 0x92 inlen 8 failed rc=-71 (raw=0) arg=0
>  sfc 0000:03:00.0 (unnamed net_device) (uninitialized): failed to link VI 4294967295 to PIO buffer 1 (-71)
> 
> Those messages were also shown before these patch.
> 
> And then this other message and backtrace were also shown many times,
> but now they're not:
>  sfc 0000:03:00.0 ens6f0np0: MC command 0x82 inlen 544 failed rc=-22 (raw=0) arg=0
>  ------------[ cut here ]------------
>  netdevice: ens6f0np0: failed to initialise TXQ -1
>  WARNING: CPU: 1 PID: 626 at drivers/net/ethernet/sfc/ef10.c:2393 efx_ef10_tx_init+0x201/0x300 [sfc]
>  [...] stripped
>  RIP: 0010:efx_ef10_tx_init+0x201/0x300 [sfc]
>  [...] stripped
>  Call Trace:
>   efx_init_tx_queue+0xaa/0xf0 [sfc]
>   efx_start_channels+0x49/0x120 [sfc]
>   efx_start_all+0x1f8/0x430 [sfc]
>   efx_net_open+0x5a/0xe0 [sfc]
>   __dev_open+0xd0/0x190
>   __dev_change_flags+0x1b3/0x220
>   dev_change_flags+0x21/0x60
>  [...]
> 
> At remove time, these messages were shown. Now they're neither shown:
>  sfc 0000:03:00.0 ens6f0np0: failed to flush 10 queues
>  sfc 0000:03:00.0 ens6f0np0: failed to flush queues
> 
> Fixes: 7ec3de426014 ("sfc: move datapath management code")
> Reported-by: Tianhao Zhao <tizhao@...hat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@...hat.com>
> ---
>  drivers/net/ethernet/sfc/efx_channels.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index da2db6791907..b6b960e2021c 100644
> --- a/drivers/net/ethernet/sfc/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/efx_channels.c
> @@ -1139,17 +1139,21 @@ void efx_start_channels(struct efx_nic *efx)
>  	struct efx_channel *channel;
>  
>  	efx_for_each_channel_rev(channel, efx) {
> -		efx_for_each_channel_tx_queue(tx_queue, channel) {
> -			efx_init_tx_queue(tx_queue);
> -			atomic_inc(&efx->active_queues);
> +		if (channel->channel >= efx->tx_channel_offset) {
> +			efx_for_each_channel_tx_queue(tx_queue, channel) {
> +				efx_init_tx_queue(tx_queue);
> +				atomic_inc(&efx->active_queues);
> +			}
>  		}
>  
> -		efx_for_each_channel_rx_queue(rx_queue, channel) {
> -			efx_init_rx_queue(rx_queue);
> -			atomic_inc(&efx->active_queues);
> -			efx_stop_eventq(channel);
> -			efx_fast_push_rx_descriptors(rx_queue, false);
> -			efx_start_eventq(channel);
> +		if (channel->channel < efx->n_rx_channels) {
> +			efx_for_each_channel_rx_queue(rx_queue, channel) {
> +				efx_init_rx_queue(rx_queue);
> +				atomic_inc(&efx->active_queues);
> +				efx_stop_eventq(channel);
> +				efx_fast_push_rx_descriptors(rx_queue, false);
> +				efx_start_eventq(channel);
> +			}
>  		}
>  
>  		WARN_ON(channel->rx_pkt_n_frags);
> -- 
> 2.34.1