Date:	Sat, 28 Jan 2012 10:36:31 +0100
From:	Michał Mirosław <mirqus@...il.com>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Hayes Wang <hayeswang@...ltek.com>
Subject: Re: [PATCH net-next 7/7] r8169: remove work from irq handler.

2012/1/27 Francois Romieu <romieu@...zoreil.com>:
[...]
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
[...]
> @@ -716,7 +722,10 @@ struct rtl8169_private {
>        int (*do_ioctl)(struct rtl8169_private *tp, struct mii_ioctl_data *data, int cmd);
>
>        struct {
> +               DECLARE_BITMAP(flags, RTL_FLAG_MAX);
> +               struct mutex mutex;
>                struct work_struct work;
> +               bool enabled;
>        } wk;
>
>        unsigned features;
> @@ -768,13 +777,20 @@ static int rtl8169_close(struct net_device *dev);
[...]
> +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.

[...]
> @@ -5959,7 +5970,11 @@ static int rtl8169_close(struct net_device *dev)
>        /* Update counters before going down */
>        rtl8169_update_counters(dev);
>
> +       rtl_lock_work(tp);
> +       tp->wk.enabled = false;

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);
>
[...]
> @@ -6081,7 +6096,9 @@ static void __rtl8169_resume(struct net_device *dev)
>
>        rtl_pll_power_up(tp);
>
> -       rtl8169_schedule_work(dev);
> +       tp->wk.enabled = true;

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists