* Re: via_rhine modules error on 2.6.16 with mii-tool
[not found] ` <20060327203946.GA11824@k3.hellgate.ch>
@ 2006-04-13 18:26 ` John W. Linville
2006-04-13 18:40 ` Stephen Hemminger
2006-04-13 20:31 ` Roger Luethi
0 siblings, 2 replies; 9+ messages in thread
From: John W. Linville @ 2006-04-13 18:26 UTC (permalink / raw)
To: Roger Luethi; +Cc: Marco Berizzi, netdev
On Mon, Mar 27, 2006 at 10:39:46PM +0200, Roger Luethi wrote:
> On Fri, 24 Mar 2006 16:49:10 +0100, Marco Berizzi wrote:
> > Hello evebody.
> > I get this error on linux vanilla 2.6.16
> > with via_rhine module loaded when
> > I run mii-tool:
>
> That was caused by a recent change that replaced an mdelay with msleep.
>
> netdev_ioctl and friends (ethtool calls, too) are known to grab a spin lock
> before they do much of anything, and they hang onto it until they're done.
> They also call mdio_read/write, which requires millisecond delays on Rhine-I.
>
> So on Rhine-I with a 2.6.15+ kernel, the driver ends up calling msleep
> while holding a spin lock -- hence the stack dump.
>
> I wonder if low latency for ancient Rhine-I chips is worth the trouble.
IIRC, the point was that mdelay was getting called in interrupt
context and causing ugly messages to show-up in dmesg.
Would the patch below be sufficient? Or does the whole patch need
to be reverted?
John
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index 2418715..e7b4bc3 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -1145,8 +1130,8 @@ static void rhine_disable_linkmon(void _
if (quirks & rqRhineI) {
iowrite8(0x01, ioaddr + MIIRegAddr); // MII_BMSR
- /* Do not call from ISR! */
- msleep(1);
+ /* Can be called from ISR. Evil. */
+ mdelay(1);
/* 0x80 must be set immediately before turning it off */
iowrite8(0x80, ioaddr + MIICmd);
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: via_rhine modules error on 2.6.16 with mii-tool
2006-04-13 18:26 ` via_rhine modules error on 2.6.16 with mii-tool John W. Linville
@ 2006-04-13 18:40 ` Stephen Hemminger
2006-04-13 20:47 ` Roger Luethi
2006-04-13 20:31 ` Roger Luethi
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2006-04-13 18:40 UTC (permalink / raw)
To: John W. Linville; +Cc: Roger Luethi, Marco Berizzi, netdev
On Thu, 13 Apr 2006 14:26:43 -0400
"John W. Linville" <linville@tuxdriver.com> wrote:
> On Mon, Mar 27, 2006 at 10:39:46PM +0200, Roger Luethi wrote:
> > On Fri, 24 Mar 2006 16:49:10 +0100, Marco Berizzi wrote:
> > > Hello evebody.
> > > I get this error on linux vanilla 2.6.16
> > > with via_rhine module loaded when
> > > I run mii-tool:
> >
> > That was caused by a recent change that replaced an mdelay with msleep.
> >
> > netdev_ioctl and friends (ethtool calls, too) are known to grab a spin lock
> > before they do much of anything, and they hang onto it until they're done.
> > They also call mdio_read/write, which requires millisecond delays on Rhine-I.
> >
> > So on Rhine-I with a 2.6.15+ kernel, the driver ends up calling msleep
> > while holding a spin lock -- hence the stack dump.
> >
> > I wonder if low latency for ancient Rhine-I chips is worth the trouble.
>
> IIRC, the point was that mdelay was getting called in interrupt
> context and causing ugly messages to show-up in dmesg.
>
> Would the patch below be sufficient? Or does the whole patch need
> to be reverted?
>
> John
>
> diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
> index 2418715..e7b4bc3 100644
> --- a/drivers/net/via-rhine.c
> +++ b/drivers/net/via-rhine.c
> @@ -1145,8 +1130,8 @@ static void rhine_disable_linkmon(void _
> if (quirks & rqRhineI) {
> iowrite8(0x01, ioaddr + MIIRegAddr); // MII_BMSR
>
> - /* Do not call from ISR! */
> - msleep(1);
> + /* Can be called from ISR. Evil. */
> + mdelay(1);
>
> /* 0x80 must be set immediately before turning it off */
> iowrite8(0x80, ioaddr + MIICmd);
The right thing to do is get rid of the locking in via_rhine:netdev_ioctl and push
the locking down into mdio_read, mdio_write.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: via_rhine modules error on 2.6.16 with mii-tool
2006-04-13 18:26 ` via_rhine modules error on 2.6.16 with mii-tool John W. Linville
2006-04-13 18:40 ` Stephen Hemminger
@ 2006-04-13 20:31 ` Roger Luethi
2006-05-19 14:41 ` [patch] via-rhine: revert "change mdelay to msleep and remove from ISR path" John W. Linville
1 sibling, 1 reply; 9+ messages in thread
From: Roger Luethi @ 2006-04-13 20:31 UTC (permalink / raw)
To: John W. Linville; +Cc: Marco Berizzi, netdev
On Thu, 13 Apr 2006 14:26:43 -0400, John W. Linville wrote:
> > I wonder if low latency for ancient Rhine-I chips is worth the trouble.
>
> IIRC, the point was that mdelay was getting called in interrupt
> context and causing ugly messages to show-up in dmesg.
I suppose the patch back then was to reduce latency; the ugly messages in
the kernel ring buffer were _introduced_ with the patch (you shouldn't get
error messages calling mdelay in interrupt context because that's what
mdelay is for).
> Would the patch below be sufficient? Or does the whole patch need
> to be reverted?
I'd revert the whole thing. There's no point in having the additional
work_struct complexity if we end up calling mdelay anyway.
Roger
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: via_rhine modules error on 2.6.16 with mii-tool
2006-04-13 18:40 ` Stephen Hemminger
@ 2006-04-13 20:47 ` Roger Luethi
2006-04-13 21:02 ` Stephen Hemminger
0 siblings, 1 reply; 9+ messages in thread
From: Roger Luethi @ 2006-04-13 20:47 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: John W. Linville, Marco Berizzi, netdev
On Thu, 13 Apr 2006 11:40:18 -0700, Stephen Hemminger wrote:
> The right thing to do is get rid of the locking in via_rhine:netdev_ioctl
> and push the locking down into mdio_read, mdio_write.
As I said before, a dozen other network drivers do the exact same thing --
they call generic_mii_ioctl right after grabbing the private spin lock (and
only one driver calls generic_mii_ioctl without taking the lock).
I am not keen on patches that make via-rhine more of a special case even if
it was safe now; next thing you know generic_mii_ioctl is changed in a way
that breaks the only driver that foolishly made assumptions about the
side-effects of that function.
If you can safely move the locking down for all network drivers, that would
be a different story, of course.
Roger
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: via_rhine modules error on 2.6.16 with mii-tool
2006-04-13 20:47 ` Roger Luethi
@ 2006-04-13 21:02 ` Stephen Hemminger
2006-04-13 21:36 ` Francois Romieu
2006-04-13 21:55 ` Roger Luethi
0 siblings, 2 replies; 9+ messages in thread
From: Stephen Hemminger @ 2006-04-13 21:02 UTC (permalink / raw)
To: Roger Luethi; +Cc: John W. Linville, Marco Berizzi, netdev
On Thu, 13 Apr 2006 22:47:31 +0200
Roger Luethi <rl@hellgate.ch> wrote:
> On Thu, 13 Apr 2006 11:40:18 -0700, Stephen Hemminger wrote:
> > The right thing to do is get rid of the locking in via_rhine:netdev_ioctl
> > and push the locking down into mdio_read, mdio_write.
>
> As I said before, a dozen other network drivers do the exact same thing --
> they call generic_mii_ioctl right after grabbing the private spin lock (and
> only one driver calls generic_mii_ioctl without taking the lock).
>
>
> I am not keen on patches that make via-rhine more of a special case even if
> it was safe now; next thing you know generic_mii_ioctl is changed in a way
> that breaks the only driver that foolishly made assumptions about the
> side-effects of that function.
>
> If you can safely move the locking down for all network drivers, that would
> be a different story, of course.
>
> Roger
Didn't your mother ever tell you that just because everybody else does
it wrong, you don't have to.
The other drivers should be fixed as well. Phy access with irq's disabled
is not good. The hardware I checked takes 100's of usecs to do one read
transaction.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: via_rhine modules error on 2.6.16 with mii-tool
2006-04-13 21:02 ` Stephen Hemminger
@ 2006-04-13 21:36 ` Francois Romieu
2006-04-13 21:55 ` Roger Luethi
1 sibling, 0 replies; 9+ messages in thread
From: Francois Romieu @ 2006-04-13 21:36 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Roger Luethi, John W. Linville, Marco Berizzi, netdev
Stephen Hemminger <shemminger@osdl.org> :
[...]
> The other drivers should be fixed as well. Phy access with irq's disabled
> is not good. The hardware I checked takes 100's of usecs to do one read
> transaction.
Yep, 802.3 allows it. :o|
It's surprizing that the low latency squadron has not complained so far
from our bad habit of spinlocked, irq disabled, MII access.
--
Ueimor
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: via_rhine modules error on 2.6.16 with mii-tool
2006-04-13 21:02 ` Stephen Hemminger
2006-04-13 21:36 ` Francois Romieu
@ 2006-04-13 21:55 ` Roger Luethi
1 sibling, 0 replies; 9+ messages in thread
From: Roger Luethi @ 2006-04-13 21:55 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: John W. Linville, Marco Berizzi, netdev
On Thu, 13 Apr 2006 14:02:52 -0700, Stephen Hemminger wrote:
> > I am not keen on patches that make via-rhine more of a special case even if
> > it was safe now; next thing you know generic_mii_ioctl is changed in a way
> > that breaks the only driver that foolishly made assumptions about the
> > side-effects of that function.
> >
> > If you can safely move the locking down for all network drivers, that would
> > be a different story, of course.
>
> Didn't your mother ever tell you that just because everybody else does
> it wrong, you don't have to.
No, but she warned me not to spend time on fixing botched low latency
patches.
Look, it took a lot of time to make via-rhine stable. It's still got
unexplained issues. I have a patch here for a bug that makes a driver
reload necessary when it occurs (and the patch is sitting here because
nobody's able to reproduce the problem anymore). I am lacking adequate
documentation, I have little time to work on the driver, but quite a
to do list.
Does that sound like via-rhine would make a good guinea pig?
> The other drivers should be fixed as well. Phy access with irq's disabled
> is not good. The hardware I checked takes 100's of usecs to do one read
> transaction.
If you want to fix this in all drivers, more power to you. It is just not
high on my own priority list. I see the need for low latency, but latency
issues that only happen when people fiddle with MII settings don't seem all
that dramatic to me.
Roger
^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch] via-rhine: revert "change mdelay to msleep and remove from ISR path"
2006-04-13 20:31 ` Roger Luethi
@ 2006-05-19 14:41 ` John W. Linville
2006-05-19 14:51 ` [patch w/o fuzz] " John W. Linville
0 siblings, 1 reply; 9+ messages in thread
From: John W. Linville @ 2006-05-19 14:41 UTC (permalink / raw)
To: Roger Luethi, jeff; +Cc: Marco Berizzi, netdev
Revert previous patch with subject "change mdelay to msleep and remove
from ISR path". This patch seems to have caused bigger problems than
it solved, and it didn't solve much of a problem to begin with...
Signed-off-by: John W. Linville <linville@tuxdriver.com
---
Discussion about backing-out this patch can be found here:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114321570402396&w=2
The git commit associated w/ the original patch is:
6ba98d311d0a4ff7dc36d8f435ce60174f4c30ec
drivers/net/via-rhine.c | 34 +++-------------------------------
1 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index 2418715..e7b4bc3 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -490,8 +490,6 @@ struct rhine_private {
u8 tx_thresh, rx_thresh;
struct mii_if_info mii_if;
- struct work_struct tx_timeout_task;
- struct work_struct check_media_task;
void __iomem *base;
};
@@ -499,8 +497,6 @@ static int mdio_read(struct net_device
static void mdio_write(struct net_device *dev, int phy_id, int location, int value);
static int rhine_open(struct net_device *dev);
static void rhine_tx_timeout(struct net_device *dev);
-static void rhine_tx_timeout_task(struct net_device *dev);
-static void rhine_check_media_task(struct net_device *dev);
static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev);
static irqreturn_t rhine_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
static void rhine_tx(struct net_device *dev);
@@ -855,12 +851,6 @@ #endif
if (rp->quirks & rqRhineI)
dev->features |= NETIF_F_SG|NETIF_F_HW_CSUM;
- INIT_WORK(&rp->tx_timeout_task,
- (void (*)(void *))rhine_tx_timeout_task, dev);
-
- INIT_WORK(&rp->check_media_task,
- (void (*)(void *))rhine_check_media_task, dev);
-
/* dev->name not defined before register_netdev()! */
rc = register_netdev(dev);
if (rc)
@@ -1087,11 +1077,6 @@ static void rhine_check_media(struct net
ioaddr + ChipCmd1);
}
-static void rhine_check_media_task(struct net_device *dev)
-{
- rhine_check_media(dev, 0);
-}
-
static void init_registers(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
@@ -1145,8 +1130,8 @@ static void rhine_disable_linkmon(void _
if (quirks & rqRhineI) {
iowrite8(0x01, ioaddr + MIIRegAddr); // MII_BMSR
- /* Do not call from ISR! */
- msleep(1);
+ /* Can be called from ISR. Evil. */
+ mdelay(1);
/* 0x80 must be set immediately before turning it off */
iowrite8(0x80, ioaddr + MIICmd);
@@ -1236,16 +1221,6 @@ static int rhine_open(struct net_device
static void rhine_tx_timeout(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
-
- /*
- * Move bulk of work outside of interrupt context
- */
- schedule_work(&rp->tx_timeout_task);
-}
-
-static void rhine_tx_timeout_task(struct net_device *dev)
-{
- struct rhine_private *rp = netdev_priv(dev);
void __iomem *ioaddr = rp->base;
printk(KERN_WARNING "%s: Transmit timed out, status %4.4x, PHY status "
@@ -1651,7 +1626,7 @@ static void rhine_error(struct net_devic
spin_lock(&rp->lock);
if (intr_status & IntrLinkChange)
- schedule_work(&rp->check_media_task);
+ rhine_check_media(dev, 0);
if (intr_status & IntrStatsMax) {
rp->stats.rx_crc_errors += ioread16(ioaddr + RxCRCErrs);
rp->stats.rx_missed_errors += ioread16(ioaddr + RxMissed);
@@ -1899,9 +1874,6 @@ static int rhine_close(struct net_device
spin_unlock_irq(&rp->lock);
free_irq(rp->pdev->irq, dev);
-
- flush_scheduled_work();
-
free_rbufs(dev);
free_tbufs(dev);
free_ring(dev);
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [patch w/o fuzz] via-rhine: revert "change mdelay to msleep and remove from ISR path"
2006-05-19 14:41 ` [patch] via-rhine: revert "change mdelay to msleep and remove from ISR path" John W. Linville
@ 2006-05-19 14:51 ` John W. Linville
0 siblings, 0 replies; 9+ messages in thread
From: John W. Linville @ 2006-05-19 14:51 UTC (permalink / raw)
To: Roger Luethi, jeff; +Cc: Marco Berizzi, netdev
Revert previous patch with subject "change mdelay to msleep and remove
from ISR path". This patch seems to have caused bigger problems than
it solved, and it didn't solve much of a problem to begin with...
Signed-off-by: John W. Linville <linville@tuxdriver.com
---
Use this one, it applies w/o fuzz...
Discussion about backing-out this patch can be found here:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114321570402396&w=2
The git commit associated w/ the original patch is:
6ba98d311d0a4ff7dc36d8f435ce60174f4c30ec
drivers/net/via-rhine.c | 34 +++-------------------------------
1 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index a6dc53b..fdc2103 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -491,8 +491,6 @@ struct rhine_private {
u8 tx_thresh, rx_thresh;
struct mii_if_info mii_if;
- struct work_struct tx_timeout_task;
- struct work_struct check_media_task;
void __iomem *base;
};
@@ -500,8 +498,6 @@ static int mdio_read(struct net_device
static void mdio_write(struct net_device *dev, int phy_id, int location, int value);
static int rhine_open(struct net_device *dev);
static void rhine_tx_timeout(struct net_device *dev);
-static void rhine_tx_timeout_task(struct net_device *dev);
-static void rhine_check_media_task(struct net_device *dev);
static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev);
static irqreturn_t rhine_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
static void rhine_tx(struct net_device *dev);
@@ -856,12 +852,6 @@ #endif
if (rp->quirks & rqRhineI)
dev->features |= NETIF_F_SG|NETIF_F_HW_CSUM;
- INIT_WORK(&rp->tx_timeout_task,
- (void (*)(void *))rhine_tx_timeout_task, dev);
-
- INIT_WORK(&rp->check_media_task,
- (void (*)(void *))rhine_check_media_task, dev);
-
/* dev->name not defined before register_netdev()! */
rc = register_netdev(dev);
if (rc)
@@ -1108,11 +1098,6 @@ static void rhine_set_carrier(struct mii
netif_carrier_ok(mii->dev));
}
-static void rhine_check_media_task(struct net_device *dev)
-{
- rhine_check_media(dev, 0);
-}
-
static void init_registers(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
@@ -1166,8 +1151,8 @@ static void rhine_disable_linkmon(void _
if (quirks & rqRhineI) {
iowrite8(0x01, ioaddr + MIIRegAddr); // MII_BMSR
- /* Do not call from ISR! */
- msleep(1);
+ /* Can be called from ISR. Evil. */
+ mdelay(1);
/* 0x80 must be set immediately before turning it off */
iowrite8(0x80, ioaddr + MIICmd);
@@ -1257,16 +1242,6 @@ static int rhine_open(struct net_device
static void rhine_tx_timeout(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
-
- /*
- * Move bulk of work outside of interrupt context
- */
- schedule_work(&rp->tx_timeout_task);
-}
-
-static void rhine_tx_timeout_task(struct net_device *dev)
-{
- struct rhine_private *rp = netdev_priv(dev);
void __iomem *ioaddr = rp->base;
printk(KERN_WARNING "%s: Transmit timed out, status %4.4x, PHY status "
@@ -1677,7 +1652,7 @@ static void rhine_error(struct net_devic
spin_lock(&rp->lock);
if (intr_status & IntrLinkChange)
- schedule_work(&rp->check_media_task);
+ rhine_check_media(dev, 0);
if (intr_status & IntrStatsMax) {
rp->stats.rx_crc_errors += ioread16(ioaddr + RxCRCErrs);
rp->stats.rx_missed_errors += ioread16(ioaddr + RxMissed);
@@ -1927,9 +1902,6 @@ static int rhine_close(struct net_device
spin_unlock_irq(&rp->lock);
free_irq(rp->pdev->irq, dev);
-
- flush_scheduled_work();
-
free_rbufs(dev);
free_tbufs(dev);
free_ring(dev);
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-05-19 14:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <BAY103-F401F96F0B06D75E70AF0BFB2DF0@phx.gbl>
[not found] ` <20060327203946.GA11824@k3.hellgate.ch>
2006-04-13 18:26 ` via_rhine modules error on 2.6.16 with mii-tool John W. Linville
2006-04-13 18:40 ` Stephen Hemminger
2006-04-13 20:47 ` Roger Luethi
2006-04-13 21:02 ` Stephen Hemminger
2006-04-13 21:36 ` Francois Romieu
2006-04-13 21:55 ` Roger Luethi
2006-04-13 20:31 ` Roger Luethi
2006-05-19 14:41 ` [patch] via-rhine: revert "change mdelay to msleep and remove from ISR path" John W. Linville
2006-05-19 14:51 ` [patch w/o fuzz] " John W. Linville
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).