* [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
@ 2010-08-31 0:15 Ben Hutchings
2010-09-01 21:38 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2010-08-31 0:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
2010-08-31 0:15 [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy Ben Hutchings
@ 2010-09-01 21:38 ` David Miller
2010-09-01 21:47 ` Ben Hutchings
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2010-09-01 21:38 UTC (permalink / raw)
To: ben; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
2010-09-01 21:38 ` David Miller
@ 2010-09-01 21:47 ` Ben Hutchings
2010-09-01 22:08 ` Ben Hutchings
0 siblings, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2010-09-01 21:47 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
2010-09-01 21:47 ` Ben Hutchings
@ 2010-09-01 22:08 ` Ben Hutchings
2010-09-02 0:37 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2010-09-01 22:08 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
2010-09-01 22:08 ` Ben Hutchings
@ 2010-09-02 0:37 ` David Miller
2010-09-02 0:42 ` Ben Hutchings
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2010-09-02 0:37 UTC (permalink / raw)
To: ben; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
2010-09-02 0:37 ` David Miller
@ 2010-09-02 0:42 ` Ben Hutchings
2010-09-02 1:02 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2010-09-02 0:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy
2010-09-02 0:42 ` Ben Hutchings
@ 2010-09-02 1:02 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2010-09-02 1:02 UTC (permalink / raw)
To: ben; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-09-02 1:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-31 0:15 [PATCH net-2.6] 3c59x: Remove incorrect locking; correct documented lock hierarchy Ben Hutchings
2010-09-01 21:38 ` David Miller
2010-09-01 21:47 ` Ben Hutchings
2010-09-01 22:08 ` Ben Hutchings
2010-09-02 0:37 ` David Miller
2010-09-02 0:42 ` Ben Hutchings
2010-09-02 1:02 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.