2012/1/27 Francois Romieu [off-list ref]:
[...]

[...]

[...]

+static void rtl_lock_work(struct rtl8169_private *tp)
+{
+       mutex_lock(&tp->wk.mutex);
+}
+
+static void rtl_unlock_work(struct rtl8169_private *tp)
+{
+       mutex_unlock(&tp->wk.mutex);
+}
+

[...]

+static void rtl_schedule_task(struct rtl8169_private *tp, enum rtl_flag flag)
+{
+       spin_lock(&tp->lock);
+       if (!test_and_set_bit(flag, tp->wk.flags))
+               schedule_work(&tp->wk.work);
+       spin_unlock(&tp->lock);
+}
+
+static void rtl_schedule_task_bh(struct rtl8169_private *tp, enum rtl_flag flag)
+{
+       local_bh_disable();
+       rtl_schedule_task(tp, flag);
+       local_bh_enable();
+}
+

It might be enough to do:

rtl_schedule_task():

set_bit(flag, tp->wk.flags);
schedule_work(&tp->wk.work);

This will guarantee that the work is done at least once (twice in
unlikely case that it was being executed just before schedule_work()).

[...]

+/*
+ * Workqueue context.
+ */
+static void rtl_slow_event_work(struct rtl8169_private *tp)
+{
+       struct net_device *dev = tp->dev;
+       u16 status;
+
+       status = rtl_get_events(tp) & tp->event_slow;
+       rtl_ack_events(tp, status);

-               if (unlikely(status & SYSErr)) {
-                       rtl8169_pcierr_interrupt(dev);
+       if (unlikely(status & RxFIFOOver)) {
+               switch (tp->mac_version) {
+               /* Work around for rx fifo overflow */
+               case RTL_GIGA_MAC_VER_11:
+                       netif_stop_queue(dev);
+                       rtl_schedule_task_bh(tp, RTL_FLAG_TASK_RESET_PENDING);

Since this is running from the same task it schedules, it should be
enough to set_bit() and ensure that rtl_slow_event_work() is first on
the list in rtl_task().

[...]

 static void rtl_task(struct work_struct *work)
 {
+       static const struct {
+               int bitnr;
+               void (*action)(struct rtl8169_private *);
+       } rtl_work[] = {
+               { RTL_FLAG_TASK_SLOW_PENDING,   rtl_slow_event_work },
+               { RTL_FLAG_TASK_RESET_PENDING,  rtl_reset_work },
+               { RTL_FLAG_TASK_PHY_PENDING,    rtl_phy_work }
+       };
       struct rtl8169_private *tp =
               container_of(work, struct rtl8169_private, wk.work);
+       struct net_device *dev = tp->dev;
+       int i;
+
+       rtl_lock_work(tp);
+
+       if (!netif_running(dev) || !tp->wk.enabled)
+               goto out_unlock;
+
+       for (i = 0; i < ARRAY_SIZE(rtl_work); i++) {
+               bool pending;
+
+               spin_lock_bh(&tp->lock);
+               pending = test_and_clear_bit(rtl_work[i].bitnr, tp->wk.flags);
+               spin_unlock_bh(&tp->lock);
+
+               if (pending)
+                       rtl_work[i].action(tp);
+       }

This is equivalent to: (the lock does nothing here)

for (...)
   if (test_and_clear_bit(rtl_work[i].bitnr, tp->wk.flags))
      rtl_work[i].action(tp);

This still races with rtl_schedule_task(), but the race is harmless.

[...]

clear_bit() would work, too. rtl_lock_work() makes sure the work is
not running (or even if it waits on this lock, i won't run after
seeing !enabled).

       rtl8169_down(dev);
+       rtl_unlock_work(tp);

       free_irq(dev->irq, dev);

[...]

set_bit() would be enough here (as work is guaranteed to not be
running before because of suspend()).

+
+       rtl_schedule_task_bh(tp, RTL_FLAG_TASK_RESET_PENDING);
 }

 static int rtl8169_resume(struct device *device)

[...]

Best Regards,
Michał Mirosław