| Message ID | 1288297007-663-1-git-send-email-leedom@chelsio.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {
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(-)