[PATCH net] tg3: Fix deadlock in tg3_change_mtu()

[PATCH net] tg3: Fix deadlock in tg3_change_mtu()

[Nithin Nayak Sujir]

Quoting David Vrabel -
"5780 cards cannot have jumbo frames and TSO enabled together.  When
jumbo frames are enabled by setting the MTU, the TSO feature must be
cleared.  This is done indirectly by calling netdev_update_features()
which will call tg3_fix_features() to actually clear the flags.

netdev_update_features() will also trigger a new netlink message for the
feature change event which will result in a call to tg3_get_stats64()
which deadlocks on the tg3 lock."

tg3_set_mtu() does not need to be under the tg3 lock since converting
the flags to use set_bit(). Move it out to after tg3_netif_stop().

Cc: <stable@vger.kernel.org>
Reported-by: David Vrabel <david.vrabel@citrix.com>
Tested-by: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index e2ca03e..0bb79b8 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -14113,12 +14113,12 @@ static int tg3_change_mtu(struct net_device *dev, int new_mtu)
 
 	tg3_netif_stop(tp);
 
+	tg3_set_mtu(dev, tp, new_mtu);
+
 	tg3_full_lock(tp, 1);
 
 	tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
 
-	tg3_set_mtu(dev, tp, new_mtu);
-
 	/* Reset PHY, otherwise the read DMA engine will be in a mode that
 	 * breaks all requests to 256 bytes.
 	 */
-- 
1.7.9.5

Re: [PATCH net] tg3: Fix deadlock in tg3_change_mtu()

[David Miller]

Networking patches should never be CC:'d to -stable.  And you should
never add the:

Cc: <stable@vger.kernel.org>

tag to networking commit messages.

Instead, after the "---" delimiter, you should kindly request that
I queue up that patch for stable, and why.  I handle networking
stable submissions on my own.

Re: [PATCH net] tg3: Fix deadlock in tg3_change_mtu()

[Nithin Nayak Sujir]

On 02/06/2014 04:54 PM, David Miller wrote:
>
> Networking patches should never be CC:'d to -stable.  And you should
> never add the:
>
> Cc: <stable@vger.kernel.org>
>
> tag to networking commit messages.
>
> Instead, after the "---" delimiter, you should kindly request that
> I queue up that patch for stable, and why.  I handle networking
> stable submissions on my own.
>

I apologize. For some reason, I was under the impression that the rule applied 
to core networking and not driver-only changes. Will keep this in mind.

How should I proceed with this? Should I resubmit?

Also, I'm a little confused as to why my previous submissions to net with stable 
Cc'd were ok. E.g. commit 375679104ab3ccfd18dcbd7ba503734fb9a2c63a - "tg3: 
Expand 4g_overflow_test workaround to skb fragments of any size.". Did it slip 
by because I didn't have the < >?

Thanks,
Nithin.

Re: [PATCH net] tg3: Fix deadlock in tg3_change_mtu()

[David Miller]

From: Nithin Nayak Sujir <nsujir@broadcom.com>
Date: Thu, 6 Feb 2014 17:08:41 -0800

> I apologize. For some reason, I was under the impression that the rule
> applied to core networking and not driver-only changes. Will keep this
> in mind.
> 
> How should I proceed with this? Should I resubmit?

You don't need to resubmit.

> Also, I'm a little confused as to why my previous submissions to net
> with stable Cc'd were ok. E.g. commit
> 375679104ab3ccfd18dcbd7ba503734fb9a2c63a - "tg3: Expand
> 4g_overflow_test workaround to skb fragments of any size.". Did it
> slip by because I didn't have the < >?

I didn't catch them at the time, that's all.  I process hundreds of
patches a day.

Re: [PATCH net] tg3: Fix deadlock in tg3_change_mtu()

[David Miller]

From: Nithin Nayak Sujir <nsujir@broadcom.com>
Date: Thu, 6 Feb 2014 14:13:05 -0800

> Quoting David Vrabel -
> "5780 cards cannot have jumbo frames and TSO enabled together.  When
> jumbo frames are enabled by setting the MTU, the TSO feature must be
> cleared.  This is done indirectly by calling netdev_update_features()
> which will call tg3_fix_features() to actually clear the flags.
> 
> netdev_update_features() will also trigger a new netlink message for the
> feature change event which will result in a call to tg3_get_stats64()
> which deadlocks on the tg3 lock."
> 
> tg3_set_mtu() does not need to be under the tg3 lock since converting
> the flags to use set_bit(). Move it out to after tg3_netif_stop().
> 
> Reported-by: David Vrabel <david.vrabel@citrix.com>
> Tested-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>

Applied and queued up for -stable, thanks.