Recent changes to the r8169 driver cause it to call
device_set_wakeup_enable under spinlock, which may sleep.
Locking isn't necessary around the call to device_set_wakeup_enable,
so drop the spinlock before calling it, to address this.
Signed-off-by: Daniel J Blueman <[email protected]>
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index d88ce9f..894e7c7 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -846,10 +846,10 @@ static int rtl8169_set_wol(struct net_device
*dev, struct ethtool_wolinfo *wol)
else
tp->features &= ~RTL_FEATURE_WOL;
__rtl8169_set_wol(tp, wol->wolopts);
- device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
-
spin_unlock_irq(&tp->lock);
+ device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
+
return 0;
}
--
Daniel J Blueman
Daniel J Blueman <[email protected]> :
> Recent changes to the r8169 driver cause it to call
> device_set_wakeup_enable under spinlock, which may sleep.
The change is fine but the description is misleading : the r8169
driver has been issuing device_set_wakeup_enable under spinlock since
october 2008. device_set_wakeup_enable did not sleep until recently
(see f2dc0d1809ab7e0147c7e4ac837be1682f706538 for instance).
drivers/net/gianfar_ethtool.c::gfar_set_wol is broken as well when
CONFIG_PM is set.
--
Ueimor
Hi Francois,
On 1 November 2010 23:46, Francois Romieu <[email protected]> wrote:
> Daniel J Blueman <[email protected]> :
>> Recent changes to the r8169 driver cause it to call
>> device_set_wakeup_enable under spinlock, which may sleep.
>
> The change is fine but the description is misleading : the r8169
> driver has been issuing device_set_wakeup_enable under spinlock since
> october 2008. device_set_wakeup_enable did not sleep until recently
> (see f2dc0d1809ab7e0147c7e4ac837be1682f706538 for instance).
Good catch; I also find that only the gainfar is the other driver
needing fixing; I'll follow up with this.
Patch with updated description for David:
As device_set_wakeup_enable can now sleep, move the call to outside
the critical section.
Signed-off-by: Daniel J Blueman <[email protected]>
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index d88ce9f..894e7c7 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -846,10 +846,10 @@ static int rtl8169_set_wol(struct net_device
*dev, struct ethtool_wolinfo *wol)
else
tp->features &= ~RTL_FEATURE_WOL;
__rtl8169_set_wol(tp, wol->wolopts);
- device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
-
spin_unlock_irq(&tp->lock);
+ device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
+
return 0;
}
--
Daniel J Blueman
On Tuesday, November 02, 2010, Daniel J Blueman wrote:
> Hi Francois,
>
> On 1 November 2010 23:46, Francois Romieu <[email protected]> wrote:
> > Daniel J Blueman <[email protected]> :
> >> Recent changes to the r8169 driver cause it to call
> >> device_set_wakeup_enable under spinlock, which may sleep.
> >
> > The change is fine but the description is misleading : the r8169
> > driver has been issuing device_set_wakeup_enable under spinlock since
> > october 2008. device_set_wakeup_enable did not sleep until recently
> > (see f2dc0d1809ab7e0147c7e4ac837be1682f706538 for instance).
Yes, sorry about that. I overlooked the fact that these drivers called
device_set_wakeup_enable() under spinlocks.
> Good catch; I also find that only the gainfar is the other driver
> needing fixing; I'll follow up with this.
Thanks a lot for taking care of this!
> Patch with updated description for David:
>
> As device_set_wakeup_enable can now sleep, move the call to outside
> the critical section.
>
> Signed-off-by: Daniel J Blueman <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index d88ce9f..894e7c7 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -846,10 +846,10 @@ static int rtl8169_set_wol(struct net_device
> *dev, struct ethtool_wolinfo *wol)
> else
> tp->features &= ~RTL_FEATURE_WOL;
> __rtl8169_set_wol(tp, wol->wolopts);
> - device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
> -
> spin_unlock_irq(&tp->lock);
>
> + device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts);
> +
> return 0;
> }
>
>