* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Stefan Richter @ 2007-08-16 9:54 UTC (permalink / raw)
To: Herbert Xu
Cc: Paul Mackerras, Satyam Sharma, Christoph Lameter,
Paul E. McKenney, Chris Snook, Linux Kernel Mailing List,
linux-arch, Linus Torvalds, netdev, Andrew Morton, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl, segher
In-Reply-To: <20070816081049.GA1431@gondor.apana.org.au>
Herbert Xu wrote:
> On Thu, Aug 16, 2007 at 10:06:31AM +0200, Stefan Richter wrote:
>> >
>> > Do you (or anyone else for that matter) have an example of this?
>>
>> The only code I somewhat know, the ieee1394 subsystem, was perhaps
>> authored and is currently maintained with the expectation that each
>> occurrence of atomic_read actually results in a load operation, i.e. is
>> not optimized away. This means all atomic_t (bus generation, packet and
>> buffer refcounts, and some other state variables)* and likewise all
>> atomic bitops in that subsystem.
>
> Can you find an actual atomic_read code snippet there that is
> broken without the volatile modifier?
What do I have to look for? atomic_read after another read or write
access to the same variable, in the same function scope? Or in the sum
of scopes of functions that could be merged by function inlining?
One example was discussed here earlier: The for (;;) loop in
nodemgr_host_thread. There an msleep_interruptible implicitly acted as
barrier (at the moment because it's in a different translation unit; if
it were the same, then because it hopefully has own barriers). So that
happens to work, although such an implicit barrier is bad style: Better
enforce the desired behaviour (== guaranteed load operation) *explicitly*.
--
Stefan Richter
-=====-=-=== =--- =----
http://arcgraph.de/sr/
^ permalink raw reply
* Re: drivers/infiniband/mlx/mad.c misplaced ;
From: Ilpo Järvinen @ 2007-08-16 10:22 UTC (permalink / raw)
To: Heiko Carstens
Cc: Joe Perches, Dave Jones, Linux Kernel, rolandd, Chas Williams,
Paul Mundt, isdn4linux, mikep, Netdev, swen, linux390, linux-s390,
jdike, user-mode-linux-devel, user-mode-linux-user,
netfilter-devel, coreteam, Satyam Sharma
In-Reply-To: <20070816084635.GA15985@osiris.boeblingen.de.ibm.com>
...I guess those guys hunting for broken busyloops in the other thread
could also benefit from similar searching commands introduced in this
thread... ...Ccing Satyam to caught their attention too.
> On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> >
> > There's more than a few of these (not inspected).
> >
> > $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" *
...Hmm, I plugged in "a preprocessor" too to manage with non compliant
coding styles :-). Please understand that the line numbers are not an
exact match due to preprocessor changes:
$ for i in `find . -name '*.[ch]'`; do echo $i; indent -npro -kr -i8 -ts8
-sob -l8000 -ss -ncs -cp1 -nhnl -st $i | egrep -n "[[:space:]]if [(].*[)] ;$";
done | grep -B1 "^[^.]"
./arch/arm/mach-omap1/leds-innovator.c
97: if (led_state & LED_STATE_ENABLED) ;
--
./arch/mips/sibyte/cfe/console.c
23: if (written < 0) ;
32: if (written < 0) ;
--
./arch/powerpc/kernel/legacy_serial.c
524: if (0) ;
--
./arch/powerpc/xmon/ppc-opc.c
938: else if (value == 0) ;
--
./arch/sh/boards/se/7343/io.c
137: if (0) ;
--
./arch/um/kernel/tt/tracer.c
254: if (WIFEXITED(status)) ;
--
./arch/x86_64/ia32/ptrace32.c
363: if (__copy_from_user(&child->thread.i387.fxsave, u, sizeof(*u))) ;
--
./arch/x86_64/kernel/traps.c
801: if (eregs == (struct pt_regs *)eregs->rsp) ;
--
./drivers/atm/iphase.c
159: if (!desc1) ;
--
./drivers/isdn/capi/capiutil.c
456: else if (c <= 0x0f) ;
--
./drivers/isdn/hisax/hfc_pci.c
125: if (Read_hfc(cs, HFCPCI_INT_S1)) ;
155: if (Read_hfc(cs, HFCPCI_INT_S1)) ;
1483: if (Read_hfc(cs, HFCPCI_INT_S1)) ;
--
./drivers/isdn/hisax/hfc_sx.c
377: if (Read_hfc(cs, HFCSX_INT_S1)) ;
407: if (Read_hfc(cs, HFCSX_INT_S2)) ;
1246: if (Read_hfc(cs, HFCSX_INT_S1)) ;
--
./drivers/media/video/video-buf.c
1141: if (q->bufs[i]) ;
--
./drivers/net/lp486e.c
777: if (lp->scb.command && i596_timeout(dev, "i596_cleanup_cmd", 100)) ;
785: if (lp->scb.command && i596_timeout(dev, "i596_reset", 100)) ;
794: if (lp->scb.command && i596_timeout(dev, "i596_reset(2)", 400)) ;
820: if (lp->scb.command && i596_timeout(dev, "i596_add_cmd", 100)) ;
1146: if (lp->scb.command && i596_timeout(dev, "interrupt", 40)) ;
1192: if (lp->scb.command && i596_timeout(dev, "i596 interrupt", 100)) ;
1217: if (lp->scb.command && i596_timeout(dev, "i596_close", 200)) ;
--
./drivers/net/ni5010.c
273: if (dev->irq == 0xff) ;
--
./drivers/net/ni52.c
648: if (result & TDR_LNK_OK) ;
--
./drivers/net/sun3_82586.c
498: if (result & TDR_LNK_OK) ;
--
./drivers/pci/hotplug/ibmphp_core.c
418: else if (mode == BUS_MODE_PCI) ;
636: else if (mode == BUS_MODE_PCI) ;
--
./drivers/usb/gadget/file_storage.c
2480: if (protocol_is_scsi()) ;
--
./drivers/usb/host/uhci-debug.c
416: if (i <= SKEL_ISO) ;
419: else if (!uhci->fsbr_is_on) ;
--
./drivers/usb/host/uhci-q.c
541: if (qh->skel == SKEL_ISO) ;
--
./drivers/usb/misc/usbtest.c
1401: if (status != 0) ;
--
./drivers/video/intelfb/intelfbdrv.c
337: if (get_opt_bool(this_opt, "accel", &accel)) ;
338: else if (get_opt_int(this_opt, "vram", &vram)) ;
339: else if (get_opt_bool(this_opt, "hwcursor", &hwcursor)) ;
340: else if (get_opt_bool(this_opt, "mtrr", &mtrr)) ;
341: else if (get_opt_bool(this_opt, "fixed", &fixed)) ;
--
./drivers/video/matrox/matroxfb_DAC1064.c
46: if (fvco <= 100000) ;
--
./drivers/video/matrox/matroxfb_maven.c
298: if (fvco <= 100000000) ;
316: if (fvco <= 100000) ;
--
./fs/hfs/inode.c
72: if (!node) ;
--
./fs/hfsplus/inode.c
67: if (!node) ;
--
./fs/hostfs/hostfs_user.c
300: if (attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
--
./fs/xfs/xfs_bmap.c
2287: if (nullfb || XFS_FSB_TO_AGNO(mp, ap->rval) == fb_agno) ;
--
./fs/xfs/xfs_dir2.c
281: else if ((rval = xfs_dir2_isblock(tp, dp, &v))) ;
--
./fs/xfs/xfs_iomap.c
248: if (io->io_flags & XFS_IOCORE_RT) ;
--
./include/asm-cris/uaccess.h
255: if (n == 0) ;
303: if (n == 0) ;
351: if (n == 0) ;
--
./mm/swapfile.c
791: if (swcount <= 1) ;
--
./net/core/pktgen.c
2256: if (pkt_dev->min_in6_daddr.s6_addr32[0] == 0 && pkt_dev->min_in6_daddr.s6_addr32[1] == 0 && pkt_dev->min_in6_daddr.s6_addr32[2] == 0 && pkt_dev->min_in6_daddr.s6_addr32[3] == 0) ;
--
./net/irda/af_irda.c
1357: if (ret) ;
1358: else if (sk->sk_shutdown & RCV_SHUTDOWN) ;
--
./net/irda/irnetlink.c
105: if (nla_put_string(msg, IRDA_NL_ATTR_IFNAME, dev->name)) ;
--
./net/netfilter/xt_u32.c
38: if (skb->len < 4 || pos > skb->len - 4) ;
--
./sound/pci/au88x0/au88x0_core.c
2076: if (vortex_adbdma_bufshift(vortex, i)) ;
2085: if (vortex_wtdma_bufshift(vortex, i)) ;
--
./sound/pci/au88x0/au88x0_synth.c
352: if (eax == 0) ;
--
./sound/pci/ice1712/ice1724.c
596: if (!ptr) ;
636: if (!ptr) ;
--
./sound/usb/usbmixer.c
1296: if (check_mapped_name(state, unitid, cval->control, kctl->id.name, sizeof(kctl->id.name))) ;
1500: if (len) ;
...some of these are false positives due to constructs like this (not
sure if there's some better alternative):
if (!ptr)
;
else if (ptr->something)
do_it();
else
do_other();
...plus there might be some #ifdefs in that construct too.
--
i.
^ permalink raw reply
* Re: [REVISED PATCH] 3c59x: check return of pci_enable_device()
From: Mark Hindley @ 2007-08-16 10:28 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Jeff Garzik, akpm, netdev
In-Reply-To: <20070815163000.GB619@bayes.mathematik.tu-chemnitz.de>
Revised patch for this.
Mark
commit 5cf33391eba81a49038fa8be8cbad8425b80bf7f
Author: Mark Hindley <mark@hindley.org.uk>
Date: Thu Aug 16 11:26:35 2007 +0100
Check return of pci_enable_device in vortex_up().
Also modify vortex_up to return error to callers. Handle failure of vortex_up in
vortex_open and vortex_resume.
Signed-off-by: Mark Hindley <mark@hindley.org.uk>
diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index 001c66d..7b3050c 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -705,7 +705,7 @@ static struct {
static int vortex_probe1(struct device *gendev, void __iomem *ioaddr, int irq,
int chip_idx, int card_idx);
-static void vortex_up(struct net_device *dev);
+static int vortex_up(struct net_device *dev);
static void vortex_down(struct net_device *dev, int final);
static int vortex_open(struct net_device *dev);
static void mdio_sync(void __iomem *ioaddr, int bits);
@@ -841,8 +841,11 @@ static int vortex_resume(struct pci_dev *pdev)
return -EBUSY;
}
if (netif_running(dev)) {
- vortex_up(dev);
- netif_device_attach(dev);
+ err = vortex_up(dev);
+ if (err)
+ return err;
+ else
+ netif_device_attach(dev);
}
}
return 0;
@@ -1484,19 +1487,24 @@ static void vortex_check_media(struct net_device *dev, unsigned int init)
}
}
-static void
+static int
vortex_up(struct net_device *dev)
{
struct vortex_private *vp = netdev_priv(dev);
void __iomem *ioaddr = vp->ioaddr;
unsigned int config;
- int i, mii_reg1, mii_reg5;
+ int i, mii_reg1, mii_reg5, err;
if (VORTEX_PCI(vp)) {
pci_set_power_state(VORTEX_PCI(vp), PCI_D0); /* Go active */
if (vp->pm_state_valid)
pci_restore_state(VORTEX_PCI(vp));
- pci_enable_device(VORTEX_PCI(vp));
+ err = pci_enable_device(VORTEX_PCI(vp));
+ if (err) {
+ printk(KERN_WARNING "%s: Could not enable device \n",
+ dev->name);
+ goto err_out;
+ }
}
/* Before initializing select the active media port. */
@@ -1660,6 +1668,8 @@ vortex_up(struct net_device *dev)
if (vp->cb_fn_base) /* The PCMCIA people are idiots. */
iowrite32(0x8000, vp->cb_fn_base + 4);
netif_start_queue (dev);
+err_out:
+ return err;
}
static int
@@ -1673,7 +1683,7 @@ vortex_open(struct net_device *dev)
if ((retval = request_irq(dev->irq, vp->full_bus_master_rx ?
&boomerang_interrupt : &vortex_interrupt, IRQF_SHARED, dev->name, dev))) {
printk(KERN_ERR "%s: Could not reserve IRQ %d\n", dev->name, dev->irq);
- goto out;
+ goto err;
}
if (vp->full_bus_master_rx) { /* Boomerang bus master. */
@@ -1702,20 +1712,22 @@ vortex_open(struct net_device *dev)
}
}
retval = -ENOMEM;
- goto out_free_irq;
+ goto err_free_irq;
}
/* Wrap the ring. */
vp->rx_ring[i-1].next = cpu_to_le32(vp->rx_ring_dma);
}
- vortex_up(dev);
- return 0;
+ retval = vortex_up(dev);
+ if (!retval)
+ goto out;
-out_free_irq:
+err_free_irq:
free_irq(dev->irq, dev);
-out:
+err:
if (vortex_debug > 1)
printk(KERN_ERR "%s: vortex_open() fails: returning %d\n", dev->name, retval);
+out:
return retval;
}
^ permalink raw reply related
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Stefan Richter @ 2007-08-16 10:31 UTC (permalink / raw)
To: Herbert Xu
Cc: Paul Mackerras, Satyam Sharma, Christoph Lameter,
Paul E. McKenney, Chris Snook, Linux Kernel Mailing List,
linux-arch, Linus Torvalds, netdev, Andrew Morton, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl, segher
In-Reply-To: <46C41EE4.9090806@s5r6.in-berlin.de>
I wrote:
> Herbert Xu wrote:
>> On Thu, Aug 16, 2007 at 10:06:31AM +0200, Stefan Richter wrote:
[...]
>>> expectation that each
>>> occurrence of atomic_read actually results in a load operation, i.e. is
>>> not optimized away.
[...]
>> Can you find an actual atomic_read code snippet there that is
>> broken without the volatile modifier?
PS: Just to clarify, I'm not speaking for the volatile modifier. I'm
not speaking for any particular implementation of atomic_t and its
accessors at all. All I am saying is that
- we use atomically accessed data types because we concurrently but
locklessly access this data,
- hence a read access to this data that could be optimized away
makes *no sense at all*.
The only sensible read accessor to an atomic datatype is a read accessor
that will not be optimized away.
So, the architecture guys can implement atomic_read however they want
--- as long as it cannot be optimized away.*
PPS: If somebody has code where he can afford to let the compiler
coalesce atomic_read with a previous access to the same data, i.e.
doesn't need and doesn't want all guarantees that the atomic_read API
makes (or IMO should make), then he can replace the atomic_read by a
local temporary variable.
*) Exceptions:
if (known_to_be_false)
read_access(a);
and the like.
--
Stefan Richter
-=====-=-=== =--- =----
http://arcgraph.de/sr/
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Herbert Xu @ 2007-08-16 10:35 UTC (permalink / raw)
To: Stefan Richter
Cc: Paul Mackerras, Satyam Sharma, Christoph Lameter,
Paul E. McKenney, Chris Snook, Linux Kernel Mailing List,
linux-arch, Linus Torvalds, netdev, Andrew Morton, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl, segher
In-Reply-To: <46C41EE4.9090806@s5r6.in-berlin.de>
On Thu, Aug 16, 2007 at 11:54:44AM +0200, Stefan Richter wrote:
>
> One example was discussed here earlier: The for (;;) loop in
> nodemgr_host_thread. There an msleep_interruptible implicitly acted as
> barrier (at the moment because it's in a different translation unit; if
> it were the same, then because it hopefully has own barriers). So that
> happens to work, although such an implicit barrier is bad style: Better
> enforce the desired behaviour (== guaranteed load operation) *explicitly*.
Hmm, it's not bad style at all. Let's assume that everything
is in the same scope. Such a loop must either call a function
that busy-waits, which should always have a cpu_relax or
something equivalent, or it'll call a function that schedules
away which immediately invalidates any values the compiler might
have cached for the atomic_read.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures
From: Herbert Xu @ 2007-08-16 10:42 UTC (permalink / raw)
To: Stefan Richter
Cc: Paul Mackerras, Satyam Sharma, Christoph Lameter,
Paul E. McKenney, Chris Snook, Linux Kernel Mailing List,
linux-arch, Linus Torvalds, netdev, Andrew Morton, ak,
heiko.carstens, davem, schwidefsky, wensong, horms, wjiang,
cfriesen, zlynx, rpjday, jesper.juhl, segher
In-Reply-To: <46C42767.4070104@s5r6.in-berlin.de>
On Thu, Aug 16, 2007 at 12:31:03PM +0200, Stefan Richter wrote:
>
> PS: Just to clarify, I'm not speaking for the volatile modifier. I'm
> not speaking for any particular implementation of atomic_t and its
> accessors at all. All I am saying is that
> - we use atomically accessed data types because we concurrently but
> locklessly access this data,
> - hence a read access to this data that could be optimized away
> makes *no sense at all*.
No sane compiler can optimise away an atomic_read per se.
That's only possible if there's a preceding atomic_set or
atomic_read, with no barriers in the middle.
If that's the case, then one has to conclude that doing
away with the second read is acceptable, as otherwise
a memory (or at least a compiler) barrier should have been
used.
In fact, volatile doesn't guarantee that the memory gets
read anyway. You might be reading some stale value out
of the cache. Granted this doesn't happen on x86 but
when you're coding for the kernel you can't make such
assumptions.
So the point here is that if you don't mind getting a stale
value from the CPU cache when doing an atomic_read, then
surely you won't mind getting a stale value from the compiler
"cache".
> So, the architecture guys can implement atomic_read however they want
> --- as long as it cannot be optimized away.*
They can implement it however they want as long as it stays
atomic.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: drivers/infiniband/mlx/mad.c misplaced ;
From: Karsten Keil @ 2007-08-16 11:01 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Heiko Carstens, Joe Perches, Dave Jones, Linux Kernel, rolandd,
Chas Williams, Paul Mundt, isdn4linux, mikep, Netdev, swen,
linux390, linux-s390, jdike, user-mode-linux-devel,
user-mode-linux-user, netfilter-devel, coreteam, Satyam Sharma
In-Reply-To: <Pine.LNX.4.64.0708161248050.22579@kivilampi-30.cs.helsinki.fi>
On Thu, Aug 16, 2007 at 01:22:04PM +0300, Ilpo Järvinen wrote:
>
> ...I guess those guys hunting for broken busyloops in the other thread
> could also benefit from similar searching commands introduced in this
> thread... ...Ccing Satyam to caught their attention too.
>
>
> ./drivers/isdn/hisax/hfc_pci.c
> 125: if (Read_hfc(cs, HFCPCI_INT_S1)) ;
> 155: if (Read_hfc(cs, HFCPCI_INT_S1)) ;
> 1483: if (Read_hfc(cs, HFCPCI_INT_S1)) ;
> --
> ./drivers/isdn/hisax/hfc_sx.c
> 377: if (Read_hfc(cs, HFCSX_INT_S1)) ;
> 407: if (Read_hfc(cs, HFCSX_INT_S2)) ;
> 1246: if (Read_hfc(cs, HFCSX_INT_S1)) ;
> --
These are workaround to not get compiler warnings about ignored return
values I got some time ago under some architecture.
--
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)
^ permalink raw reply
* Re: [patch 2/4] Update e100 driver to use devres.
From: Tejun Heo @ 2007-08-16 11:34 UTC (permalink / raw)
To: Brandon Philips; +Cc: netdev, e1000-devel
In-Reply-To: <20070815185949.GC7294@ifup.org>
Brandon Philips wrote:
> @@ -2554,8 +2547,10 @@ static int __devinit e100_probe(struct p
> struct net_device *netdev;
> struct nic *nic;
> int err;
> + void __iomem **iomap;
> + int bar;
>
> - if(!(netdev = alloc_etherdev(sizeof(struct nic)))) {
> + if (!(netdev = devm_alloc_etherdev(&pdev->dev, sizeof(struct nic)))) {
> if(((1 << debug) - 1) & NETIF_MSG_PROBE)
> printk(KERN_ERR PFX "Etherdev alloc failed, abort.\n");
> return -ENOMEM;
> @@ -2585,26 +2580,28 @@ static int __devinit e100_probe(struct p
> nic->msg_enable = (1 << debug) - 1;
> pci_set_drvdata(pdev, netdev);
>
> - if((err = pci_enable_device(pdev))) {
> + if ((err = pcim_enable_device(pdev))) {
> DPRINTK(PROBE, ERR, "Cannot enable PCI device, aborting.\n");
> - goto err_out_free_dev;
> + return err;
> }
>
> if(!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
> DPRINTK(PROBE, ERR, "Cannot find proper PCI device "
> "base address, aborting.\n");
> err = -ENODEV;
> - goto err_out_disable_pdev;
> + return err;
> }
>
> - if((err = pci_request_regions(pdev, DRV_NAME))) {
> + bar = use_io ? 1 : 0;
> + if((err = pcim_iomap_regions(pdev, 1 << bar, DRV_NAME))) {
> DPRINTK(PROBE, ERR, "Cannot obtain PCI resources, aborting.\n");
> - goto err_out_disable_pdev;
> + return err;
> }
> + iomap = pcim_iomap_table(pdev)[bar];
Type mismatch. Didn't compiler warn about it?
>
> if((err = pci_set_dma_mask(pdev, DMA_32BIT_MASK))) {
> DPRINTK(PROBE, ERR, "No usable DMA configuration, aborting.\n");
> - goto err_out_free_res;
> + return err;
> }
>
> SET_MODULE_OWNER(netdev);
> @@ -2613,12 +2610,6 @@ static int __devinit e100_probe(struct p
> if (use_io)
> DPRINTK(PROBE, INFO, "using i/o access mode\n");
>
> - nic->csr = pci_iomap(pdev, (use_io ? 1 : 0), sizeof(struct csr));
> - if(!nic->csr) {
> - DPRINTK(PROBE, ERR, "Cannot map device registers, aborting.\n");
> - err = -ENOMEM;
> - goto err_out_free_res;
> - }
nic->csr initialization seems missing. Have you tested the patched code?
--
tejun
^ permalink raw reply
* Re: [patch 4/4] Update e1000 driver to use devres.
From: Tejun Heo @ 2007-08-16 11:44 UTC (permalink / raw)
To: Brandon Philips; +Cc: e1000-devel, netdev
In-Reply-To: <20070815190014.GE7294@ifup.org>
Brandon Philips wrote:
> - mmio_start = pci_resource_start(pdev, BAR_0);
> mmio_len = pci_resource_len(pdev, BAR_0);
You don't need mmio_len either.
> - err = -EIO;
> - adapter->hw.hw_addr = ioremap(mmio_start, mmio_len);
> + adapter->hw.hw_addr = pcim_iomap(pdev, BAR_0, mmio_len);
Passing 0 as @max_len tells pci[m]_iomap() to use pci_resource_len() of
the BAR.
> @@ -952,16 +948,15 @@ e1000_probe(struct pci_dev *pdev,
> /* setup the private structure */
>
> if ((err = e1000_sw_init(adapter)))
> - goto err_sw_init;
> + return err;
>
> err = -EIO;
> /* Flash BAR mapping must happen after e1000_sw_init
> * because it depends on mac_type */
> if ((adapter->hw.mac_type == e1000_ich8lan) &&
> (pci_resource_flags(pdev, 1) & IORESOURCE_MEM)) {
> - flash_start = pci_resource_start(pdev, 1);
> flash_len = pci_resource_len(pdev, 1);
Ditto.
> - adapter->hw.flash_address = ioremap(flash_start, flash_len);
> + adapter->hw.flash_address = pcim_iomap(pdev, 1, flash_len);
> if (!adapter->hw.flash_address)
> goto err_flashmap;
> }
--
tejun
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
^ permalink raw reply
* [RFC] net/core/dst.c : Should'nt dst_run_gc() be more scalable and friendly ?
From: Eric Dumazet @ 2007-08-16 12:09 UTC (permalink / raw)
To: netdev@vger.kernel.org
Hi all
When the periodic route cache flush is done, this machine suffers a lot and eventually triggers the "soft lockup" message.
dst_run_gc() is doing a scan of a possibly huge list of dst_entries, eventually freeing some (less than 1%) of them, but holding the dst_lock spinlock for the whole scan.
Then it rearms a timer to redo the full thing 1/10 s later. The slowdown can last one minute or so.
No need to say that machine is not really usable in the meantime.
I already looked at the code and I am testing a patch (included in this mail) where I limit the number of entries that can be scanned at each dst_run_gc() call, and not holding dst_lock during the scan. But then I dont know how to address the dst_dev_event() problem, since we must be able to dst_ifdown() all entries attached to one dev.
I am wondering how to really solve the problem. Could a workqueue be used here instead of a timer ?
Aug 16 06:21:37 SRV1 kernel: BUG: soft lockup detected on CPU#0!
Aug 16 06:21:37 SRV1 kernel:
Aug 16 06:21:37 SRV1 kernel: Call Trace:
Aug 16 06:21:37 SRV1 kernel: <IRQ> [<ffffffff802286f0>] wake_up_process+0x10/0x20
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80251e09>] softlockup_tick+0xe9/0x110
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803cd380>] dst_run_gc+0x0/0x140
Aug 16 06:21:37 SRV1 kernel: [<ffffffff802376f3>] run_local_timers+0x13/0x20
Aug 16 06:21:37 SRV1 kernel: [<ffffffff802379c7>] update_process_times+0x57/0x90
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80216034>] smp_local_timer_interrupt+0x34/0x60
Aug 16 06:21:37 SRV1 kernel: [<ffffffff802165cc>] smp_apic_timer_interrupt+0x5c/0x80
Aug 16 06:21:37 SRV1 kernel: [<ffffffff8020a816>] apic_timer_interrupt+0x66/0x70
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803cd3d3>] dst_run_gc+0x53/0x140
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803cd3c6>] dst_run_gc+0x46/0x140
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80237148>] run_timer_softirq+0x148/0x1c0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff8023340c>] __do_softirq+0x6c/0xe0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff8020ad6c>] call_softirq+0x1c/0x30
Aug 16 06:21:37 SRV1 kernel: <EOI> [<ffffffff8020cb34>] do_softirq+0x34/0x90
Aug 16 06:21:37 SRV1 kernel: [<ffffffff802331cf>] local_bh_enable_ip+0x3f/0x60
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80422913>] _spin_unlock_bh+0x13/0x20
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803dfde8>] rt_garbage_collect+0x1d8/0x320
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803cd4dd>] dst_alloc+0x1d/0xa0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803e1433>] __ip_route_output_key+0x573/0x800
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803c02e2>] sock_common_recvmsg+0x32/0x50
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803e16dc>] ip_route_output_flow+0x1c/0x60
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80400160>] tcp_v4_connect+0x150/0x610
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803ebf07>] inet_bind_bucket_create+0x17/0x60
Aug 16 06:21:37 SRV1 kernel: [<ffffffff8040cd16>] inet_stream_connect+0xa6/0x2c0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80422981>] _spin_lock_bh+0x11/0x30
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803c0bdf>] lock_sock_nested+0xcf/0xe0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80422981>] _spin_lock_bh+0x11/0x30
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803be551>] sys_connect+0x71/0xa0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803eee3f>] tcp_setsockopt+0x1f/0x30
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803c030f>] sock_common_setsockopt+0xf/0x20
Aug 16 06:21:37 SRV1 kernel: [<ffffffff803be4bd>] sys_setsockopt+0x9d/0xc0
Aug 16 06:21:37 SRV1 kernel: [<ffffffff8028881e>] sys_ioctl+0x5e/0x80
Aug 16 06:21:37 SRV1 kernel: [<ffffffff80209c4e>] system_call+0x7e/0x83
Prelimary patch :
--- linux-2.6.22/net/core/dst.c
+++ linux-2.6.22-ed/net/core/dst.c
@@ -28,6 +28,9 @@
* 4) All operations modify state, so a spinlock is used.
*/
static struct dst_entry *dst_garbage_list;
+static struct dst_entry *dst_garbage_wrk;
+static struct dst_entry *dst_garbage_inuse;
+static int dst_gc_running;
#if RT_CACHE_DEBUG >= 2
static atomic_t dst_total = ATOMIC_INIT(0);
#endif
@@ -42,26 +45,42 @@
static void dst_run_gc(unsigned long dummy)
{
- int delayed = 0;
- int work_performed;
- struct dst_entry * dst, **dstp;
-
- if (!spin_trylock(&dst_lock)) {
- mod_timer(&dst_gc_timer, jiffies + HZ/10);
- return;
- }
-
+ int quota = 1000;
+ int work_performed = 0;
+ struct dst_entry *dst, *next;
+ struct dst_entry *first = NULL, *last = NULL;
+#if RT_CACHE_DEBUG >= 2
+ unsigned long t0 = jiffies;
+#endif
+ spin_lock(&dst_lock);
+ if (dst_gc_running)
+ goto out;
del_timer(&dst_gc_timer);
- dstp = &dst_garbage_list;
- work_performed = 0;
- while ((dst = *dstp) != NULL) {
- if (atomic_read(&dst->__refcnt)) {
- dstp = &dst->next;
- delayed++;
+ if (!dst_garbage_wrk) {
+ dst_garbage_wrk = dst_garbage_list;
+ dst_garbage_list = dst_garbage_inuse;
+ dst_garbage_inuse = NULL;
+ }
+ /*
+ * before releasing dst_lock, tell others we are currently running
+ * so they wont start timer or mess dst_garbage_wrk
+ */
+ dst_gc_running = 1;
+ next = dst_garbage_wrk;
+ spin_unlock(&dst_lock);
+
+ while ((dst = next) != NULL) {
+ next = dst->next;
+ if (likely(atomic_read(&dst->__refcnt))) {
+ if (unlikely(last == NULL))
+ last = dst;
+ dst->next = first;
+ first = dst;
+ if (--quota == 0)
+ break;
continue;
}
- *dstp = dst->next;
- work_performed = 1;
+ work_performed++;
dst = dst_destroy(dst);
if (dst) {
@@ -77,16 +96,26 @@
continue;
___dst_free(dst);
- dst->next = *dstp;
- *dstp = dst;
- dstp = &dst->next;
+ dst->next = next;
+ next = dst;
}
}
- if (!dst_garbage_list) {
+ dst_garbage_wrk = next;
+
+ spin_lock(&dst_lock);
+ dst_gc_running = 0;
+ if (last != NULL) {
+ last->next = dst_garbage_inuse;
+ dst_garbage_inuse = first;
+ }
+ /*
+ * If all lists are empty, no need to rearm a timer
+ */
+ if (!dst_garbage_list && !dst_garbage_wrk && !dst_garbage_inuse) {
dst_gc_timer_inc = DST_GC_MAX;
goto out;
}
- if (!work_performed) {
+ if (!work_performed && !dst_garbage_wrk) {
if ((dst_gc_timer_expires += dst_gc_timer_inc) > DST_GC_MAX)
dst_gc_timer_expires = DST_GC_MAX;
dst_gc_timer_inc += DST_GC_INC;
@@ -95,8 +124,12 @@
dst_gc_timer_expires = DST_GC_MIN;
}
#if RT_CACHE_DEBUG >= 2
- printk("dst_total: %d/%d %ld\n",
- atomic_read(&dst_total), delayed, dst_gc_timer_expires);
+ if (net_msg_warn)
+ printk(KERN_DEBUG "dst_run_gc: "
+ "dst_total=%d quota=%d perf=%d expires=%ld elapsed=%lu\n",
+ atomic_read(&dst_total),
+ quota, work_performed,
+ dst_gc_timer_expires, jiffies - t0);
#endif
/* if the next desired timer is more than 4 seconds in the future
* then round the timer to whole seconds
@@ -157,7 +190,7 @@
___dst_free(dst);
dst->next = dst_garbage_list;
dst_garbage_list = dst;
- if (dst_gc_timer_inc > DST_GC_INC) {
+ if (dst_gc_timer_inc > DST_GC_INC && !dst_gc_running) {
dst_gc_timer_inc = DST_GC_INC;
dst_gc_timer_expires = DST_GC_MIN;
mod_timer(&dst_gc_timer, jiffies + dst_gc_timer_expires);
^ permalink raw reply
* zd1211rw regression, device does not enumerate
From: Oliver Neukum @ 2007-08-16 12:25 UTC (permalink / raw)
To: netdev, linux-usb-devel, Daniel Drake, John W.Linville
Hi,
after bisection it boils down to this patch:
oliver@oenone:/home/olli2/Trees/linux-2.6> git bisect bad
74553aedd46b3a2cae986f909cf2a3f99369decc is first bad commit
commit 74553aedd46b3a2cae986f909cf2a3f99369decc
Author: Daniel Drake <dsd@gentoo.org>
Date: Sun Jul 1 18:22:32 2007 +0100
[PATCH] zd1211rw: Defer firmware load until first ifup
that plugging in my device I get an error:
Aug 16 13:17:11 oenone kernel: zd1211rw 3-4.3:1.0: zd1211b chip 0ace:1215 v4810 high 00-02-72 AL2230_RF pa0 g--NS
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh1-0001/ffff810037c04960 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: ehci_hcd 0000:00:02.2: reused qh ffff810037c04960 schedule
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh1-0001/ffff810037c04960 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: link qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:11 oenone kernel: usb 3-4.3: unlink qh0-00ff/ffff810037c04a00 start 0 [2/0 us]
Aug 16 13:17:12 oenone ifup: Cannot enable interface eth2.
Aug 16 13:17:12 oenone kernel: zd1211rw 3-4.3:1.0: error ioread32(CR_REG1): -110
Aug 16 13:17:12 oenone kernel: usb 3-4.3: unlink qh1-0001/ffff810037c04960 start 0 [2/0 us]
Aug 16 13:17:12 oenone ifup-route: interface eth2 is not up
Bus 003 Device 006: ID 0ace:1215 ZyDAS
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 255 Vendor Specific Class
bDeviceSubClass 255 Vendor Specific Subclass
bDeviceProtocol 255 Vendor Specific Protocol
bMaxPacketSize0 64
idVendor 0x0ace ZyDAS
idProduct 0x1215
bcdDevice 48.10
iManufacturer 16 ZyDAS
iProduct 32 USB2.0 WLAN
iSerial 0
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 46
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x04 EP 4 OUT
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 1
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00
bDeviceClass 255 Vendor Specific Class
bDeviceSubClass 255 Vendor Specific Subclass
bDeviceProtocol 255 Vendor Specific Protocol
bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)
Regards
Oliver
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply
* Re: drivers/infiniband/mlx/mad.c misplaced ;
From: Satyam Sharma @ 2007-08-16 12:41 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Heiko Carstens, Joe Perches, Dave Jones, Linux Kernel, rolandd,
Chas Williams, Paul Mundt, isdn4linux, mikep, Netdev, swen,
linux390, linux-s390, jdike, user-mode-linux-devel,
user-mode-linux-user, netfilter-devel, coreteam
In-Reply-To: <Pine.LNX.4.64.0708161248050.22579@kivilampi-30.cs.helsinki.fi>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 914 bytes --]
Hi Ilpo,
On Thu, 16 Aug 2007, Ilpo Järvinen wrote:
>
> ...I guess those guys hunting for broken busyloops in the other thread
> could also benefit from similar searching commands introduced in this
> thread... ...Ccing Satyam to caught their attention too.
>
>
> > On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> > >
> > > There's more than a few of these (not inspected).
> > >
> > > $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" *
>
> ...Hmm, I plugged in "a preprocessor" too to manage with non compliant
> coding styles :-). Please understand that the line numbers are not an
> exact match due to preprocessor changes:
>
> $ for i in `find . -name '*.[ch]'`; do echo $i; indent -npro -kr -i8 -ts8
> -sob -l8000 -ss -ncs -cp1 -nhnl -st $i | egrep -n "[[:space:]]if [(].*[)] ;$";
> done | grep -B1 "^[^.]"
Thanks, looks useful, will check with this.
Satyam
^ permalink raw reply
* Re: [RFC] net/core/dst.c : Should'nt dst_run_gc() be more scalable and friendly ?
From: Herbert Xu @ 2007-08-16 12:41 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <20070816140914.de24335c.dada1@cosmosbay.com>
Eric Dumazet <dada1@cosmosbay.com> wrote:
>
> I am wondering how to really solve the problem. Could a workqueue be used here instead of a timer ?
I think so.
A mutex would separate the GC and dst_ifdown. You'd take the
spin lock in the GC only to replace the garbage list with NULL.
You then drop the lock to process it. Once it's done you take
the lock again and join it with whatever that's been added in
the mean time. This is easy because you should already have
the tail after the GC process.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: 2.6.23-rc3 and SKY2 driver issue
From: Stephen Hemminger @ 2007-08-16 13:05 UTC (permalink / raw)
To: Michal Piotrowski; +Cc: James Corey, linux-kernel, Netdev
In-Reply-To: <6bffcb0e0708160125v56f3d0f3l865574bdd56bd6ad@mail.gmail.com>
On Thu, 16 Aug 2007 10:25:45 +0200
"Michal Piotrowski" <michal.k.k.piotrowski@gmail.com> wrote:
> [Adding Stephen and netdev to CC]
>
> On 15/08/07, James Corey <ploversegg@yahoo.com> wrote:
> >
> > I tried running a D-link gig card on kernel 2.6.21.1
> > and it came up fine, but when I did a sftp of
> > an linux dvd ISO to it, the interface would lock
> > up hard with the error
> >
> > eth1: hw csum failure.
> >
> > Call Trace:
> > <IRQ> [<ffffffff804d3e31>]
> > __skb_checksum_complete_head+0x46/0x5f
> > [<ffffffff804d3e56>] __skb_checksum_complete+0xc/0x11
> > [<ffffffff805046c7>] tcp_v4_rcv+0x157/0x810
> > [<ffffffff804d8a6f>] dev_queue_xmit+0x237/0x260
> > [<ffffffff80229990>] find_busiest_group+0x252/0x684
> > [<ffffffff804ead50>] ip_local_deliver+0xca/0x14c
> > [<ffffffff804eb24a>] ip_rcv+0x478/0x4ba
> > [<ffffffff803ec7d3>] sky2_poll+0x6f9/0x9b9
> > [<ffffffff8022bd5d>]
> > run_rebalance_domains+0x13e/0x408
> > [<ffffffff804d877a>] net_rx_action+0xa8/0x166
> > [<ffffffff80235d62>] __do_softirq+0x55/0xc3
> > [<ffffffff8020a4ec>] call_softirq+0x1c/0x28
> > [<ffffffff8020b611>] do_softirq+0x2c/0x7d
> > [<ffffffff8020b8cf>] do_IRQ+0x13e/0x15f
> > [<ffffffff802086ce>] mwait_idle+0x0/0x46
> > [<ffffffff80209871>] ret_from_intr+0x0/0xa
> > <EOI> [<ffffffff80208710>] mwait_idle+0x42/0x46
> > [<ffffffff80208666>] cpu_idle+0x8c/0xaf
> > [<ffffffff8078174e>] start_kernel+0x2ac/0x2b8
> > [<ffffffff80781140>] _sinittext+0x140/0x144
> >
> >
> > So I tried running the latest snapshot 2.6.23-rc3
> > and the almost the same thing happens. Only
> > difference is that now the entire box locks up.
> > The error is almost the same
> >
> > eth1: hw csum failure.
> >
> > Call Trace:
> > <IRQ> [<ffffffff804779b6>]
> > __skb_checksum_complete_head+0x43/0x56
> > [<ffffffff804779d5>] __skb_checksum_complete+0xc/0x11
> > [<ffffffff804a989d>] tcp_v4_rcv+0x14e/0x801
> > [<ffffffff8048ff84>] ip_local_deliver+0xca/0x14c
> > [<ffffffff80490472>] ip_rcv+0x46c/0x4ae
> > [<ffffffff880060f9>] :sky2:sky2_poll+0x72b/0x9c7
> > [<ffffffff8047c934>] net_rx_action+0xa8/0x166
> > [<ffffffff80235ced>] __do_softirq+0x55/0xc4
> > [<ffffffff8020c5cc>] call_softirq+0x1c/0x28
> > [<ffffffff8020d6fd>] do_softirq+0x2c/0x7d
> > [<ffffffff8020d9bb>] do_IRQ+0x13e/0x15f
> > [<ffffffff8020a780>] mwait_idle+0x0/0x48
> > [<ffffffff8020b951>] ret_from_intr+0x0/0xa
> > <EOI> [<ffffffff880063e7>]
> > :sky2:sky2_xmit_frame+0x0/0x41d
> > [<ffffffff8020a7c2>] mwait_idle+0x42/0x48
> > [<ffffffff8020a718>] cpu_idle+0xbd/0xe0
> > [<ffffffff80704a5a>] start_kernel+0x2ac/0x2b8
> > [<ffffffff80704140>] _sinittext+0x140/0x144
> >
> >
> > I see that the new kernel includes some sort of
> > SKY2 DEBUG stuff. I would be happy to rerun
> > the test with that turned on, given some minor
> > direction.
> >
>
> Regards,
> Michal
Please reproduce with a more recent kernel?
^ permalink raw reply
* [PATCH 0/4] backport of sky2 stability fixes
From: Stephen Hemminger @ 2007-08-16 13:12 UTC (permalink / raw)
To: stable; +Cc: netdev
These are bugfixes that happened late in 2.6.22 cycle that missed
getting merged.
--
^ permalink raw reply
* [PATCH 1/4] sky2: restore workarounds for lost interrupts
From: Stephen Hemminger @ 2007-08-16 13:12 UTC (permalink / raw)
To: stable; +Cc: netdev
In-Reply-To: <20070816131244.726293914@linux-foundation.org>
[-- Attachment #1: sky2-lost-irq.patch --]
[-- Type: text/plain, Size: 1351 bytes --]
Backport of commit c59697e06058fc2361da8cefcfa3de85ac107582
This patch restores a couple of workarounds from 2.6.16:
* restart transmit moderation timer in case it expires during IRQ routine
* default to having 10 HZ watchdog timer.
At this point it more important not to hang than to worry about the
power cost.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/drivers/net/sky2.c 2007-08-08 21:50:10.000000000 +0100
+++ b/drivers/net/sky2.c 2007-08-08 22:01:00.000000000 +0100
@@ -96,7 +96,7 @@ static int disable_msi = 0;
module_param(disable_msi, int, 0);
MODULE_PARM_DESC(disable_msi, "Disable Message Signaled Interrupt (MSI)");
-static int idle_timeout = 0;
+static int idle_timeout = 100;
module_param(idle_timeout, int, 0);
MODULE_PARM_DESC(idle_timeout, "Watchdog timer for lost interrupts (ms)");
@@ -2442,6 +2442,13 @@ static int sky2_poll(struct net_device *
work_done = sky2_status_intr(hw, work_limit);
if (work_done < work_limit) {
+ /* Bug/Errata workaround?
+ * Need to kick the TX irq moderation timer.
+ */
+ if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
+ sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
+ sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
+ }
netif_rx_complete(dev0);
/* end of interrupt, re-enables also acts as I/O synchronization */
--
^ permalink raw reply
* [PATCH 3/4] sky2: check for more work before leaving NAPI
From: Stephen Hemminger @ 2007-08-16 13:12 UTC (permalink / raw)
To: stable; +Cc: netdev
In-Reply-To: <20070816131244.726293914@linux-foundation.org>
[-- Attachment #1: sky2-napi-fill.patch --]
[-- Type: text/plain, Size: 2050 bytes --]
Backport of commit 5c11ce700f77fada15b6264417d72462da4bbb1c
This patch avoids generating another IRQ if more packets
arrive while in the NAPI poll routine. Before marking device as
finished, it rechecks that the status ring is empty.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/drivers/net/sky2.c 2007-08-08 22:01:11.000000000 +0100
+++ b/drivers/net/sky2.c 2007-08-08 22:01:28.000000000 +0100
@@ -2428,8 +2428,7 @@ static void sky2_err_intr(struct sky2_hw
static int sky2_poll(struct net_device *dev0, int *budget)
{
struct sky2_hw *hw = ((struct sky2_port *) netdev_priv(dev0))->hw;
- int work_limit = min(dev0->quota, *budget);
- int work_done = 0;
+ int work_done;
u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
if (unlikely(status & Y2_IS_ERROR))
@@ -2441,25 +2440,25 @@ static int sky2_poll(struct net_device *
if (status & Y2_IS_IRQ_PHY2)
sky2_phy_intr(hw, 1);
- work_done = sky2_status_intr(hw, work_limit);
- if (work_done < work_limit) {
- /* Bug/Errata workaround?
- * Need to kick the TX irq moderation timer.
- */
- if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
- sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
- sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
- }
- netif_rx_complete(dev0);
+ work_done = sky2_status_intr(hw, min(dev0->quota, *budget));
+ *budget -= work_done;
+ dev0->quota -= work_done;
- /* end of interrupt, re-enables also acts as I/O synchronization */
- sky2_read32(hw, B0_Y2_SP_LISR);
- return 0;
- } else {
- *budget -= work_done;
- dev0->quota -= work_done;
+ /* More work? */
+ if (hw->st_idx != sky2_read16(hw, STAT_PUT_IDX))
return 1;
+
+ /* Bug/Errata workaround?
+ * Need to kick the TX irq moderation timer.
+ */
+ if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
+ sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
+ sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
}
+ netif_rx_complete(dev0);
+
+ sky2_read32(hw, B0_Y2_SP_LISR);
+ return 0;
}
static irqreturn_t sky2_intr(int irq, void *dev_id)
--
^ permalink raw reply
* [PATCH 4/4] sky2: check drop truncated packets
From: Stephen Hemminger @ 2007-08-16 13:12 UTC (permalink / raw)
To: stable; +Cc: netdev
In-Reply-To: <20070816131244.726293914@linux-foundation.org>
[-- Attachment #1: sky2-stable-trunc.patch --]
[-- Type: text/plain, Size: 909 bytes --]
Backport of commit 71749531f2d1954137a1a77422ef4ff29eb102dd
If packet larger than MTU is received, the driver uses hardware to
truncate the packet. Use the status registers to catch/drop them.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/drivers/net/sky2.c 2007-08-08 22:01:28.000000000 +0100
+++ b/drivers/net/sky2.c 2007-08-08 22:01:37.000000000 +0100
@@ -2065,6 +2065,9 @@ static struct sk_buff *sky2_receive(stru
if (!(status & GMR_FS_RX_OK))
goto resubmit;
+ if (status >> 16 != length)
+ goto len_mismatch;
+
if (length < copybreak)
skb = receive_copy(sky2, re, length);
else
@@ -2074,6 +2077,11 @@ resubmit:
return skb;
+len_mismatch:
+ /* Truncation of overlength packets
+ causes PHY length to not match MAC length */
+ ++sky2->net_stats.rx_length_errors;
+
error:
++sky2->net_stats.rx_errors;
if (status & GMR_FS_RX_FF_OV) {
--
^ permalink raw reply
* [PATCH 2/4] sky2: carrier management
From: Stephen Hemminger @ 2007-08-16 13:12 UTC (permalink / raw)
To: stable; +Cc: netdev
In-Reply-To: <20070816131244.726293914@linux-foundation.org>
[-- Attachment #1: sky2-carrier-mgmt.patch --]
[-- Type: text/plain, Size: 1917 bytes --]
backport of commit 55d7b4e6ed6ad3ec5e5e30b3b4515a0a6a53e344
Make sky2 handle carrier similar to other drivers,
eliminate some possible races in carrier state transistions.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/drivers/net/sky2.c 2007-08-08 22:01:00.000000000 +0100
+++ b/drivers/net/sky2.c 2007-08-08 22:01:11.000000000 +0100
@@ -1234,6 +1234,8 @@ static int sky2_up(struct net_device *de
if (netif_msg_ifup(sky2))
printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
+ netif_carrier_off(dev);
+
/* must be power of 2 */
sky2->tx_le = pci_alloc_consistent(hw->pdev,
TX_RING_SIZE *
@@ -1573,7 +1575,6 @@ static int sky2_down(struct net_device *
/* Stop more packets from being queued */
netif_stop_queue(dev);
- netif_carrier_off(dev);
/* Disable port IRQ */
imask = sky2_read32(hw, B0_IMSK);
@@ -1625,6 +1626,8 @@ static int sky2_down(struct net_device *
sky2_phy_power(hw, port, 0);
+ netif_carrier_off(dev);
+
/* turn off LED's */
sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
@@ -1689,7 +1692,6 @@ static void sky2_link_up(struct sky2_por
gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);
netif_carrier_on(sky2->netdev);
- netif_wake_queue(sky2->netdev);
/* Turn on link LED */
sky2_write8(hw, SK_REG(port, LNK_LED_REG),
@@ -1741,7 +1743,6 @@ static void sky2_link_down(struct sky2_p
gma_write16(hw, port, GM_GP_CTRL, reg);
netif_carrier_off(sky2->netdev);
- netif_stop_queue(sky2->netdev);
/* Turn on link LED */
sky2_write8(hw, SK_REG(port, LNK_LED_REG), LINKLED_OFF);
@@ -3493,10 +3494,6 @@ static __devinit struct net_device *sky2
memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8, ETH_ALEN);
memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
- /* device is off until link detection */
- netif_carrier_off(dev);
- netif_stop_queue(dev);
-
return dev;
}
--
^ permalink raw reply
* Re: skge and sky2 stop working after a few minutes.
From: Stephen Hemminger @ 2007-08-16 13:17 UTC (permalink / raw)
To: Sébastien Judenherc; +Cc: netdev
In-Reply-To: <loom.20070815T224114-635@post.gmane.org>
On Wed, 15 Aug 2007 21:20:37 +0000 (UTC)
Sébastien Judenherc <sebastien.judenherc@agecodagis.com> wrote:
> Hello,
> I can see some very strange problem with sky2 and skge interfaces with 2.6.22,
> and also 2.6.23-rc2/3.
> After 8-9 minutes, the interfaces stop working. ethtool reports that the link is
> down and the only way to make the interfaces usable again is
> removing/reinserting the module or running ethtool -r.
> This occurs for both sky2 and skge for two PCs connected to a gigabit switch.
> The other PCs on the same switch continue to work fine (most are 100Mbit/s). It
> also occurs on a third PC with skge, directly connected to a 100Mbit/s interface
> over a cross over cable. I didn't notice the problem with 2.6.19.
>
Looks like some noise sensitivity in the PHY. Could you try the vendor sk98lin
driver, although all drivers use the same phy settings.
^ permalink raw reply
* [PATCH 3/4] net: add dev_get_stats
From: Stephen Hemminger @ 2007-08-16 13:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070816132523.201766718@linux-foundation.org>
[-- Attachment #1: dev_get_stats.patch --]
[-- Type: text/plain, Size: 3819 bytes --]
Since we now have internal stats, it cleans up code to have a
dev_get_stats() interface. This allows for future patches where
'network device ops' patch where get_stats is immutable.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/include/linux/netdevice.h 2007-08-16 06:34:25.000000000 -0400
+++ b/include/linux/netdevice.h 2007-08-16 06:38:33.000000000 -0400
@@ -809,6 +809,7 @@ extern int dev_set_mac_address(struct n
struct sockaddr *);
extern int dev_hard_start_xmit(struct sk_buff *skb,
struct net_device *dev);
+extern struct net_device_stats *dev_get_stats(struct net_device *dev);
extern int netdev_budget;
--- a/net/core/dev.c 2007-08-16 06:34:25.000000000 -0400
+++ b/net/core/dev.c 2007-08-16 08:26:04.000000000 -0400
@@ -2304,7 +2304,7 @@ void dev_seq_stop(struct seq_file *seq,
static void dev_seq_printf_stats(struct seq_file *seq, struct net_device *dev)
{
- struct net_device_stats *stats = dev->get_stats(dev);
+ struct net_device_stats *stats = dev_get_stats(dev);
seq_printf(seq, "%6s:%8lu %7lu %4lu %4lu %4lu %5lu %10lu %9lu "
"%8lu %7lu %4lu %4lu %4lu %5lu %7lu %10lu\n",
@@ -3682,10 +3682,21 @@ out:
mutex_unlock(&net_todo_run_mutex);
}
-static struct net_device_stats *internal_stats(struct net_device *dev)
+/**
+ * dev_get_stats - get network device statistics
+ * @dev: network device
+ *
+ * Get standard network device statistics.
+ * Use internal stastics unless device overides
+ */
+struct net_device_stats *dev_get_stats(struct net_device *dev)
{
- return &dev->stats;
+ if (!dev->get_stats)
+ return &dev->stats;
+
+ return dev->get_stats(dev);
}
+EXPORT_SYMBOL(dev_get_stats);
/**
* alloc_netdev_mq - allocate network device
@@ -3733,7 +3744,6 @@ struct net_device *alloc_netdev_mq(int s
dev->egress_subqueue_count = queue_count;
- dev->get_stats = internal_stats;
setup(dev);
strcpy(dev->name, name);
return dev;
--- a/net/core/net-sysfs.c 2007-08-16 06:34:25.000000000 -0400
+++ b/net/core/net-sysfs.c 2007-08-16 09:23:13.000000000 -0400
@@ -264,8 +264,7 @@ static ssize_t netstat_show(const struct
WARN_ON(1);
read_lock(&dev_base_lock);
- if (dev_isalive(dev) && dev->get_stats &&
- (stats = (*dev->get_stats)(dev)))
+ if (dev_isalive(dev) && (stats = dev_get_stats(dev)))
ret = sprintf(buf, fmt_ulong,
*(unsigned long *)(((u8 *) stats) + offset));
@@ -481,8 +480,7 @@ int netdev_register_sysfs(struct net_dev
BUILD_BUG_ON(BUS_ID_SIZE < IFNAMSIZ);
strlcpy(dev->bus_id, net->name, BUS_ID_SIZE);
- if (net->get_stats)
- *groups++ = &netstat_group;
+ *groups++ = &netstat_group;
#ifdef CONFIG_WIRELESS_EXT
if (net->wireless_handlers && net->wireless_handlers->get_wireless_stats)
--- a/net/core/rtnetlink.c 2007-08-16 06:34:25.000000000 -0400
+++ b/net/core/rtnetlink.c 2007-08-16 08:26:13.000000000 -0400
@@ -619,6 +619,7 @@ static int rtnl_fill_ifinfo(struct sk_bu
{
struct ifinfomsg *ifm;
struct nlmsghdr *nlh;
+ struct net_device_stats *stats;
nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
if (nlh == NULL)
@@ -666,18 +667,13 @@ static int rtnl_fill_ifinfo(struct sk_bu
NLA_PUT(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast);
}
- if (dev->get_stats) {
- struct net_device_stats *stats = dev->get_stats(dev);
- if (stats) {
- struct nlattr *attr;
-
- attr = nla_reserve(skb, IFLA_STATS,
- sizeof(struct rtnl_link_stats));
- if (attr == NULL)
- goto nla_put_failure;
+ if ((stats = dev_get_stats(dev))) {
+ struct nlattr *attr = nla_reserve(skb, IFLA_STATS,
+ sizeof(struct rtnl_link_stats));
+ if (attr == NULL)
+ goto nla_put_failure;
- copy_rtnl_link_stats(nla_data(attr), stats);
- }
+ copy_rtnl_link_stats(nla_data(attr), stats);
}
if (dev->rtnl_link_ops) {
--
^ permalink raw reply
* [PATCH 1/4] net: cleanup left over decl
From: Stephen Hemminger @ 2007-08-16 13:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070816132523.201766718@linux-foundation.org>
[-- Attachment #1: dev_init.patch --]
[-- Type: text/plain, Size: 497 bytes --]
Remove unneeded declaration.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/include/linux/netdevice.h 2007-08-15 14:29:02.000000000 +0100
+++ b/include/linux/netdevice.h 2007-08-15 14:35:25.000000000 +0100
@@ -833,8 +833,6 @@ extern int dev_set_mac_address(struct n
extern int dev_hard_start_xmit(struct sk_buff *skb,
struct net_device *dev);
-extern void dev_init(void);
-
extern int netdev_budget;
/* Called by rtnetlink.c:rtnl_unlock() */
--
^ permalink raw reply
* [PATCH 0/4] Small network device interface changes
From: Stephen Hemminger @ 2007-08-16 13:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev
These cleanups should go in 2.6.24. Hope they don't conflict too bad
with the NAPI stuff.
--
^ permalink raw reply
* [PATCH 2/4] net: deinline dev_kfree_skb_irq
From: Stephen Hemminger @ 2007-08-16 13:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070816132523.201766718@linux-foundation.org>
[-- Attachment #1: dev_kfree_skb_irq.patch --]
[-- Type: text/plain, Size: 2821 bytes --]
Deinline dev_kfree_skb_irq. This saves about 100bytes per call
site on UP, probably more on SMP.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/include/linux/netdevice.h 2007-08-06 09:26:41.000000000 +0100
+++ b/include/linux/netdevice.h 2007-08-15 14:12:32.000000000 +0100
@@ -793,29 +793,6 @@ static inline int netif_is_multiqueue(co
#endif
}
-/* Use this variant when it is known for sure that it
- * is executing from interrupt context.
- */
-static inline void dev_kfree_skb_irq(struct sk_buff *skb)
-{
- if (atomic_dec_and_test(&skb->users)) {
- struct softnet_data *sd;
- unsigned long flags;
-
- local_irq_save(flags);
- sd = &__get_cpu_var(softnet_data);
- skb->next = sd->completion_queue;
- sd->completion_queue = skb;
- raise_softirq_irqoff(NET_TX_SOFTIRQ);
- local_irq_restore(flags);
- }
-}
-
-/* Use this variant in places where it could be invoked
- * either from interrupt or non-interrupt context.
- */
-extern void dev_kfree_skb_any(struct sk_buff *skb);
-
#define HAVE_NETIF_RX 1
extern int netif_rx(struct sk_buff *skb);
extern int netif_rx_ni(struct sk_buff *skb);
--- a/include/linux/skbuff.h 2007-08-06 09:26:43.000000000 +0100
+++ b/include/linux/skbuff.h 2007-08-15 14:09:04.000000000 +0100
@@ -378,6 +378,8 @@ extern int skb_cow_data(struct sk
struct sk_buff **trailer);
extern int skb_pad(struct sk_buff *skb, int pad);
#define dev_kfree_skb(a) kfree_skb(a)
+extern void dev_kfree_skb_any(struct sk_buff *skb);
+extern void dev_kfree_skb_irq(struct sk_buff *skb);
extern void skb_over_panic(struct sk_buff *skb, int len,
void *here);
extern void skb_under_panic(struct sk_buff *skb, int len,
--- a/net/core/dev.c 2007-08-06 09:26:48.000000000 +0100
+++ b/net/core/dev.c 2007-08-15 14:12:16.000000000 +0100
@@ -1249,6 +1249,36 @@ void __netif_rx_schedule(struct net_devi
}
EXPORT_SYMBOL(__netif_rx_schedule);
+/**
+ * dev_kfree_skb_irq - free skb from IRQ context
+ * @skb: skb to free
+ *
+ * Queue skb for later release. Used when skb needs to be
+ * free but executing in IRQ context.
+ */
+void dev_kfree_skb_irq(struct sk_buff *skb)
+{
+ if (atomic_dec_and_test(&skb->users)) {
+ struct softnet_data *sd;
+ unsigned long flags;
+
+ local_irq_save(flags);
+ sd = &__get_cpu_var(softnet_data);
+ skb->next = sd->completion_queue;
+ sd->completion_queue = skb;
+ raise_softirq_irqoff(NET_TX_SOFTIRQ);
+ local_irq_restore(flags);
+ }
+}
+EXPORT_SYMBOL(dev_kfree_skb_irq);
+
+/**
+ * dev_kfree_skb_any - free skb from any context
+ * @skb: skb to free
+ *
+ * Free skb from interrupt or non-interrupt context.
+ * Use this variant in places where context is not easily determined.
+ */
void dev_kfree_skb_any(struct sk_buff *skb)
{
if (in_irq() || irqs_disabled())
--
^ permalink raw reply
* [PATCH 4/4] net: netdev_budget rearrangement
From: Stephen Hemminger @ 2007-08-16 13:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070816132523.201766718@linux-foundation.org>
[-- Attachment #1: netdev_budget.patch --]
[-- Type: text/plain, Size: 867 bytes --]
Trivial patch, move netdev_budget declaration out of netdevice.h
to a new home with the other sysctl externs.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/include/linux/netdevice.h 2007-08-16 06:38:33.000000000 -0400
+++ b/include/linux/netdevice.h 2007-08-16 09:24:08.000000000 -0400
@@ -811,8 +811,6 @@ extern int dev_hard_start_xmit(struct s
struct net_device *dev);
extern struct net_device_stats *dev_get_stats(struct net_device *dev);
-extern int netdev_budget;
-
/* Called by rtnetlink.c:rtnl_unlock() */
extern void netdev_run_todo(void);
--- a/net/core/sysctl_net_core.c 2007-08-06 04:26:48.000000000 -0400
+++ b/net/core/sysctl_net_core.c 2007-08-16 09:24:08.000000000 -0400
@@ -13,6 +13,7 @@
#ifdef CONFIG_SYSCTL
+extern int netdev_budget;
extern int netdev_max_backlog;
extern int weight_p;
--
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox