3c59x: Remove incorrect locking; correct documented lock hierarchy
Subsystems (2)
PATCH 1/1 3c59x: Remove incorrect locking; correct documented lock hierarchy
vortex_ioctl() was grabbing vortex_private::lock around its call to generic_mii_ioctl(). This is no longer necessary since there are more specific locks which the mdio_{read,write}() functions will obtain. Worse, those functions do not save and restore IRQ flags when locking the MII state, so interrupts will be enabled when generic_mii_ioctl() returns.
Since there is currently no need for any function to call mdio_{read,write}() while holding another spinlock, do not change them to save and restore IRQ flags but remove the specification of ordering between vortex_private::lock and vortex_private::mii_lock.
---
I've now borrowed a card to test 3c59x on. I've seen another regression reported <http://bugs.debian.org/586967> after my locking changes, which I can't reproduce.
Ben.
drivers/net/3c59x.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
drivers/net/3c59x.c +1 -4
@@ -647,7 +647,7 @@ struct vortex_private {u16 io_size;/* Size of PCI region (for release_region) */ /* Serialises access to hardware other than MII and variables below. -* The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */ +* The lock hierarchy is rtnl_lock > {lock, mii_lock} > window_lock. */ spinlock_t lock;spinlock_t mii_lock;/* Serialises access to MII */
@@ -2984,7 +2984,6 @@ static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) {int err;struct vortex_private *vp =netdev_priv (dev); -unsigned long flags;pci_power_t state =0 ;if (VORTEX_PCI (vp))
@@ -2994,9 +2993,7 @@ static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)if (state !=0 )pci_set_power_state (VORTEX_PCI (vp),PCI_D0 ); -spin_lock_irqsave (&vp->lock, flags); err =generic_mii_ioctl (&vp->mii,if_mii (rq), cmd,NULL ); -spin_unlock_irqrestore (&vp->lock, flags);if (state !=0 )pci_set_power_state (VORTEX_PCI (vp), state);
Signature · 1 line
1.7.1
David Miller
maintainerFrom: Ben Hutchings <[email protected]>
Date: Tue, 31 Aug 2010 01:15:33 +0100
Quoted 12 lines
vortex_ioctl()was grabbingvortex_private::lockaround its call togeneric_mii_ioctl(). This is no longer necessary since there are more specific locks which the mdio_{read,write}() functions will obtain. Worse, those functions do not save and restore IRQ flags when locking the MII state, so interrupts will be enabled whengeneric_mii_ioctl()returns.Since there is currently no need for any function to call mdio_{read,write}() while holding another spinlock, do not change them to save and restore IRQ flags but remove the specification of ordering between
vortex_private::lockandvortex_private::mii_lock.
Quoted 5 lines
Signed-off-by Ben Hutchings <[email protected]>---
I've now borrowed a card to test 3c59x on. I've seen another regression reported <http://bugs.debian.org/586967> after my locking changes, which I can't reproduce.
I think the lock is necessary, in some form.
Nothing otherwise protects vp->mii, which is accessed and modified by not just this ioctl, but also ethtool operation calls.
So we can't apply your patch as-is.
Quoted 21 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Tue, 31 Aug 2010 01:15:33 +0100Quoted 12 lines
vortex_ioctl()was grabbingvortex_private::lockaround its call togeneric_mii_ioctl(). This is no longer necessary since there are more specific locks which the mdio_{read,write}() functions will obtain. Worse, those functions do not save and restore IRQ flags when locking the MII state, so interrupts will be enabled whengeneric_mii_ioctl()returns.Since there is currently no need for any function to call mdio_{read,write}() while holding another spinlock, do not change them to save and restore IRQ flags but remove the specification of ordering between
vortex_private::lockandvortex_private::mii_lock.Quoted 5 lines
Signed-off-by Ben Hutchings <[email protected]>---
I've now borrowed a card to test 3c59x on. I've seen another regression reported <http://bugs.debian.org/586967> after my locking changes, which I can't reproduce.
Quoted 6 lines
I think the lock is necessary, in some form.
Nothing otherwise protects
vp->mii, which is accessed and modified by not just this ioctl, but also ethtool operation calls.So we can't apply your patch as-is.
Hmm, yes, I forgot that mii caches information in struct mii_if_info. Let me rethink this.
Ben.
Signature · 2 lines
Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.
Quoted 24 lines · Ben Hutchings
Quoted 23 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Tue, 31 Aug 2010 01:15:33 +0100Quoted 12 lines
vortex_ioctl()was grabbingvortex_private::lockaround its call togeneric_mii_ioctl(). This is no longer necessary since there are more specific locks which the mdio_{read,write}() functions will obtain. Worse, those functions do not save and restore IRQ flags when locking the MII state, so interrupts will be enabled whengeneric_mii_ioctl()returns.Since there is currently no need for any function to call mdio_{read,write}() while holding another spinlock, do not change them to save and restore IRQ flags but remove the specification of ordering between
vortex_private::lockandvortex_private::mii_lock.Quoted 5 lines
Signed-off-by Ben Hutchings <[email protected]>---
I've now borrowed a card to test 3c59x on. I've seen another regression reported <http://bugs.debian.org/586967> after my locking changes, which I can't reproduce.I think the lock is necessary, in some form.
Quoted 7 lines
Quoted 4 lines
Nothing otherwise protects
vp->mii, which is accessed and modified by not just this ioctl, but also ethtool operation calls.So we can't apply your patch as-is.
Hmm, yes, I forgot that mii caches information in struct
mii_if_info. Let me rethink this.
I think this is safe after all because ethtool and MII ioctls are all serialised by the RTNL.
Ben.
Signature · 2 lines
Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.
David Miller
maintainerFrom: Ben Hutchings <[email protected]>
Date: Wed, 01 Sep 2010 23:08:27 +0100
Quoted 2 lines
I think this is safe after all because ethtool and MII ioctls are all serialised by the RTNL.
Well since you said that there was a regression that you're unable to reproduce, I was trying to find a probably cause for it :-)
I think you should work to resolve that report, and resend this locking patch once that is sorted out.
Thanks.
Quoted 11 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Wed, 01 Sep 2010 23:08:27 +0100Quoted 2 lines
I think this is safe after all because ethtool and MII ioctls are all serialised by the RTNL.
Well since you said that there was a regression that you're unable to reproduce, I was trying to find a probably cause for it :-)
I think you should work to resolve that report, and resend this locking patch once that is sorted out.
That regression seems to due to my *earlier* changes to locking. This new patch fixes a different regression I found while trying to reproduce that, and it has not been applied in Debian yet. Sorry for the ambiguous wording.
Ben.
Signature · 2 lines
Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.
David Miller
maintainerFrom: Ben Hutchings <[email protected]>
Date: Thu, 02 Sep 2010 01:42:58 +0100
Quoted 10 lines · David Miller
Quoted 9 lines
From: Ben Hutchings <[email protected]>
Date: Wed, 01 Sep 2010 23:08:27 +0100Quoted 2 lines
I think this is safe after all because ethtool and MII ioctls are all serialised by the RTNL.
Well since you said that there was a regression that you're unable to reproduce, I was trying to find a probably cause for it :-)
Quoted 7 lines
Quoted 2 lines
I think you should work to resolve that report, and resend this locking patch once that is sorted out.
That regression seems to due to my *earlier* changes to locking. This new patch fixes a different regression I found while trying to reproduce that, and it has not been applied in Debian yet. Sorry for the ambiguous wording.
Gotcha, patch applied to net-2.6, thanks.