* 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: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
* 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
* [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).