lkml.org 
[lkml]   [2006]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [patch 07/30] bcm43xx: Drain TX status before starting IRQs
    Date
    On Tuesday 21 November 2006 07:43, Ray Lee wrote:
    > On 11/18/06, Larry Finger <Larry.Finger@lwfinger.net> wrote:
    > > The regression turns out to be a locking problem involving bcm43xx,
    > > wpa_supplicant, and NetworkManager. The exact cause is unknown; however,
    > > this patch is clearly not the problem. Please
    > > reinstate it for inclusion in -stable.
    >
    > This patch is different from the patch that I claim is wrong. The patch that I
    > think is bad *actually changes locking* (merge error?); it is different than
    > the -stable patch that was submitted.
    >
    > Michael et al please read the patch (diff portion) below as if you're seeing
    > it for the first time. Please note the locking changes introduced.

    If you read it carefully, you will notice that there aren't locking changes.
    Only a few loads and stores to net_dev->trans_start are added.

    > user: Michael Buesch <mb@bu3sch.de>
    > date: Wed Nov 01 08:15:40 2006 +0500
    > files: drivers/net/wireless/bcm43xx/bcm43xx_main.c
    > description:
    > [PATCH] bcm43xx: Fix low-traffic netdev watchdog TX timeouts
    >
    > This fixes a netdev watchdog timeout problem.
    > The software needs to call netif_tx_disable before running the
    > hardware calibration code. The problem condition can be shown by the
    > following timegraph.
    >
    > |---5secs - ~10 jiffies time---|---|OOPS
    > ^ ^
    > last real TX periodic work stops netif
    >
    > At OOPS, the following happens:
    > The watchdog timer triggers, because the timeout of 5secs
    > is over. The watchdog first checks for stopped TX.
    > _Usually_ TX is only stopped from the TX handler to indicate
    > a full TX queue. But this is different. We need to stop TX here,
    > regardless of the TX queue state. So the watchdog recognizes
    > the stopped device and assumes it is stopped due to full
    > TX queues (Which is a _wrong_ assumption in this case). It then
    > tests how far the last TX has been in the past. If it's more than
    > 5secs (which is the case for low or no traffic), it will fire
    > a TX timeout.
    >
    > Signed-off-by: Michael Buesch <mb@bu3sch.de>
    > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
    > Signed-off-by: John W. Linville <linville@tuxdriver.com>
    >
    > committer: John W. Linville <linville@laptop.(none)> 1162350940 -0500
    >
    >
    > diff -r 41ff0150cbadd56e692f148adb1bfd4ca420e3e0 -r
    > ca97546422bd9a52a7000607d657ca2915f31104
    > drivers/net/wireless/bcm43xx/bcm43xx_main.c
    > --- a/drivers/net/wireless/bcm43xx/bcm43xx_main.c Wed Nov 01 08:15:39
    > 2006 +0500
    > +++ b/drivers/net/wireless/bcm43xx/bcm43xx_main.c Wed Nov 01 08:15:40
    > 2006 +0500
    > @@ -3163,9 +3163,11 @@ static void bcm43xx_periodic_work_handle
    > static void bcm43xx_periodic_work_handler(void *d)
    > {
    > struct bcm43xx_private *bcm = d;
    > + struct net_device *net_dev = bcm->net_dev;
    > unsigned long flags;
    > u32 savedirqs = 0;
    > int badness;
    > + unsigned long orig_trans_start = 0;
    >
    > mutex_lock(&bcm->mutex);
    > badness = estimate_periodic_work_badness(bcm->periodic_state);
    > @@ -3173,7 +3175,18 @@ static void bcm43xx_periodic_work_handle
    > /* Periodic work will take a long time, so we want it to
    > * be preemtible.
    > */
    > - netif_tx_disable(bcm->net_dev);
    > +
    > + netif_tx_lock_bh(net_dev);
    > + /* We must fake a started transmission here, as we are going to
    > + * disable TX. If we wouldn't fake a TX, it would be possible to
    > + * trigger the netdev watchdog, if the last real TX is already
    > + * some time on the past (slightly less than 5secs)
    > + */
    > + orig_trans_start = net_dev->trans_start;
    > + net_dev->trans_start = jiffies;
    > + netif_stop_queue(net_dev);
    > + netif_tx_unlock_bh(net_dev);
    > +
    > spin_lock_irqsave(&bcm->irq_lock, flags);
    > bcm43xx_mac_suspend(bcm);
    > if (bcm43xx_using_pio(bcm))
    > @@ -3198,6 +3211,7 @@ static void bcm43xx_periodic_work_handle
    > bcm43xx_pio_thaw_txqueues(bcm);
    > bcm43xx_mac_enable(bcm);
    > netif_wake_queue(bcm->net_dev);
    > + net_dev->trans_start = orig_trans_start;
    > }
    > mmiowb();
    > spin_unlock_irqrestore(&bcm->irq_lock, flags);
    >
    > or the patch via gitweb:
    > http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=81e171b95d2d06a64465a1e6ab1e2fb864ea2448
    >
    > Ray
    >
    >
    >

    --
    Greetings Michael.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-11-21 09:57    [W:0.026 / U:0.440 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site