[PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[Ben Hutchings]

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.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
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(-)

diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index c685a55..a045559 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -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);
 
-- 
1.7.1

Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Tue, 31 Aug 2010 01:15:33 +0100

> 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.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> 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.

Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On Wed, 2010-09-01 at 14:38 -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Tue, 31 Aug 2010 01:15:33 +0100
> 
> > 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.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> > 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.

Hmm, yes, I forgot that mii caches information in struct mii_if_info.
Let me rethink this.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

On Wed, 2010-09-01 at 22:47 +0100, Ben Hutchings wrote:
> On Wed, 2010-09-01 at 14:38 -0700, David Miller wrote:
> > From: Ben Hutchings <ben@decadent.org.uk>
> > Date: Tue, 31 Aug 2010 01:15:33 +0100
> > 
> > > 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.
> > > 
> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > > ---
> > > 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.
> 
> 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.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 01 Sep 2010 23:08:27 +0100

> 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.

Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Wed, 2010-09-01 at 17:37 -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Wed, 01 Sep 2010 23:08:27 +0100
> 
> > 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.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Thu, 02 Sep 2010 01:42:58 +0100

> On Wed, 2010-09-01 at 17:37 -0700, David Miller wrote:
>> From: Ben Hutchings <ben@decadent.org.uk>
>> Date: Wed, 01 Sep 2010 23:08:27 +0100
>> 
>> > 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.

Gotcha, patch applied to net-2.6, thanks.