[PATCH net 0/2] sfc/siena: fix some efx_separate_tx_channels errors

[PATCH net 0/2] sfc/siena: fix some efx_separate_tx_channels errors

[Íñigo Huguet]

Trying to load sfc driver with modparam efx_separate_tx_channels=1
resulted in errors during initialization and not being able to use the
NIC. This patches fix a few bugs and make it work again.

This has been already done in sfc, do it also in sfc_siena.

Martin Habets (1):
  sfc/siena: fix considering that all channels have TX queues

Íñigo Huguet (1):
  sfc/siena: fix wrong tx channel offset with efx_separate_tx_channels

 drivers/net/ethernet/sfc/siena/efx_channels.c | 6 ++----
 drivers/net/ethernet/sfc/siena/net_driver.h   | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH net 1/2] sfc/siena: fix considering that all channels have TX queues

[Íñigo Huguet]

From: Martin Habets <habetsm.xilinx@gmail.com>

Normally, all channels have RX and TX queues, but this is not true if
modparam efx_separate_tx_channels=1 is used. In that cases, some
channels only have RX queues and others only TX queues (or more
preciselly, they have them allocated, but not initialized).

Fix efx_channel_has_tx_queues to return the correct value for this case
too.

This has been already done for sfc, do it also for sfc_siena.

Messages shown at probe time before the fix:
 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
 [...] stripped

Messages shown at remove time before the fix:
 sfc 0000:03:00.0 ens6f0np0: failed to flush 10 queues
 sfc 0000:03:00.0 ens6f0np0: failed to flush queues

Fixes: 8700aff08984 ("sfc: fix channel allocation with brute force")
Reported-by: Tianhao Zhao <tizhao@redhat.com>
Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com>
Tested-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/net_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/siena/net_driver.h b/drivers/net/ethernet/sfc/siena/net_driver.h
index 318db906a154..723bbeea5d0c 100644
--- a/drivers/net/ethernet/sfc/siena/net_driver.h
+++ b/drivers/net/ethernet/sfc/siena/net_driver.h
@@ -1530,7 +1530,7 @@ static inline bool efx_channel_is_xdp_tx(struct efx_channel *channel)
 
 static inline bool efx_channel_has_tx_queues(struct efx_channel *channel)
 {
-	return true;
+	return channel && channel->channel >= channel->efx->tx_channel_offset;
 }
 
 static inline unsigned int efx_channel_num_tx_queues(struct efx_channel *channel)
-- 
2.34.1

[PATCH net 2/2] sfc/siena: fix wrong tx channel offset with efx_separate_tx_channels

[Íñigo Huguet]

tx_channel_offset is calculated in efx_allocate_msix_channels, but it is
also calculated again in efx_set_channels because it was originally done
there, and when efx_allocate_msix_channels was introduced it was
forgotten to be removed from efx_set_channels.

Moreover, the old calculation is wrong when using
efx_separate_tx_channels because now we can have XDP channels after the
TX channels, so n_channels - n_tx_channels doesn't point to the first TX
channel.

Remove the old calculation from efx_set_channels, and add the
initialization of this variable if MSI or legacy interrupts are used,
next to the initialization of the rest of the related variables, where
it was missing.

This has been already done for sfc, do it also for sfc_siena.

Fixes: 3990a8fffbda ("sfc: allocate channels for XDP tx queues")
Reported-by: Tianhao Zhao <tizhao@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/efx_channels.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sfc/siena/efx_channels.c b/drivers/net/ethernet/sfc/siena/efx_channels.c
index f4919e7ee77b..032b8c0bd788 100644
--- a/drivers/net/ethernet/sfc/siena/efx_channels.c
+++ b/drivers/net/ethernet/sfc/siena/efx_channels.c
@@ -298,6 +298,7 @@ int efx_probe_interrupts(struct efx_nic *efx)
 		efx->n_channels = 1;
 		efx->n_rx_channels = 1;
 		efx->n_tx_channels = 1;
+		efx->tx_channel_offset = 0;
 		efx->n_xdp_channels = 0;
 		efx->xdp_channel_offset = efx->n_channels;
 		rc = pci_enable_msi(efx->pci_dev);
@@ -318,6 +319,7 @@ int efx_probe_interrupts(struct efx_nic *efx)
 		efx->n_channels = 1 + (efx_separate_tx_channels ? 1 : 0);
 		efx->n_rx_channels = 1;
 		efx->n_tx_channels = 1;
+		efx->tx_channel_offset = 1;
 		efx->n_xdp_channels = 0;
 		efx->xdp_channel_offset = efx->n_channels;
 		efx->legacy_irq = efx->pci_dev->irq;
@@ -954,10 +956,6 @@ int efx_set_channels(struct efx_nic *efx)
 	struct efx_channel *channel;
 	int rc;
 
-	efx->tx_channel_offset =
-		efx_siena_separate_tx_channels ?
-		efx->n_channels - efx->n_tx_channels : 0;
-
 	if (efx->xdp_tx_queue_count) {
 		EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
 
-- 
2.34.1

Re: [PATCH net 0/2] sfc/siena: fix some efx_separate_tx_channels errors

[Jakub Kicinski]

On Tue, 31 May 2022 15:40:32 +0200 Íñigo Huguet wrote:
> Trying to load sfc driver with modparam efx_separate_tx_channels=1
> resulted in errors during initialization and not being able to use the
> NIC. This patches fix a few bugs and make it work again.
> 
> This has been already done in sfc, do it also in sfc_siena.

Does not apply.

Re: [PATCH net 0/2] sfc/siena: fix some efx_separate_tx_channels errors

[Íñigo Huguet]

On Tue, May 31, 2022 at 5:38 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 31 May 2022 15:40:32 +0200 Íñigo Huguet wrote:
> > Trying to load sfc driver with modparam efx_separate_tx_channels=1
> > resulted in errors during initialization and not being able to use the
> > NIC. This patches fix a few bugs and make it work again.
> >
> > This has been already done in sfc, do it also in sfc_siena.
>
> Does not apply.
>

Sorry about that, I had only applied with patch, not with git, but it
applies with fuzz 1, as I'm seeing now, so not 100% cleanly. I will
send v2.

-- 
Íñigo Huguet

[PATCH net v2 0/2] sfc/siena: fix some efx_separate_tx_channels errors

[Íñigo Huguet]

Trying to load sfc driver with modparam efx_separate_tx_channels=1
resulted in errors during initialization and not being able to use the
NIC. This patches fix a few bugs and make it work again.

This has been already done for sfc, do it also for sfc_siena.

v2:
* fix patch that didn't apply cleanly

Martin Habets (1):
  sfc/siena: fix considering that all channels have TX queues

Íñigo Huguet (1):
  sfc/siena: fix wrong tx channel offset with efx_separate_tx_channels

 drivers/net/ethernet/sfc/siena/efx_channels.c | 6 ++----
 drivers/net/ethernet/sfc/siena/net_driver.h   | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH net v2 1/2] sfc/siena: fix considering that all channels have TX queues

[Íñigo Huguet]

From: Martin Habets <habetsm.xilinx@gmail.com>

Normally, all channels have RX and TX queues, but this is not true if
modparam efx_separate_tx_channels=1 is used. In that cases, some
channels only have RX queues and others only TX queues (or more
preciselly, they have them allocated, but not initialized).

Fix efx_channel_has_tx_queues to return the correct value for this case
too.

This has been already done for sfc, do it also for sfc_siena.

Messages shown at probe time before the fix:
 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
 [...] stripped

Messages shown at remove time before the fix:
 sfc 0000:03:00.0 ens6f0np0: failed to flush 10 queues
 sfc 0000:03:00.0 ens6f0np0: failed to flush queues

Fixes: 8700aff08984 ("sfc: fix channel allocation with brute force")
Reported-by: Tianhao Zhao <tizhao@redhat.com>
Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com>
Tested-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/siena/net_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/siena/net_driver.h b/drivers/net/ethernet/sfc/siena/net_driver.h
index a8f6c3699c8b..c4a97fbf4672 100644
--- a/drivers/net/ethernet/sfc/siena/net_driver.h
+++ b/drivers/net/ethernet/sfc/siena/net_driver.h
@@ -1529,7 +1529,7 @@ static inline bool efx_channel_is_xdp_tx(struct efx_channel *channel)
 
 static inline bool efx_channel_has_tx_queues(struct efx_channel *channel)
 {
-	return true;
+	return channel && channel->channel >= channel->efx->tx_channel_offset;
 }
 
 static inline unsigned int efx_channel_num_tx_queues(struct efx_channel *channel)
-- 
2.34.1

[PATCH net v2 2/2] sfc/siena: fix wrong tx channel offset with efx_separate_tx_channels

[Íñigo Huguet]

tx_channel_offset is calculated in efx_allocate_msix_channels, but it is
also calculated again in efx_set_channels because it was originally done
there, and when efx_allocate_msix_channels was introduced it was
forgotten to be removed from efx_set_channels.

Moreover, the old calculation is wrong when using
efx_separate_tx_channels because now we can have XDP channels after the
TX channels, so n_channels - n_tx_channels doesn't point to the first TX
channel.

Remove the old calculation from efx_set_channels, and add the
initialization of this variable if MSI or legacy interrupts are used,
next to the initialization of the rest of the related variables, where
it was missing.

This has been already done for sfc, do it also for sfc_siena.

Fixes: 3990a8fffbda ("sfc: allocate channels for XDP tx queues")
Reported-by: Tianhao Zhao <tizhao@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/siena/efx_channels.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sfc/siena/efx_channels.c b/drivers/net/ethernet/sfc/siena/efx_channels.c
index 2465cf4d505c..017212a40df3 100644
--- a/drivers/net/ethernet/sfc/siena/efx_channels.c
+++ b/drivers/net/ethernet/sfc/siena/efx_channels.c
@@ -299,6 +299,7 @@ int efx_siena_probe_interrupts(struct efx_nic *efx)
 		efx->n_channels = 1;
 		efx->n_rx_channels = 1;
 		efx->n_tx_channels = 1;
+		efx->tx_channel_offset = 0;
 		efx->n_xdp_channels = 0;
 		efx->xdp_channel_offset = efx->n_channels;
 		rc = pci_enable_msi(efx->pci_dev);
@@ -319,6 +320,7 @@ int efx_siena_probe_interrupts(struct efx_nic *efx)
 		efx->n_channels = 1 + (efx_siena_separate_tx_channels ? 1 : 0);
 		efx->n_rx_channels = 1;
 		efx->n_tx_channels = 1;
+		efx->tx_channel_offset = 1;
 		efx->n_xdp_channels = 0;
 		efx->xdp_channel_offset = efx->n_channels;
 		efx->legacy_irq = efx->pci_dev->irq;
@@ -958,10 +960,6 @@ int efx_siena_set_channels(struct efx_nic *efx)
 	struct efx_channel *channel;
 	int rc;
 
-	efx->tx_channel_offset =
-		efx_siena_separate_tx_channels ?
-		efx->n_channels - efx->n_tx_channels : 0;
-
 	if (efx->xdp_tx_queue_count) {
 		EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
 
-- 
2.34.1

Re: [PATCH net v2 2/2] sfc/siena: fix wrong tx channel offset with efx_separate_tx_channels

[Martin Habets]

On Wed, Jun 01, 2022 at 08:36:03AM +0200, Íñigo Huguet wrote:
> tx_channel_offset is calculated in efx_allocate_msix_channels, but it is
> also calculated again in efx_set_channels because it was originally done
> there, and when efx_allocate_msix_channels was introduced it was
> forgotten to be removed from efx_set_channels.
> 
> Moreover, the old calculation is wrong when using
> efx_separate_tx_channels because now we can have XDP channels after the
> TX channels, so n_channels - n_tx_channels doesn't point to the first TX
> channel.
> 
> Remove the old calculation from efx_set_channels, and add the
> initialization of this variable if MSI or legacy interrupts are used,
> next to the initialization of the rest of the related variables, where
> it was missing.
> 
> This has been already done for sfc, do it also for sfc_siena.
> 
> Fixes: 3990a8fffbda ("sfc: allocate channels for XDP tx queues")
> Reported-by: Tianhao Zhao <tizhao@redhat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>

Acked-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  drivers/net/ethernet/sfc/siena/efx_channels.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/siena/efx_channels.c b/drivers/net/ethernet/sfc/siena/efx_channels.c
> index 2465cf4d505c..017212a40df3 100644
> --- a/drivers/net/ethernet/sfc/siena/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/siena/efx_channels.c
> @@ -299,6 +299,7 @@ int efx_siena_probe_interrupts(struct efx_nic *efx)
>  		efx->n_channels = 1;
>  		efx->n_rx_channels = 1;
>  		efx->n_tx_channels = 1;
> +		efx->tx_channel_offset = 0;
>  		efx->n_xdp_channels = 0;
>  		efx->xdp_channel_offset = efx->n_channels;
>  		rc = pci_enable_msi(efx->pci_dev);
> @@ -319,6 +320,7 @@ int efx_siena_probe_interrupts(struct efx_nic *efx)
>  		efx->n_channels = 1 + (efx_siena_separate_tx_channels ? 1 : 0);
>  		efx->n_rx_channels = 1;
>  		efx->n_tx_channels = 1;
> +		efx->tx_channel_offset = 1;
>  		efx->n_xdp_channels = 0;
>  		efx->xdp_channel_offset = efx->n_channels;
>  		efx->legacy_irq = efx->pci_dev->irq;
> @@ -958,10 +960,6 @@ int efx_siena_set_channels(struct efx_nic *efx)
>  	struct efx_channel *channel;
>  	int rc;
>  
> -	efx->tx_channel_offset =
> -		efx_siena_separate_tx_channels ?
> -		efx->n_channels - efx->n_tx_channels : 0;
> -
>  	if (efx->xdp_tx_queue_count) {
>  		EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
>  
> -- 
> 2.34.1

Re: [PATCH net v2 0/2] sfc/siena: fix some efx_separate_tx_channels errors

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  1 Jun 2022 08:36:01 +0200 you wrote:
> Trying to load sfc driver with modparam efx_separate_tx_channels=1
> resulted in errors during initialization and not being able to use the
> NIC. This patches fix a few bugs and make it work again.
> 
> This has been already done for sfc, do it also for sfc_siena.
> 
> v2:
> * fix patch that didn't apply cleanly
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] sfc/siena: fix considering that all channels have TX queues
    https://git.kernel.org/netdev/net/c/183614bff5fc
  - [net,v2,2/2] sfc/siena: fix wrong tx channel offset with efx_separate_tx_channels
    https://git.kernel.org/netdev/net/c/25bde571b4a8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html