[PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[Casey Leedom]

Before commit "net: allocate tx queues in register_netdevice"
netif_tx_stop_all_queues and related functions could be used between
device allocation and registration but now only after registration.
cxgb4 has such a call before registration and crashes now.  Move it
after register_netdev.

Signed-off-by: Casey Leedom <leedom@chelsio.com>
---
 drivers/net/cxgb4vf/cxgb4vf_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/cxgb4vf/cxgb4vf_main.c b/drivers/net/cxgb4vf/cxgb4vf_main.c
index 555ecc5..b9d92a5 100644
--- a/drivers/net/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/cxgb4vf/cxgb4vf_main.c
@@ -2600,7 +2600,6 @@ static int __devinit cxgb4vf_pci_probe(struct pci_dev *pdev,
 		pi->xact_addr_filt = -1;
 		pi->rx_offload = RX_CSO;
 		netif_carrier_off(netdev);
-		netif_tx_stop_all_queues(netdev);
 		netdev->irq = pdev->irq;
 
 		netdev->features = (NETIF_F_SG | NETIF_F_TSO | NETIF_F_TSO6 |
@@ -2661,6 +2660,7 @@ static int __devinit cxgb4vf_pci_probe(struct pci_dev *pdev,
 			continue;
 		}
 
+		netif_tx_stop_all_queues(netdev);
 		set_bit(pidx, &adapter->registered_device_map);
 	}
 	if (adapter->registered_device_map == 0) {
-- 
1.7.0.4

Re: [PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[David Miller]

From: Casey Leedom <leedom@chelsio.com>
Date: Thu, 28 Oct 2010 13:16:47 -0700

> Before commit "net: allocate tx queues in register_netdevice"
> netif_tx_stop_all_queues and related functions could be used between
> device allocation and registration but now only after registration.
> cxgb4 has such a call before registration and crashes now.  Move it
> after register_netdev.
> 
> Signed-off-by: Casey Leedom <leedom@chelsio.com>

Why are you manipulating the queue at all here?

The queue state is "don't care" at this point in time,
and has no meaning until ->open() is invoked.

Re: [PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[Casey Leedom]

On Oct 28, 2010, at 1:21 PM, David Miller wrote:

> From: Casey Leedom <leedom@chelsio.com>
> Date: Thu, 28 Oct 2010 13:16:47 -0700
> 
>> Before commit "net: allocate tx queues in register_netdevice"
>> netif_tx_stop_all_queues and related functions could be used between
>> device allocation and registration but now only after registration.
>> cxgb4 has such a call before registration and crashes now.  Move it
>> after register_netdev.
>> 
>> Signed-off-by: Casey Leedom <leedom@chelsio.com>
> 
> Why are you manipulating the queue at all here?
> 
> The queue state is "don't care" at this point in time,
> and has no meaning until ->open() is invoked.

True.  This driver was modeled on cxgb4, we wanted to make sure that the cxgb4 crash fix is ported to cxgb4vf.  Let me sync up with the other Chelsio maintainers.

Casey

RE: [PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[Dimitrios Michailidis]

> > Before commit "net: allocate tx queues in register_netdevice"
> > netif_tx_stop_all_queues and related functions could be used between
> > device allocation and registration but now only after registration.
> > cxgb4 has such a call before registration and crashes now.  Move it
> > after register_netdev.
> >
> > Signed-off-by: Casey Leedom <leedom@chelsio.com>
> 
> Why are you manipulating the queue at all here?
> 
> The queue state is "don't care" at this point in time,
> and has no meaning until ->open() is invoked.

The reason for the call at its initial pre register_netdev location was
an attempt to make the queues have the same state at the time of each
open.  Without it at the first open __QUEUE_STATE_XOFF is not set due to
0-initialization but at subsequent opens it is set because close stops
the queues.  I agree however that this disparity is not functionally
significant.  Further, I believe moving the call after register_netdev
is buggy as open can be called after registration and it can clash with
the queue stopping.  It seems then that these netif_tx_stop_all_queues
calls have to go now.

Re: [PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[David Miller]

From: "Dimitrios Michailidis" <dm@chelsio.com>
Date: Fri, 29 Oct 2010 00:36:22 -0700

> Further, I believe moving the call after register_netdev is buggy as
> open can be called after registration and it can clash with the
> queue stopping.  It seems then that these netif_tx_stop_all_queues
> calls have to go now.

This is a good explanation of why no driver should be touching the
queue state before the first ->open() call.

Re: [PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[Divy Le Ray]

On 10/29/2010 01:05 PM, David Miller wrote:
> From: "Dimitrios Michailidis"<dm@chelsio.com>
> Date: Fri, 29 Oct 2010 00:36:22 -0700
>
>> Further, I believe moving the call after register_netdev is buggy as
>> open can be called after registration and it can clash with the
>> queue stopping.  It seems then that these netif_tx_stop_all_queues
>> calls have to go now.
> This is a good explanation of why no driver should be touching the
> queue state before the first ->open() call.

I'm sending a series of 3 patches fixing cxgb3, cxgb4, cxgb4vf.

Cheers,
Divy

Re: [PATCH] cxgb4vf: fix crash due to manipulating queues before registration

[David Miller]

From: Divy Le Ray <divy@chelsio.com>
Date: Mon, 01 Nov 2010 14:02:56 -0700

> On 10/29/2010 01:05 PM, David Miller wrote:
>> From: "Dimitrios Michailidis"<dm@chelsio.com>
>> Date: Fri, 29 Oct 2010 00:36:22 -0700
>>
>>> Further, I believe moving the call after register_netdev is buggy as
>>> open can be called after registration and it can clash with the
>>> queue stopping.  It seems then that these netif_tx_stop_all_queues
>>> calls have to go now.
>> This is a good explanation of why no driver should be touching the
>> queue state before the first ->open() call.
> 
> I'm sending a series of 3 patches fixing cxgb3, cxgb4, cxgb4vf.

Thank you.