* [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol
@ 2010-09-15 6:24 Denis Kirjanov
2010-09-15 6:35 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Denis Kirjanov @ 2010-09-15 6:24 UTC (permalink / raw)
To: davem; +Cc: andrew, ben, netdev
There is no need to use spinlocks in vortex_{set|get}_wol.
This also fixes a bug:
[ 254.214993] 3c59x 0000:00:0d.0: PME# enabled
[ 254.215021] BUG: sleeping function called from invalid context at kernel/mutex.c:94
[ 254.215030] in_atomic(): 0, irqs_disabled(): 1, pid: 4875, name: ethtool
[ 254.215042] Pid: 4875, comm: ethtool Tainted: G W 2.6.36-rc3+ #7
[ 254.215049] Call Trace:
[ 254.215050] [] __might_sleep+0xb1/0xb6
[ 254.215050] [] mutex_lock+0x17/0x30
[ 254.215050] [] acpi_enable_wakeup_device_power+0x2b/0xb1
[ 254.215050] [] acpi_pm_device_sleep_wake+0x42/0x7f
[ 254.215050] [] acpi_pci_sleep_wake+0x5d/0x63
[ 254.215050] [] platform_pci_sleep_wake+0x1d/0x20
[ 254.215050] [] __pci_enable_wake+0x90/0xd0
[ 254.215050] [] acpi_set_WOL+0x8e/0xf5 [3c59x]
[ 254.215050] [] vortex_set_wol+0x4e/0x5e [3c59x]
[ 254.215050] [] dev_ethtool+0x1cf/0xb61
[ 254.215050] [] ? debug_mutex_free_waiter+0x45/0x4a
[ 254.215050] [] ? __mutex_lock_common+0x204/0x20e
[ 254.215050] [] ? __mutex_lock_slowpath+0x12/0x15
[ 254.215050] [] ? mutex_lock+0x23/0x30
[ 254.215050] [] dev_ioctl+0x42c/0x533
[ 254.215050] [] ? _cond_resched+0x8/0x1c
[ 254.215050] [] ? lock_page+0x1c/0x30
[ 254.215050] [] ? page_address+0x15/0x7c
[ 254.215050] [] ? filemap_fault+0x187/0x2c4
[ 254.215050] [] sock_ioctl+0x1d4/0x1e0
[ 254.215050] [] ? sock_ioctl+0x0/0x1e0
[ 254.215050] [] vfs_ioctl+0x19/0x33
[ 254.215050] [] do_vfs_ioctl+0x424/0x46f
[ 254.215050] [] ? selinux_file_ioctl+0x3c/0x40
[ 254.215050] [] sys_ioctl+0x40/0x5a
[ 254.215050] [] sysenter_do_call+0x12/0x22
vortex_set_wol protected with a spinlock, but nested acpi_set_WOL acquires a mutex inside atomic context.
Ethtool operations are already serialized by RTNL mutex, so it is safe to drop the locks.
Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
drivers/net/3c59x.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index 7a01588..a5c697b 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -634,6 +634,8 @@ struct vortex_private {
medialock:1,
must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */
large_frames:1; /* accept large frames */
+ /* {get|set}_wol operations are already serialized by rtnl.
+ * no additional locking is required for the enable_wol and acpi_set_WOL() */
int drv_flags;
u16 status_enable;
u16 intr_enable;
@@ -2922,13 +2924,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
{
struct vortex_private *vp = netdev_priv(dev);
- spin_lock_irq(&vp->lock);
wol->supported = WAKE_MAGIC;
wol->wolopts = 0;
if (vp->enable_wol)
wol->wolopts |= WAKE_MAGIC;
- spin_unlock_irq(&vp->lock);
}
static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
@@ -2937,13 +2937,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
if (wol->wolopts & ~WAKE_MAGIC)
return -EINVAL;
- spin_lock_irq(&vp->lock);
if (wol->wolopts & WAKE_MAGIC)
vp->enable_wol = 1;
else
vp->enable_wol = 0;
acpi_set_WOL(dev);
- spin_unlock_irq(&vp->lock);
return 0;
}
--
1.6.4.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol 2010-09-15 6:24 [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol Denis Kirjanov @ 2010-09-15 6:35 ` Eric Dumazet 2010-09-15 10:15 ` Denis Kirjanov 0 siblings, 1 reply; 5+ messages in thread From: Eric Dumazet @ 2010-09-15 6:35 UTC (permalink / raw) To: Denis Kirjanov; +Cc: davem, andrew, ben, netdev Le mercredi 15 septembre 2010 à 06:24 +0000, Denis Kirjanov a écrit : > There is no need to use spinlocks in vortex_{set|get}_wol. > This also fixes a bug: > [ 254.214993] 3c59x 0000:00:0d.0: PME# enabled > [ 254.215021] BUG: sleeping function called from invalid context at kernel/mutex.c:94 > [ 254.215030] in_atomic(): 0, irqs_disabled(): 1, pid: 4875, name: ethtool > [ 254.215042] Pid: 4875, comm: ethtool Tainted: G W 2.6.36-rc3+ #7 > [ 254.215049] Call Trace: > [ 254.215050] [] __might_sleep+0xb1/0xb6 > [ 254.215050] [] mutex_lock+0x17/0x30 > [ 254.215050] [] acpi_enable_wakeup_device_power+0x2b/0xb1 > [ 254.215050] [] acpi_pm_device_sleep_wake+0x42/0x7f > [ 254.215050] [] acpi_pci_sleep_wake+0x5d/0x63 > [ 254.215050] [] platform_pci_sleep_wake+0x1d/0x20 > [ 254.215050] [] __pci_enable_wake+0x90/0xd0 > [ 254.215050] [] acpi_set_WOL+0x8e/0xf5 [3c59x] > [ 254.215050] [] vortex_set_wol+0x4e/0x5e [3c59x] > [ 254.215050] [] dev_ethtool+0x1cf/0xb61 > [ 254.215050] [] ? debug_mutex_free_waiter+0x45/0x4a > [ 254.215050] [] ? __mutex_lock_common+0x204/0x20e > [ 254.215050] [] ? __mutex_lock_slowpath+0x12/0x15 > [ 254.215050] [] ? mutex_lock+0x23/0x30 > [ 254.215050] [] dev_ioctl+0x42c/0x533 > [ 254.215050] [] ? _cond_resched+0x8/0x1c > [ 254.215050] [] ? lock_page+0x1c/0x30 > [ 254.215050] [] ? page_address+0x15/0x7c > [ 254.215050] [] ? filemap_fault+0x187/0x2c4 > [ 254.215050] [] sock_ioctl+0x1d4/0x1e0 > [ 254.215050] [] ? sock_ioctl+0x0/0x1e0 > [ 254.215050] [] vfs_ioctl+0x19/0x33 > [ 254.215050] [] do_vfs_ioctl+0x424/0x46f > [ 254.215050] [] ? selinux_file_ioctl+0x3c/0x40 > [ 254.215050] [] sys_ioctl+0x40/0x5a > [ 254.215050] [] sysenter_do_call+0x12/0x22 > > vortex_set_wol protected with a spinlock, but nested acpi_set_WOL acquires a mutex inside atomic context. > Ethtool operations are already serialized by RTNL mutex, so it is safe to drop the locks. > > Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org> > --- > drivers/net/3c59x.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c > index 7a01588..a5c697b 100644 > --- a/drivers/net/3c59x.c > +++ b/drivers/net/3c59x.c > @@ -634,6 +634,8 @@ struct vortex_private { > medialock:1, > must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */ > large_frames:1; /* accept large frames */ > + /* {get|set}_wol operations are already serialized by rtnl. > + * no additional locking is required for the enable_wol and acpi_set_WOL() */ Please format this comment like this : /* This is a fine ............................ * and long comment .................................. */ > int drv_flags; > u16 status_enable; > u16 intr_enable; > @@ -2922,13 +2924,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > { > struct vortex_private *vp = netdev_priv(dev); > > - spin_lock_irq(&vp->lock); > wol->supported = WAKE_MAGIC; > > wol->wolopts = 0; > if (vp->enable_wol) > wol->wolopts |= WAKE_MAGIC; > - spin_unlock_irq(&vp->lock); > } > > static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > @@ -2937,13 +2937,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > if (wol->wolopts & ~WAKE_MAGIC) > return -EINVAL; > > - spin_lock_irq(&vp->lock); > if (wol->wolopts & WAKE_MAGIC) > vp->enable_wol = 1; > else > vp->enable_wol = 0; > acpi_set_WOL(dev); > - spin_unlock_irq(&vp->lock); > > return 0; > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol 2010-09-15 6:35 ` Eric Dumazet @ 2010-09-15 10:15 ` Denis Kirjanov [not found] ` <1284547021.2495.1.camel@edumazet-laptop> 0 siblings, 1 reply; 5+ messages in thread From: Denis Kirjanov @ 2010-09-15 10:15 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, David S. Miller, ben On 09/15/2010 10:35 AM, Eric Dumazet wrote: > Le mercredi 15 septembre 2010 à 06:24 +0000, Denis Kirjanov a écrit : >> There is no need to use spinlocks in vortex_{set|get}_wol. >> This also fixes a bug: >> [ 254.214993] 3c59x 0000:00:0d.0: PME# enabled >> [ 254.215021] BUG: sleeping function called from invalid context at kernel/mutex.c:94 >> [ 254.215030] in_atomic(): 0, irqs_disabled(): 1, pid: 4875, name: ethtool >> [ 254.215042] Pid: 4875, comm: ethtool Tainted: G W 2.6.36-rc3+ #7 >> [ 254.215049] Call Trace: >> [ 254.215050] [] __might_sleep+0xb1/0xb6 >> [ 254.215050] [] mutex_lock+0x17/0x30 >> [ 254.215050] [] acpi_enable_wakeup_device_power+0x2b/0xb1 >> [ 254.215050] [] acpi_pm_device_sleep_wake+0x42/0x7f >> [ 254.215050] [] acpi_pci_sleep_wake+0x5d/0x63 >> [ 254.215050] [] platform_pci_sleep_wake+0x1d/0x20 >> [ 254.215050] [] __pci_enable_wake+0x90/0xd0 >> [ 254.215050] [] acpi_set_WOL+0x8e/0xf5 [3c59x] >> [ 254.215050] [] vortex_set_wol+0x4e/0x5e [3c59x] >> [ 254.215050] [] dev_ethtool+0x1cf/0xb61 >> [ 254.215050] [] ? debug_mutex_free_waiter+0x45/0x4a >> [ 254.215050] [] ? __mutex_lock_common+0x204/0x20e >> [ 254.215050] [] ? __mutex_lock_slowpath+0x12/0x15 >> [ 254.215050] [] ? mutex_lock+0x23/0x30 >> [ 254.215050] [] dev_ioctl+0x42c/0x533 >> [ 254.215050] [] ? _cond_resched+0x8/0x1c >> [ 254.215050] [] ? lock_page+0x1c/0x30 >> [ 254.215050] [] ? page_address+0x15/0x7c >> [ 254.215050] [] ? filemap_fault+0x187/0x2c4 >> [ 254.215050] [] sock_ioctl+0x1d4/0x1e0 >> [ 254.215050] [] ? sock_ioctl+0x0/0x1e0 >> [ 254.215050] [] vfs_ioctl+0x19/0x33 >> [ 254.215050] [] do_vfs_ioctl+0x424/0x46f >> [ 254.215050] [] ? selinux_file_ioctl+0x3c/0x40 >> [ 254.215050] [] sys_ioctl+0x40/0x5a >> [ 254.215050] [] sysenter_do_call+0x12/0x22 >> >> vortex_set_wol protected with a spinlock, but nested acpi_set_WOL acquires a mutex inside atomic context. >> Ethtool operations are already serialized by RTNL mutex, so it is safe to drop the locks. >> >> Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org> >> --- >> drivers/net/3c59x.c | 6 ++---- >> 1 files changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c >> index 7a01588..a5c697b 100644 >> --- a/drivers/net/3c59x.c >> +++ b/drivers/net/3c59x.c >> @@ -634,6 +634,8 @@ struct vortex_private { >> medialock:1, >> must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */ >> large_frames:1; /* accept large frames */ >> + /* {get|set}_wol operations are already serialized by rtnl. >> + * no additional locking is required for the enable_wol and acpi_set_WOL() */ > > Please format this comment like this : > > /* This is a fine ............................ > * and long comment .................................. > */ > >> int drv_flags; >> u16 status_enable; >> u16 intr_enable; >> @@ -2922,13 +2924,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> { >> struct vortex_private *vp = netdev_priv(dev); >> >> - spin_lock_irq(&vp->lock); >> wol->supported = WAKE_MAGIC; >> >> wol->wolopts = 0; >> if (vp->enable_wol) >> wol->wolopts |= WAKE_MAGIC; >> - spin_unlock_irq(&vp->lock); >> } >> >> static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> @@ -2937,13 +2937,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> if (wol->wolopts & ~WAKE_MAGIC) >> return -EINVAL; >> >> - spin_lock_irq(&vp->lock); >> if (wol->wolopts & WAKE_MAGIC) >> vp->enable_wol = 1; >> else >> vp->enable_wol = 0; >> acpi_set_WOL(dev); >> - spin_unlock_irq(&vp->lock); >> >> return 0; >> } > > Comments fixed, thanks Eric --- drivers/net/3c59x.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c index 7a01588..afcad74 100644 --- a/drivers/net/3c59x.c +++ b/drivers/net/3c59x.c @@ -634,6 +634,9 @@ struct vortex_private { medialock:1, must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */ large_frames:1; /* accept large frames */ + /* {get|set}_wol operations are already serialized by rtnl. + * no additional locking is required for the enable_wol and acpi_set_WOL() + */ int drv_flags; u16 status_enable; u16 intr_enable; @@ -2922,13 +2925,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct vortex_private *vp = netdev_priv(dev); - spin_lock_irq(&vp->lock); wol->supported = WAKE_MAGIC; wol->wolopts = 0; if (vp->enable_wol) wol->wolopts |= WAKE_MAGIC; - spin_unlock_irq(&vp->lock); } static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) @@ -2937,13 +2938,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if (wol->wolopts & ~WAKE_MAGIC) return -EINVAL; - spin_lock_irq(&vp->lock); if (wol->wolopts & WAKE_MAGIC) vp->enable_wol = 1; else vp->enable_wol = 0; acpi_set_WOL(dev); - spin_unlock_irq(&vp->lock); return 0; } -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <1284547021.2495.1.camel@edumazet-laptop>]
* Re: [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol [not found] ` <1284547021.2495.1.camel@edumazet-laptop> @ 2010-09-15 10:58 ` Denis Kirjanov 2010-09-15 21:36 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Denis Kirjanov @ 2010-09-15 10:58 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, David S. Miller, ben On 09/15/2010 02:37 PM, Eric Dumazet wrote: > Le mercredi 15 septembre 2010 à 14:15 +0400, Denis Kirjanov a écrit : >> On 09/15/2010 10:35 AM, Eric Dumazet wrote: >>> Le mercredi 15 septembre 2010 à 06:24 +0000, Denis Kirjanov a écrit : >>>> There is no need to use spinlocks in vortex_{set|get}_wol. >>>> This also fixes a bug: >>>> [ 254.214993] 3c59x 0000:00:0d.0: PME# enabled >>>> [ 254.215021] BUG: sleeping function called from invalid context at kernel/mutex.c:94 >>>> [ 254.215030] in_atomic(): 0, irqs_disabled(): 1, pid: 4875, name: ethtool >>>> [ 254.215042] Pid: 4875, comm: ethtool Tainted: G W 2.6.36-rc3+ #7 >>>> [ 254.215049] Call Trace: >>>> [ 254.215050] [] __might_sleep+0xb1/0xb6 >>>> [ 254.215050] [] mutex_lock+0x17/0x30 >>>> [ 254.215050] [] acpi_enable_wakeup_device_power+0x2b/0xb1 >>>> [ 254.215050] [] acpi_pm_device_sleep_wake+0x42/0x7f >>>> [ 254.215050] [] acpi_pci_sleep_wake+0x5d/0x63 >>>> [ 254.215050] [] platform_pci_sleep_wake+0x1d/0x20 >>>> [ 254.215050] [] __pci_enable_wake+0x90/0xd0 >>>> [ 254.215050] [] acpi_set_WOL+0x8e/0xf5 [3c59x] >>>> [ 254.215050] [] vortex_set_wol+0x4e/0x5e [3c59x] >>>> [ 254.215050] [] dev_ethtool+0x1cf/0xb61 >>>> [ 254.215050] [] ? debug_mutex_free_waiter+0x45/0x4a >>>> [ 254.215050] [] ? __mutex_lock_common+0x204/0x20e >>>> [ 254.215050] [] ? __mutex_lock_slowpath+0x12/0x15 >>>> [ 254.215050] [] ? mutex_lock+0x23/0x30 >>>> [ 254.215050] [] dev_ioctl+0x42c/0x533 >>>> [ 254.215050] [] ? _cond_resched+0x8/0x1c >>>> [ 254.215050] [] ? lock_page+0x1c/0x30 >>>> [ 254.215050] [] ? page_address+0x15/0x7c >>>> [ 254.215050] [] ? filemap_fault+0x187/0x2c4 >>>> [ 254.215050] [] sock_ioctl+0x1d4/0x1e0 >>>> [ 254.215050] [] ? sock_ioctl+0x0/0x1e0 >>>> [ 254.215050] [] vfs_ioctl+0x19/0x33 >>>> [ 254.215050] [] do_vfs_ioctl+0x424/0x46f >>>> [ 254.215050] [] ? selinux_file_ioctl+0x3c/0x40 >>>> [ 254.215050] [] sys_ioctl+0x40/0x5a >>>> [ 254.215050] [] sysenter_do_call+0x12/0x22 >>>> >>>> vortex_set_wol protected with a spinlock, but nested acpi_set_WOL acquires a mutex inside atomic context. >>>> Ethtool operations are already serialized by RTNL mutex, so it is safe to drop the locks. >>>> >>>> Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org> >>>> --- >>>> drivers/net/3c59x.c | 6 ++---- >>>> 1 files changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c >>>> index 7a01588..a5c697b 100644 >>>> --- a/drivers/net/3c59x.c >>>> +++ b/drivers/net/3c59x.c >>>> @@ -634,6 +634,8 @@ struct vortex_private { >>>> medialock:1, >>>> must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */ >>>> large_frames:1; /* accept large frames */ >>>> + /* {get|set}_wol operations are already serialized by rtnl. >>>> + * no additional locking is required for the enable_wol and acpi_set_WOL() */ >>> >>> Please format this comment like this : >>> >>> /* This is a fine ............................ >>> * and long comment .................................. >>> */ >>> >>>> int drv_flags; >>>> u16 status_enable; >>>> u16 intr_enable; >>>> @@ -2922,13 +2924,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >>>> { >>>> struct vortex_private *vp = netdev_priv(dev); >>>> >>>> - spin_lock_irq(&vp->lock); >>>> wol->supported = WAKE_MAGIC; >>>> >>>> wol->wolopts = 0; >>>> if (vp->enable_wol) >>>> wol->wolopts |= WAKE_MAGIC; >>>> - spin_unlock_irq(&vp->lock); >>>> } >>>> >>>> static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >>>> @@ -2937,13 +2937,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >>>> if (wol->wolopts & ~WAKE_MAGIC) >>>> return -EINVAL; >>>> >>>> - spin_lock_irq(&vp->lock); >>>> if (wol->wolopts & WAKE_MAGIC) >>>> vp->enable_wol = 1; >>>> else >>>> vp->enable_wol = 0; >>>> acpi_set_WOL(dev); >>>> - spin_unlock_irq(&vp->lock); >>>> >>>> return 0; >>>> } >>> >>> >> Comments fixed, thanks Eric >> > > Yes, but you forgot to submit your patch completely, with changelog and > your Signned-off-by ;) > >> --- >> drivers/net/3c59x.c | 7 +++---- >> 1 files changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c >> index 7a01588..afcad74 100644 >> --- a/drivers/net/3c59x.c >> +++ b/drivers/net/3c59x.c >> @@ -634,6 +634,9 @@ struct vortex_private { >> medialock:1, >> must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */ >> large_frames:1; /* accept large frames */ >> + /* {get|set}_wol operations are already serialized by rtnl. >> + * no additional locking is required for the enable_wol and acpi_set_WOL() >> + */ >> int drv_flags; >> u16 status_enable; >> u16 intr_enable; >> @@ -2922,13 +2925,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> { >> struct vortex_private *vp = netdev_priv(dev); >> >> - spin_lock_irq(&vp->lock); >> wol->supported = WAKE_MAGIC; >> >> wol->wolopts = 0; >> if (vp->enable_wol) >> wol->wolopts |= WAKE_MAGIC; >> - spin_unlock_irq(&vp->lock); >> } >> >> static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> @@ -2937,13 +2938,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) >> if (wol->wolopts & ~WAKE_MAGIC) >> return -EINVAL; >> >> - spin_lock_irq(&vp->lock); >> if (wol->wolopts & WAKE_MAGIC) >> vp->enable_wol = 1; >> else >> vp->enable_wol = 0; >> acpi_set_WOL(dev); >> - spin_unlock_irq(&vp->lock); >> >> return 0; >> } > > Next attempt. With a changelog :) Thanks. [PATCH v2] 3c59x: Remove atomic context inside vortex_{set|get}_wol There is no need to use spinlocks in vortex_{set|get}_wol. This also fixes a bug: [ 254.214993] 3c59x 0000:00:0d.0: PME# enabled [ 254.215021] BUG: sleeping function called from invalid context at kernel/mutex.c:94 [ 254.215030] in_atomic(): 0, irqs_disabled(): 1, pid: 4875, name: ethtool [ 254.215042] Pid: 4875, comm: ethtool Tainted: G W 2.6.36-rc3+ #7 [ 254.215049] Call Trace: [ 254.215050] [] __might_sleep+0xb1/0xb6 [ 254.215050] [] mutex_lock+0x17/0x30 [ 254.215050] [] acpi_enable_wakeup_device_power+0x2b/0xb1 [ 254.215050] [] acpi_pm_device_sleep_wake+0x42/0x7f [ 254.215050] [] acpi_pci_sleep_wake+0x5d/0x63 [ 254.215050] [] platform_pci_sleep_wake+0x1d/0x20 [ 254.215050] [] __pci_enable_wake+0x90/0xd0 [ 254.215050] [] acpi_set_WOL+0x8e/0xf5 [3c59x] [ 254.215050] [] vortex_set_wol+0x4e/0x5e [3c59x] [ 254.215050] [] dev_ethtool+0x1cf/0xb61 [ 254.215050] [] ? debug_mutex_free_waiter+0x45/0x4a [ 254.215050] [] ? __mutex_lock_common+0x204/0x20e [ 254.215050] [] ? __mutex_lock_slowpath+0x12/0x15 [ 254.215050] [] ? mutex_lock+0x23/0x30 [ 254.215050] [] dev_ioctl+0x42c/0x533 [ 254.215050] [] ? _cond_resched+0x8/0x1c [ 254.215050] [] ? lock_page+0x1c/0x30 [ 254.215050] [] ? page_address+0x15/0x7c [ 254.215050] [] ? filemap_fault+0x187/0x2c4 [ 254.215050] [] sock_ioctl+0x1d4/0x1e0 [ 254.215050] [] ? sock_ioctl+0x0/0x1e0 [ 254.215050] [] vfs_ioctl+0x19/0x33 [ 254.215050] [] do_vfs_ioctl+0x424/0x46f [ 254.215050] [] ? selinux_file_ioctl+0x3c/0x40 [ 254.215050] [] sys_ioctl+0x40/0x5a [ 254.215050] [] sysenter_do_call+0x12/0x22 vortex_set_wol protected with a spinlock, but nested acpi_set_WOL acquires a mutex inside atomic context. Ethtool operations are already serialized by RTNL mutex, so it is safe to drop the locks. Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org> --- V2: fixed comments issue drivers/net/3c59x.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c index 7a01588..afcad74 100644 --- a/drivers/net/3c59x.c +++ b/drivers/net/3c59x.c @@ -634,6 +634,9 @@ struct vortex_private { medialock:1, must_free_region:1, /* Flag: if zero, Cardbus owns the I/O region */ large_frames:1; /* accept large frames */ + /* {get|set}_wol operations are already serialized by rtnl. + * no additional locking is required for the enable_wol and acpi_set_WOL() + */ int drv_flags; u16 status_enable; u16 intr_enable; @@ -2922,13 +2925,11 @@ static void vortex_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct vortex_private *vp = netdev_priv(dev); - spin_lock_irq(&vp->lock); wol->supported = WAKE_MAGIC; wol->wolopts = 0; if (vp->enable_wol) wol->wolopts |= WAKE_MAGIC; - spin_unlock_irq(&vp->lock); } static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) @@ -2937,13 +2938,11 @@ static int vortex_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if (wol->wolopts & ~WAKE_MAGIC) return -EINVAL; - spin_lock_irq(&vp->lock); if (wol->wolopts & WAKE_MAGIC) vp->enable_wol = 1; else vp->enable_wol = 0; acpi_set_WOL(dev); - spin_unlock_irq(&vp->lock); return 0; } -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol 2010-09-15 10:58 ` Denis Kirjanov @ 2010-09-15 21:36 ` David Miller 0 siblings, 0 replies; 5+ messages in thread From: David Miller @ 2010-09-15 21:36 UTC (permalink / raw) To: dkirjanov; +Cc: netdev, eric.dumazet, ben From: Denis Kirjanov <dkirjanov@kernel.org> Date: Wed, 15 Sep 2010 14:58:46 +0400 > [PATCH v2] 3c59x: Remove atomic context inside vortex_{set|get}_wol > > There is no need to use spinlocks in vortex_{set|get}_wol. > This also fixes a bug: ... > vortex_set_wol protected with a spinlock, but nested acpi_set_WOL acquires a mutex inside atomic context. > Ethtool operations are already serialized by RTNL mutex, so it is safe to drop the locks. > > Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org> > --- > V2: fixed comments issue Applied, thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-09-15 21:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-15 6:24 [PATCH] 3c59x: Remove atomic context inside vortex_{set|get}_wol Denis Kirjanov
2010-09-15 6:35 ` Eric Dumazet
2010-09-15 10:15 ` Denis Kirjanov
[not found] ` <1284547021.2495.1.camel@edumazet-laptop>
2010-09-15 10:58 ` Denis Kirjanov
2010-09-15 21:36 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).