Netdev List
 help / color / mirror / Atom feed
* Re: [BUG] New Kernel Bugs
From: Theodore Tso @ 2007-11-13 18:55 UTC (permalink / raw)
  To: Larry Finger
  Cc: Benoit Boissinot, Mark Lord, Ingo Molnar, Andrew Morton,
	David Miller, protasnb, linux-kernel, netdev, alsa-devel,
	linux-ide, linux-pcmcia, linux-input, bugme-daemon
In-Reply-To: <4739DFF8.3090702@lwfinger.net>

On Tue, Nov 13, 2007 at 11:33:44AM -0600, Larry Finger wrote:
> I'm very encouraged to read of your expanded testing efforts. As a
> bcm43xx developer, Ubuntu has been our problem distro, mostly
> because your standard kernels have debugging turned off for bcm43xx.
> When a Ubuntu user reports a problem and we ask for the relevant
> output from dmesg, they have no information. I ask two things of all
> distros: (1) Turn on debugging - we don't spam the logs that badly,
> and (2) forward any bugs found by your testing to the maintainer,
> and/or the bcm43xx mailing list.

Heh. I hadn't enabled CONFIG_BCM43XX_DEBUG myself, but I just changed
it for my next kernel build.  This is a slightly different issue,
which is that sometimes _DEBUG options shouldn't be turned on by
default (because they really trash performance and bloat log size),
and sometimes they are painless to turn on and don't cost much.

If that is the case, I'd suggest removing the option and just making
it compiled in by default with a run-time option to enable it.

   	       	  	       	 	  - Ted


   	      


^ permalink raw reply

* tg3: strange errors and non-working-ness
From: Jon Nelson @ 2007-11-13 18:57 UTC (permalink / raw)
  To: netdev

I'm not sure if this is the right place, but I've got a pair of GiG-E
cards that do not work correctly. Everything appears to come up just
fine, but sooner or later (typically fairly quickly) the cards weird
out and never really come back.

The best info I've got is this:

Nov 10 22:21:19 frank kernel: tg3.c:v3.65 (August 07, 2006)
Nov 10 22:21:19 frank kernel: ACPI: PCI Interrupt 0000:00:0b.0[A] ->
Link [LNKB] -> GSI 3 (level, low) -> IRQ 3
Nov 10 22:21:19 frank kernel: eth0: Tigon3 [partno(AC91002A1) rev 0105
PHY(5701)] (PCI:33MHz:32-bit) 10/100/1000BaseT Ethernet
00:09:5b:09:b1:69
Nov 10 22:21:19 frank kernel: eth0: RXcsums[1] LinkChgREG[0] MIirq[0]
ASF[0] Split[0] WireSpeed[1] TSOcap[0]
Nov 10 22:21:19 frank kernel: eth0: dma_rwctrl[76ff000f] dma_mask[64-bit]
Nov 10 22:21:19 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:21:19 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:21:19 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:21:19 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:21:20 frank kernel: tg3: eth0: Link is up at 1000 Mbps, full duplex.
Nov 10 22:21:20 frank kernel: tg3: eth0: Flow control is on for TX and
on for RX.
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:21:20 frank kernel: ACPI: PCI interrupt for device
0000:00:0b.0 disabled
Nov 10 22:21:20 frank kernel: PCI: Enabling device 0000:00:0b.0 (0100 -> 0102)
Nov 10 22:21:20 frank kernel: ACPI: PCI Interrupt 0000:00:0b.0[A] ->
Link [LNKB] -> GSI 3 (level, low) -> IRQ 3
Nov 10 22:21:20 frank kernel: eth0: Tigon3 [partno(AC91002A1) rev 0105
PHY(5701)] (PCI:33MHz:32-bit) 10/100/1000BaseT Ethernet
00:09:5b:09:b1:69
Nov 10 22:21:20 frank kernel: eth0: RXcsums[1] LinkChgREG[0] MIirq[0]
ASF[0] Split[0] WireSpeed[1] TSOcap[0]
Nov 10 22:21:20 frank kernel: eth0: dma_rwctrl[76ff000f] dma_mask[64-bit]
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:21:20 frank kernel: tg3: eth0: Link is up at 1000 Mbps, full duplex.
Nov 10 22:21:20 frank kernel: tg3: eth0: Flow control is on for TX and
on for RX.
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:21:20 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:21:20 frank kernel: tg3: eth0: Link is up at 1000 Mbps, full duplex.
Nov 10 22:21:20 frank kernel: tg3: eth0: Flow control is on for TX and
on for RX.
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:24:40 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:41:48 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:48 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:48 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:48 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:48 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:41:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:43:02 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:45:52 frank kernel: NETDEV WATCHDOG: eth0: transmit timed out
Nov 10 22:45:52 frank kernel: tg3: eth0: transmit timed out, resetting
Nov 10 22:45:52 frank kernel: tg3: tg3_stop_block timed out, ofs=1400
enable_bit=2
Nov 10 22:45:52 frank kernel: tg3: tg3_stop_block timed out, ofs=c00
enable_bit=2
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset b (was 164514e4, writing 302a1385)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 3 (was 0, writing 4008)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 2 (was 2000000, writing 2000015)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 1 (was 2b00000, writing 2b00106)
Nov 10 22:45:52 frank kernel: PM: Writing back config space on device
0000:00:0b.0 at offset 0 (was 164514e4, writing 3ea173b)
Nov 10 22:45:52 frank kernel: tg3: eth0: Link is down.
Nov 10 22:45:56 frank kernel: tg3: eth0: Link is up at 1000 Mbps, full duplex.
Nov 10 22:45:56 frank kernel: tg3: eth0: Flow control is on for TX and
on for RX.
Nov 10 22:47:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:47:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:47:49 frank kernel: nfs: server 192.168.2.1 not responding, timed out
Nov 10 22:49:02 frank kernel: nfs: server 192.168.2.1 not responding, timed out

Other gig-e cards work OK in this box.
I have a *pair* of the TG3 boards and they both do the same thing.

-- 
Jon

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Gabriel C @ 2007-11-13 18:57 UTC (permalink / raw)
  To: Adrian Bunk
  Cc: Theodore Tso, Benoit Boissinot, Mark Lord, Ingo Molnar,
	Andrew Morton, David Miller, protasnb, linux-kernel, netdev,
	alsa-devel, linux-ide, linux-pcmcia, linux-input, bugme-daemon
In-Reply-To: <20071113175610.GE4250@stusta.de>

Adrian Bunk wrote:
> On Tue, Nov 13, 2007 at 12:13:56PM -0500, Theodore Tso wrote:
>> On Tue, Nov 13, 2007 at 04:52:32PM +0100, Benoit Boissinot wrote:
>>> Btw, I used to test every -mm kernel. But since I've switched distros
>>> (gentoo->ubuntu)
>>> and I have less time, I feel it's harder to test -rc or -mm kernels (I
>>> know this isn't a lkml problem
>>> but more a distro problem, but I would love having an ubuntu blessed
>>> repo with current dev kernel
>>> for the latest stable ubuntu release).
>> There are two parts to this.  One is a Ubuntu development kernel which
>> we can give to large numbers of people to expand our testing pool.
>> But if we don't do a better job of responding to bug reports that
>> would be generated by expanded testing this won't necessarily help us.
>> ...
> 
> The main problem is finding experienced developers who spend time on 
> looking into bug reports.

There are already. IMO the problem is the development model.

There are tons new features in each new kernel release and 'tons new bugs'
which are not fixed during the release cycle nor in the .XX stable kernels.

Maybe after XX kernel releases there should be one just with bug-fixes _without_ any
new features , eg: cleaning bugs from bugzilla , know regressions , cleaning up code , 
removing broken drivers and the like.


> cu
> Adrian

Gabriel 

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Andrew Morton @ 2007-11-13 19:02 UTC (permalink / raw)
  To: David Miller
  Cc: protasnb, linux-kernel, netdev, alsa-devel, linux-ide,
	linux-pcmcia, linux-input, bugme-daemon
In-Reply-To: <20071113.043207.44732743.davem@davemloft.net>

On Tue, 13 Nov 2007 04:32:07 -0800 (PST) David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Tue, 13 Nov 2007 04:12:59 -0800
> 
> > On Tue, 13 Nov 2007 03:58:24 -0800 (PST) David Miller <davem@davemloft.net> wrote:
> > 
> > > From: Andrew Morton <akpm@linux-foundation.org>
> > > Date: Tue, 13 Nov 2007 03:49:16 -0800
> > > 
> > > > Do you believe that our response to bug reports is adequate?
> > > 
> > > Do you feel that making us feel and look like shit helps?
> > 
> > That doesn't answer my question.
> > 
> > See, first we need to work out whether we have a problem.  If we do this,
> > then we can then have a think about what to do about it.
> > 
> > I tried to convince the 2006 KS attendees that we have a problem and I
> > resoundingly failed.  People seemed to think that we're doing OK.
> > 
> > But it appears that data such as this contradicts that belief.
> > 
> > This is not a minor matter.  If the kernel _is_ slowly deteriorating then
> > this won't become readily apparent until it has been happening for a number
> > of years.  By that stage there will be so much work to do to get us back to
> > an acceptable level that it will take a huge effort.  And it will take a
> > long time after that for the kerel to get its reputation back.
> > 
> > So it is important that we catch deterioration *early* if it is happening.
> 
> You tell me what I should spend my time working on, and I promise to
> do it OK? :-)

My suggestion: regressions.

If we're really active in chasing down the regressions then I think we can
be confident that the kernel isn't deteriorating.  Probably it will be
improving as we also fix some always-been-there bugs.

I think that we're fairly good about working the regressions in
Adrian/Michal/Rafael's lists but once Linus releases 2.6.x we tend to let
the unsolved ones slide, and we don't pay as much attention to the
regressions which 2.6.x testers report.

> For example, if I have a choice between a TCP crash just about anyone
> can hit and some obscure issue only reported with some device nearly
> nobody has, which one should I analyze and work on?
> 
> That's the problem.  All of us prioritize and it means the chaff
> collects at the bottom.  You cannot fix that except by getting more
> bug fixers so that the chaff pile has a chance to get smaller.
> 
> Luckily if the report being ignored isn't chaff, it will show up again
> (and again and again) and this triggers a reprioritization because not
> only is the bug no longer chaff, it also now got a lot of information
> tagged to it so it's a double worthwhile investment to work on the
> problem.
> 
> I think a lot of bugs that "aren't getting looked at" are simply
> sitting in some early stage of this process.

Yes, that's a useful technique.  If multiple people are being hurt a lot by
a bug then that's a more important one to fix than the single-person
minor-irritant bug.

otoh that doesn't work very well with driver/platform bugs.  Often these
are regressions which only a single person can reproduce within the time
window which we have in which we can fix it.  If we don't fix it in that
window it'll go out to distros and presumably some more people will hit it.

So I don't see much alternative here to the traditional
work-with-the-originator way of resolving it.

git bisection should really help us with these regressions but it doesn't
appear that people are using as much as one would like.  I'm hoping that
the very good http://www.kernel.org/doc/local/git-quick.html will help us
out here.  Thanks to the mystery person who prepared that.

^ permalink raw reply

* Re: [PATCH 01/01] ipv6: RFC4214 Support (v2.1)
From: Vlad Yasevich @ 2007-11-13 19:03 UTC (permalink / raw)
  To: Templin, Fred L
  Cc: netdev, YOSHIFUJI Hideaki / 吉藤英明
In-Reply-To: <39C363776A4E8C4A94691D2BD9D1C9A1029EDC29@XCH-NW-7V2.nw.nos.boeing.com>

Hi Fred

Looks much better...  a few more comments...

Templin, Fred L wrote:
> @@ -2531,6 +2552,18 @@ static void addrconf_rs_timer(unsigned l
>  		 *	Announcement received after solicitation
>  		 *	was sent
>  		 */
> +
> +		/* ISATAP (RFC4214) - schedule next RS/RA */
> +		if (ifp->idev->dev->priv_flags & IFF_ISATAP) {
> +			struct ip_tunnel *t  = netdev_priv(ifp->idev->dev);
> +			if (t->parms.i_key != INADDR_NONE) {
> +				spin_lock(&ifp->lock);
> +				ifp->probes = 0;
> +				ifp->idev->if_flags &= ~(IF_RS_SENT|IF_RA_RCVD);
> +				addrconf_mod_timer(ifp, AC_RS, t->parms.o_key*HZ);
> +				spin_unlock(&ifp->lock);
> +			}
> +		}
>  		goto out;
>  	}
>  
> @@ -2545,10 +2578,28 @@ static void addrconf_rs_timer(unsigned l
>  				   ifp->idev->cnf.rtr_solicit_interval);
>  		spin_unlock(&ifp->lock);
>  
> -		ipv6_addr_all_routers(&all_routers);
> +		/* ISATAP (RFC4214) - unicast RS */
> +		if (ifp->idev->dev->priv_flags & IFF_ISATAP) {
> +			struct ip_tunnel *t = netdev_priv(ifp->idev->dev);
> +
> +			if (t->parms.i_key == INADDR_NONE) goto out;
> +
> +			ipv6_addr_set(&all_routers, htonl(0xFE800000), 0, 0, 0);
> +			addrconf_ifid_isatap(all_routers.s6_addr + 8, t->parms.i_key);

You have this piece of code here and once more in addrconf_dad_completed().  Move to
its own static function.

> +		} else
> +			ipv6_addr_all_routers(&all_routers);
>  
>  		ndisc_send_rs(ifp->idev->dev, &ifp->addr, &all_routers);
>  	} else {
> +		/* ISATAP (RFC4214) - try again later */
> +		if (ifp->idev->dev->priv_flags & IFF_ISATAP) {
> +			struct ip_tunnel *t = netdev_priv(ifp->idev->dev);
> +			if (t->parms.i_key != INADDR_NONE) {
> +				ifp->probes = 0;
> +				ifp->idev->if_flags &= ~(IF_RS_SENT|IF_RA_RCVD);
> +				addrconf_mod_timer(ifp, AC_RS, t->parms.o_key*HZ);
> +			}
> +		}
>  		spin_unlock(&ifp->lock);

Hm..  Just noticed.  You do this code block under lock, but the block above it is
out of the lock.   Which way should it be?  Is there any way for it to be another
piece of common code.  It shows up 3 times in the same function.

>  		/*
>  		 * Note: we do not support deprecated "all on-link"
> @@ -2594,6 +2645,7 @@ static void addrconf_dad_start(struct in
>  	spin_lock_bh(&ifp->lock);
>  
>  	if (dev->flags&(IFF_NOARP|IFF_LOOPBACK) ||
> +	    dev->priv_flags&IFF_ISATAP ||
>  	    !(ifp->flags&IFA_F_TENTATIVE) ||
>  	    ifp->flags & IFA_F_NODAD) {
>  		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC);
> @@ -2690,7 +2742,16 @@ static void addrconf_dad_completed(struc
>  	    (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL)) {
>  		struct in6_addr all_routers;
>  
> -		ipv6_addr_all_routers(&all_routers);
> +		/* ISATAP (RFC4214) - unicast RS */
> +		if (ifp->idev->dev->priv_flags & IFF_ISATAP) {
> +			struct ip_tunnel *t = netdev_priv(ifp->idev->dev);
> +
> +			if (t->parms.i_key == INADDR_NONE) return;
> +
> +			ipv6_addr_set(&all_routers, htonl(0xFE800000), 0, 0, 0);
> +			addrconf_ifid_isatap(all_routers.s6_addr + 8, t->parms.i_key);
> +		} else
> +			ipv6_addr_all_routers(&all_routers);
>  
>  		/*
>  		 *	If a host as already performed a random delay
> --- linux-2.6.24-rc2/net/ipv6/sit.c.orig	2007-11-08 12:03:41.000000000 -0800
> +++ linux-2.6.24-rc2/net/ipv6/sit.c	2007-11-13 09:34:31.000000000 -0800
> @@ -16,6 +16,7 @@
>   *	Changes:
>   * Roger Venning <r.venning@telstra.com>:	6to4 support
>   * Nate Thompson <nate@thebog.net>:		6to4 support
> + * Fred L. Templin <fltemplin@acm.org>:		isatap support
>   */
>  
>  #include <linux/module.h>
> @@ -182,6 +183,8 @@ static struct ip_tunnel * ipip6_tunnel_l
>  	dev->init = ipip6_tunnel_init;
>  	nt->parms = *parms;
>  
> +	if (parms->i_key) dev->priv_flags |= IFF_ISATAP;
> +

2 lines please.

>  	if (register_netdevice(dev) < 0) {
>  		free_netdev(dev);
>  		goto failed;
> @@ -364,6 +367,44 @@ static inline void ipip6_ecn_decapsulate
>  		IP6_ECN_set_ce(ipv6_hdr(skb));
>  }
>  
> +/* ISATAP (RFC4214) - check source address */
> +static inline int isatap_src_ok(struct sk_buff *skb, struct iphdr *iph, struct ip_tunnel *tunnel) {

Don't mark it 'inline' please.  It will usually automatically get inlined if it's called
only once.

Thanks
-vlad

> +	struct neighbour *neigh;
> +	struct dst_entry *dst;
> +	struct flowi fl;
> +	struct in6_addr *addr6;
> +	struct ipv6hdr *iph6;
> +	int ok = 0;
> +
> +	/* from ISATAP router */
> +	if ((iph->saddr == tunnel->parms.i_key) &&
> +	    (tunnel->parms.i_key != INADDR_NONE))
> +		return 1;
> +
> +	iph6 = ipv6_hdr(skb);
> +	addr6 = &iph6->saddr;
> +
> +	/* from legitimate previous hop */
> +	memset(&fl, 0, sizeof(fl));
> +	fl.proto = iph6->nexthdr;
> +	ipv6_addr_copy(&fl.fl6_dst, addr6);
> +	fl.oif = tunnel->dev->ifindex;
> +	security_skb_classify_flow(skb, &fl);
> +
> +	dst = ip6_route_output(NULL, &fl);
> +	if (!dst->error && (dst->dev == tunnel->dev) &&
> +	     ((neigh = dst->neighbour) != NULL)) {
> +
> +		addr6 = (struct in6_addr*)&neigh->primary_key;
> +
> +		if (ipv6_addr_is_isatap(addr6) &&
> +		    (addr6->s6_addr32[3] == iph->saddr))
> +			ok = 1;
> +    	}
> +	dst_release(dst);
> +	return ok;
> +}
> +
>  static int ipip6_rcv(struct sk_buff *skb)
>  {
>  	struct iphdr *iph;
> @@ -382,6 +423,14 @@ static int ipip6_rcv(struct sk_buff *skb
>  		IPCB(skb)->flags = 0;
>  		skb->protocol = htons(ETH_P_IPV6);
>  		skb->pkt_type = PACKET_HOST;
> +
> +		if ((tunnel->dev->priv_flags & IFF_ISATAP) &&
> +		    !isatap_src_ok(skb, iph, tunnel)) {
> +			tunnel->stat.rx_errors++;
> +			read_unlock(&ipip6_lock);
> +			kfree_skb(skb);
> +			return 0;
> +		}
>  		tunnel->stat.rx_packets++;
>  		tunnel->stat.rx_bytes += skb->len;
>  		skb->dev = tunnel->dev;
> @@ -444,6 +493,29 @@ static int ipip6_tunnel_xmit(struct sk_b
>  	if (skb->protocol != htons(ETH_P_IPV6))
>  		goto tx_error;
>  
> +	/* ISATAP (RFC4214) - must come before 6to4 */
> +	if (dev->priv_flags & IFF_ISATAP) {
> +		struct neighbour *neigh = NULL;
> +
> +		if (skb->dst)
> +			neigh = skb->dst->neighbour;
> +
> +		if (neigh == NULL) {
> +			if (net_ratelimit())
> +		    		printk(KERN_DEBUG "sit: nexthop == NULL\n");
> +			goto tx_error;
> +	    	}
> +
> +		addr6 = (struct in6_addr*)&neigh->primary_key;
> +		addr_type = ipv6_addr_type(addr6);
> +
> +		if ((addr_type & IPV6_ADDR_UNICAST) &&
> +		     ipv6_addr_is_isatap(addr6))
> +			dst = addr6->s6_addr32[3];
> +		else
> +			goto tx_error;
> +	}
> +
>  	if (!dst)
>  		dst = try_6to4(&iph6->daddr);
>  
> @@ -651,6 +723,8 @@ ipip6_tunnel_ioctl (struct net_device *d
>  				ipip6_tunnel_unlink(t);
>  				t->parms.iph.saddr = p.iph.saddr;
>  				t->parms.iph.daddr = p.iph.daddr;
> +				t->parms.i_key = p.i_key;
> +				t->parms.o_key = p.o_key;
>  				memcpy(dev->dev_addr, &p.iph.saddr, 4);
>  				memcpy(dev->broadcast, &p.iph.daddr, 4);
>  				ipip6_tunnel_link(t);
> @@ -663,6 +737,8 @@ ipip6_tunnel_ioctl (struct net_device *d
>  			if (cmd == SIOCCHGTUNNEL) {
>  				t->parms.iph.ttl = p.iph.ttl;
>  				t->parms.iph.tos = p.iph.tos;
> +				t->parms.i_key = p.i_key;
> +				t->parms.o_key = p.o_key;
>  			}
>  			if (copy_to_user(ifr->ifr_ifru.ifru_data, &t->parms, sizeof(p)))
>  				err = -EFAULT;
> 


^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Adrian Bunk @ 2007-11-13 19:04 UTC (permalink / raw)
  To: Mark Lord
  Cc: Ingo Molnar, Andrew Morton, David Miller, protasnb, linux-kernel,
	netdev, alsa-devel, linux-ide, linux-pcmcia, linux-input,
	bugme-daemon
In-Reply-To: <4739F12E.5020807@rtr.ca>

On Tue, Nov 13, 2007 at 01:47:10PM -0500, Mark Lord wrote:
> Adrian Bunk wrote:
> ...
>> I did bisecting myself, and I know that it costs time and work.
>>
>> But the first point is the above one that it makes otherwise nearly 
>> undebuggable problems debuggable and fixable.
> ..
>
> Definitely useful, no question.
>
> But the problem is now that kernel devs are addicted to it,
> many won't even consider resolving a problem any other way.
>
> That's not "maintaining" (or supporting) one's code.

What you replaced with two dots contained the answer to this:

Another point is that it shifts the work from the few experienced 
developers to the many users. Users (and voluntary testers) we have
many, but developer time for debugging bug reports is a quite scarce 
resource.

> And when a "maintainer" is too busy to find/fix their own bugs,
> that could be a sign that they've bitten off too big of a chunk
> of the kernel, and it's time for them to distribute code maintainership.

The problem is: Maintainers don't grow on trees.

You need people who are both technically capable and willing to spend 
time on the non-sexy task of debugging problems.

Where do you plan to find them?

If you don't believe me, please find a maintainer for the currently 
unmaintained parallel port support.

Or if you want a harder task, find a maintainer for the floppy driver...

> Cheers

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

^ permalink raw reply

* Re: [PATCH][RFC take 2] Add support for the RDC R6040 Fast Ethernet controller
From: Stephen Hemminger @ 2007-11-13 19:09 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev, Jeff Garzik
In-Reply-To: <200711101922.19004.florian.fainelli@telecomint.eu>

On Sat, 10 Nov 2007 19:22:18 +0100
Florian Fainelli <florian.fainelli@telecomint.eu> wrote:

> This patch adds support for the RDC R6040 MAC we can find in the RDC R-321x System-on-chips and some other devices.
> You will need the RDC PCI identifiers if you want to test this driver :
> 
> RDC_PCI_VENDOR_ID = 0x17f3
> RDC_PCI_DEVICE_ID_RDC_R6040 = 0x6040
> 
> Changes from first patch :
> - use ioread/iowrite
> - cleaned up NAPI
> - suppressed wrong use of local_irq_enable/disable
> - handle multicast cases
> - notify when the carrier changes
> - add ethtool routines
> - rewrite IRQ handling with
> - cleaned up PCI table
> - make checkpatch happy
> - documented registers
> - 
> 
> (I do not mention everything you have been saying in your mails, but I have fixed them as well).
> 
> Thanks to Jeff and Stephen for their detailed comments.


More comments

> +static void
> +r6040_tx_timeout(struct net_device *dev)
> +{
> +	struct r6040_private *priv = netdev_priv(dev);
> +
> +	disable_irq(dev->irq);
> +	napi_disable(&priv->napi);
> +	spin_lock(&priv->lock);
> +	dev->stats.tx_errors++;
> +	spin_unlock(&priv->lock);
> +
> +	netif_stop_queue(dev);

Message!

Also, the intention is for driver to do recovery on transmit timeout


> +}
> +
> +/* Allocate skb buffer for rx descriptor */
> +static void rx_buf_alloc(struct r6040_private *lp, struct net_device *dev)
> +{
> +	struct r6040_descriptor *descptr;
> +	void __iomem *ioaddr = lp->base;
> +
> +	descptr = lp->rx_insert_ptr;
> +	while (lp->rx_free_desc < RX_DCNT) {
> +		descptr->skb_ptr = dev_alloc_skb(MAX_BUF_SIZE);

Use netdev_alloc_skb()

You may get better performance if you allocate slightly larger buffer
and use skb_reserve(skb, NET_IP_ALIGN)

> +
> +		if (!descptr->skb_ptr)
> +			break;
> +		descptr->buf = cpu_to_le32(pci_map_single(lp->pdev,
> +			descptr->skb_ptr->tail,
> +			MAX_BUF_SIZE, PCI_DMA_FROMDEVICE));
> +		descptr->status = 0x8000;
> +		descptr = descptr->vndescp;
> +		lp->rx_free_desc++;
> +		/* Trigger RX DMA */
> +		iowrite16(lp->mcr0 | 0x0002, ioaddr);
> +	}
> +	lp->rx_insert_ptr = descptr;
> +}
> +
> +
> +static struct net_device_stats *r6040_get_stats(struct net_device *dev)
> +{
> +	struct r6040_private *priv = netdev_priv(dev);
> +	void __iomem *ioaddr = priv->base;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	priv->stats.rx_crc_errors += ioread8(ioaddr + ME_CNT1);
> +	priv->stats.multicast += ioread8(ioaddr + ME_CNT0);
> +	spin_unlock_irqrestore(&priv->lock, flags);

Don't need to have net_device_stats in r6040_private, there is space
already reserved in dev->net_stats.

> +	return &priv->stats;
> +}
> +

> +static int r6040_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> +{
> +	struct r6040_private *lp = netdev_priv(dev);
> +	struct mii_ioctl_data *data = (struct mii_ioctl_data *) &rq->ifr_data;
> +	int rc;
> +
> +	if (!netif_running(dev))
> +		return -EINVAL;
> +	spin_lock_irq(&lp->lock);
> +	rc = generic_mii_ioctl(&lp->mii_if, data, cmd, NULL);
> +	spin_unlock_irq(&lp->lock);
> +	r6040_set_carrier(&lp->mii_if);
> +	return rc;
> +}
> +
> +static int r6040_rx(struct net_device *dev, int limit)
> +{
> +	struct r6040_private *priv = netdev_priv(dev);
> +	int count;
> +	void __iomem *ioaddr = priv->base;
> +	u16 err;
> +
> +	for (count = 0; count < limit; ++count) {
> +		struct r6040_descriptor *descptr = priv->rx_remove_ptr;
> +		struct sk_buff *skb_ptr;
> +
> +		/* Disable RX interrupt */
> +		iowrite16(ioread16(ioaddr + MIER) & (~RX_INT), ioaddr + MIER);
> +		descptr = priv->rx_remove_ptr;
> +
> +		/* Check for errors */
> +		err = ioread16(ioaddr + MLSR);
> +		if (err & 0x0400) priv->stats.rx_errors++;
> +		/* RX FIFO over-run */
> +		if (err & 0x8000) priv->stats.rx_fifo_errors++;
> +		/* RX descriptor unavailable */
> +		if (err & 0x0080) priv->stats.rx_frame_errors++;
> +		/* Received packet with length over buffer lenght */
> +		if (err & 0x0020) priv->stats.rx_over_errors++;
> +		/* Received packet with too long or short */
> +		if (err & (0x0010|0x0008)) priv->stats.rx_length_errors++;
> +		/* Received packet with CRC errors */
> +		if (err & 0x0004) {
> +			spin_lock(&priv->lock);
> +			priv->stats.rx_crc_errors++;
> +			spin_unlock(&priv->lock);
> +		}
> +
> +		while (priv->rx_free_desc) {
> +			/* No RX packet */
> +			if (descptr->status & 0x8000)
> +				break;
> +			skb_ptr = descptr->skb_ptr;
> +			if (!skb_ptr) {
> +				printk(KERN_ERR "%s: Inconsistent RX"
> +					"descriptor chain\n",
> +					dev->name);
> +				break;
> +			}
> +			descptr->skb_ptr = NULL;
> +			skb_ptr->dev = priv->dev;
> +			/* Do not count the CRC */
> +			skb_put(skb_ptr, descptr->len - 4);
> +			pci_unmap_single(priv->pdev, descptr->buf,
> +				MAX_BUF_SIZE, PCI_DMA_FROMDEVICE);
> +			skb_ptr->protocol = eth_type_trans(skb_ptr, priv->dev);
> +			/* Send to upper layer */
> +			netif_receive_skb(skb_ptr);
> +			dev->last_rx = jiffies;
> +			priv->dev->stats.rx_packets++;
> +			priv->dev->stats.rx_bytes += descptr->len;
> +			/* To next descriptor */
> +			descptr = descptr->vndescp;
> +			priv->rx_free_desc--;
> +		}
> +		priv->rx_remove_ptr = descptr;
> +	}
> +	/* Allocate new RX buffer */
> +	if (priv->rx_free_desc < RX_DCNT)
> +		rx_buf_alloc(priv, priv->dev);
> +
> +	return count;
> +}
> +
> +static void r6040_tx(struct net_device *dev)
> +{
> +	struct r6040_private *priv = netdev_priv(dev);
> +	struct r6040_descriptor *descptr;
> +	void __iomem *ioaddr = priv->base;
> +	struct sk_buff *skb_ptr;
> +	u16 err;
> +
> +	spin_lock(&priv->lock);
> +	descptr = priv->tx_remove_ptr;
> +	while (priv->tx_free_desc < TX_DCNT) {
> +		/* Check for errors */
> +		err = ioread16(ioaddr + MLSR);
> +
> +		if (err & 0x0200) priv->stats.rx_fifo_errors++;
> +		if (err & (0x2000 | 0x4000)) priv->stats.tx_carrier_errors++;

Break these into two lines.

> +		if (descptr->status & 0x8000)
> +			break; /* Not complte */

Fix spelling.

> +		skb_ptr = descptr->skb_ptr;
> +		pci_unmap_single(priv->pdev, descptr->buf,
> +			skb_ptr->len, PCI_DMA_TODEVICE);
> +		/* Free buffer */
> +		dev_kfree_skb_irq(skb_ptr);
> +		descptr->skb_ptr = NULL;
> +		/* To next descriptor */
> +		descptr = descptr->vndescp;
> +		priv->tx_free_desc++;
> +	}
> +	priv->tx_remove_ptr = descptr;
> +
> +	if (priv->tx_free_desc)
> +		netif_wake_queue(dev);
> +	spin_unlock(&priv->lock);
> +}
>
> +/* The RDC interrupt handler. */
> +static irqreturn_t r6040_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct r6040_private *lp = netdev_priv(dev);
> +	void __iomem *ioaddr = lp->base;
> +	u16 status;
> +	int handled = 1;

Don't bother with handled...

> +	/* Mask off RDC MAC interrupt */
> +	iowrite16(MSK_INT, ioaddr + MIER);
> +	/* Read MISR status and clear */
> +	status = ioread16(ioaddr + MISR);
> +
> +	if (status == 0x0000 || status == 0xffff)
> +		return IRQ_NONE;
> +
> +	/* RX interrupt request */
> +	if (status & 0x01) {
> +		netif_rx_schedule(dev, &lp->napi);
> +		iowrite16(TX_INT, ioaddr + MIER);
> +	}
> +
> +	/* TX interrupt request */
> +	if (status & 0x10)
> +		r6040_tx(dev);
> +
> +	return IRQ_RETVAL(handled);
	return IRQ_HANDLED;

> +}
> +
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +static void r6040_poll_controller(struct net_device *dev)
> +{
> +	disable_irq(dev->irq);
> +	r6040_interrupt(dev->irq, (void *)dev);
> +	enable_irq(dev->irq);
> +}
> +#endif
> +
> +
> +/* Init RDC MAC */
> +static void r6040_up(struct net_device *dev)
> +{
> +	struct r6040_private *lp = netdev_priv(dev);
> +	struct r6040_descriptor *descptr;
> +	void __iomem *ioaddr = lp->base;
> +	int i;
> +	__le32 tmp_addr;
> +	dma_addr_t desc_dma, start_dma;
> +
> +	/* Initialize */
> +	lp->tx_free_desc = TX_DCNT;
> +	lp->rx_free_desc = 0;
> +	/* Init descriptor */
> +	memset(lp->desc_pool, 0, ALLOC_DESC_SIZE); /* Let all descriptor = 0 */
> +	lp->tx_insert_ptr = (struct r6040_descriptor *)lp->desc_pool;
> +	lp->tx_remove_ptr = lp->tx_insert_ptr;
> +	lp->rx_insert_ptr = (struct r6040_descriptor *)lp->tx_insert_ptr +
> +		TX_DCNT;
> +	lp->rx_remove_ptr = lp->rx_insert_ptr;
> +	/* Init TX descriptor */
> +	descptr = lp->tx_insert_ptr;
> +	desc_dma = lp->desc_dma;
> +	start_dma = desc_dma;
> +	for (i = 0; i < TX_DCNT; i++) {
> +		descptr->ndesc = cpu_to_le32(desc_dma +
> +			sizeof(struct r6040_descriptor));
> +		descptr->vndescp = (descptr + 1);
> +		descptr = (descptr + 1);
> +		desc_dma += sizeof(struct r6040_descriptor);
> +	}
> +	(descptr - 1)->ndesc = cpu_to_le32(start_dma);
> +	(descptr - 1)->vndescp = lp->tx_insert_ptr;
> +	/* Init RX descriptor */
> +	start_dma = desc_dma;
> +	descptr = lp->rx_insert_ptr;
> +	for (i = 0; i < RX_DCNT; i++) {
> +		descptr->ndesc = cpu_to_le32(desc_dma +
> +			sizeof(struct r6040_descriptor));
> +		descptr->vndescp = (descptr + 1);
> +		descptr = (descptr + 1);
> +		desc_dma += sizeof(struct r6040_descriptor);
> +	}
> +	(descptr - 1)->ndesc = cpu_to_le32(start_dma);
> +	(descptr - 1)->vndescp = lp->rx_insert_ptr;
> +
> +	/* Allocate buffer for RX descriptor */
> +	rx_buf_alloc(lp, dev);
> +
> +	/* TX and RX descriptor start Register */
> +	tmp_addr = cpu_to_le32((u32)lp->tx_insert_ptr);
> +	tmp_addr = virt_to_bus((volatile void *)tmp_addr);
> +	iowrite16(tmp_addr, ioaddr + MTD_SA0);
> +	iowrite16(tmp_addr >> 16, ioaddr + MTD_SA1);
> +	tmp_addr = cpu_to_le32((u32)lp->rx_insert_ptr);
> +	tmp_addr = virt_to_bus((volatile void *)tmp_addr);
> +	iowrite16(tmp_addr, ioaddr + MRD_SA0);
> +	iowrite16(tmp_addr >> 16, ioaddr + MRD_SA1);
> +
> +	/* Buffer Size Register */
> +	iowrite16(MAX_BUF_SIZE, ioaddr + MR_BSR);
> +	/* Read the PHY ID */
> +	lp->switch_sig = phy_read(ioaddr, 0, 2);
> +
> +	if (lp->switch_sig  == ICPLUS_PHY_ID) {
> +		phy_write(ioaddr, 29, 31, 0x175C); /* Enable registers */
> +		lp->phy_mode = 0x8000;
> +	} else {
> +		/* PHY Mode Check */
> +		phy_write(ioaddr, lp->phy_addr, 4, PHY_CAP);
> +		phy_write(ioaddr, lp->phy_addr, 0, PHY_MODE);
> +
> +		if (PHY_MODE == 0x3100)
> +			lp->phy_mode = phy_mode_chk(dev);
> +		else
> +			lp->phy_mode = (PHY_MODE & 0x0100) ? 0x8000:0x0;
> +	}
> +	/* MAC Bus Control Register */
> +	iowrite16(MBCR_DEFAULT, ioaddr + MBCR);
> +
> +	/* MAC TX/RX Enable */
> +	lp->mcr0 |= lp->phy_mode;
> +	iowrite16(lp->mcr0, ioaddr);
> +
> +	/* set interrupt waiting time and packet numbers */
> +	iowrite16(0x0F06, ioaddr + MT_ICR);
> +	iowrite16(0x0F06, ioaddr + MR_ICR);
> +
> +	/* improve performance (by RDC guys) */
> +	phy_write(ioaddr, 30, 17, (phy_read(ioaddr, 30, 17) | 0x4000));
> +	phy_write(ioaddr, 30, 17, ~((~phy_read(ioaddr, 30, 17)) | 0x2000));
> +	phy_write(ioaddr, 0, 19, 0x0000);
> +	phy_write(ioaddr, 0, 30, 0x01F0);
> +
> +	/* Interrupt Mask Register */
> +	iowrite16(INT_MASK, ioaddr + MIER);
> +}
> +
> +/*
> +  A periodic timer routine
> +	Polling PHY Chip Link Status
> +*/
> +static void r6040_timer(unsigned long data)
> +{
> +	struct net_device *dev = (struct net_device *)data;
> +	struct r6040_private *lp = netdev_priv(dev->priv);
> +	void __iomem *ioaddr = lp->base;
> +	u16 phy_mode;
> +
> +	/* Polling PHY Chip Status */
> +	if (PHY_MODE == 0x3100)
> +		phy_mode = phy_mode_chk(dev);
> +	else
> +		phy_mode = (PHY_MODE & 0x0100) ? 0x8000:0x0;
> +
> +	if (phy_mode != lp->phy_mode) {
> +		lp->phy_mode = phy_mode;
> +		lp->mcr0 = (lp->mcr0 & 0x7fff) | phy_mode;
> +		iowrite16(lp->mcr0, ioaddr);
> +		printk(KERN_INFO "Link Change %x \n", ioread16(ioaddr));
> +	}
> +
> +	/* Timer active again */
> +	lp->timer.expires = TIMER_WUT;
> +	add_timer(&lp->timer);

	Don't use add_timer like this it is racy.
	You should be doing:

	mod_timer(&lp->timer, jiffies + round_jiffies(HZ));

	Get rid of TIMER_WUT


> +
> +/* Read/set MAC address routines */
> +static void r6040_mac_address(struct net_device *dev)
> +{
> +	struct r6040_private *lp = netdev_priv(dev);
> +	void __iomem *ioaddr = lp->base;
> +	u16 *adrp;
> +
> +	/* MAC operation register */
> +	iowrite16(0x01, ioaddr + MCR1); /* Reset MAC */
> +	iowrite16(2, ioaddr + MAC_SM); /* Reset internal state machine */
> +	iowrite16(0, ioaddr + MAC_SM);
> +	udelay(5000);
> +
> +	/* Restore MAC Address */
> +	adrp = (u16 *) dev->dev_addr;
> +	iowrite16(adrp[0], ioaddr + MID_0L);
> +	iowrite16(adrp[1], ioaddr + MID_0M);
> +	iowrite16(adrp[2], ioaddr + MID_0H);
> +}
> +
> +static int
> +r6040_open(struct net_device *dev)
> +{
> +	struct r6040_private *lp = dev->priv;
> +	int ret;
> +
> +	/* Request IRQ and Register interrupt handler */
> +	ret = request_irq(dev->irq, &r6040_interrupt,
> +		IRQF_SHARED, dev->name, dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Set MAC address */
> +	r6040_mac_address(dev);
> +
> +	/* Allocate Descriptor memory */
> +	lp->desc_pool = pci_alloc_consistent(lp->pdev,
> +		ALLOC_DESC_SIZE, &lp->desc_dma);
> +	if (!lp->desc_pool)
> +		return -ENOMEM;
> +
> +	r6040_up(dev);
> +
> +	napi_enable(&lp->napi);
> +	netif_start_queue(dev);
> +
> +	if (lp->switch_sig != ICPLUS_PHY_ID) {
> +		/* set and active a timer process */
> +		init_timer(&lp->timer);
> +		lp->timer.expires = TIMER_WUT;
> +		lp->timer.data = (unsigned long)dev;
> +		lp->timer.function = &r6040_timer;
> +		add_timer(&lp->timer);
> +	}
> +	return 0;
> +}
> +
> +static int
> +r6040_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct r6040_private *lp = netdev_priv(dev);
> +	struct r6040_descriptor *descptr;
> +	void __iomem *ioaddr = lp->base;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!skb)	/* NULL skb directly return */
> +		return ret;

The start_xmit() should never be called with NULL!

> +	if (skb->len >= MAX_BUF_SIZE) {	/* Packet too long, drop it */
> +		dev_kfree_skb(skb);
> +		return ret;
ret is not initialized!  
		return NETDEV_TX_OK;
> +	}
> +
> +	/* Critical Section */
> +	spin_lock_irqsave(&lp->lock, flags);
> +
> +	/* TX resource check */
> +	if (!lp->tx_free_desc) {
> +		spin_unlock_irqrestore(&lp->lock, flags);
> +		printk(KERN_ERR DRV_NAME ": no tx descriptor\n");
> +		ret = 1;
> +		return ret;
Use instead
		return NETDEV_TX_BUSY;
and don't free skb!

> +	}
> +
> +	/* Statistic Counter */
> +	dev->stats.tx_packets++;
> +	dev->stats.tx_bytes += skb->len;
> +	/* Set TX descriptor & Transmit it */
> +	lp->tx_free_desc--;
> +	descptr = lp->tx_insert_ptr;
> +	if (skb->len < MISR)
> +		descptr->len = MISR;
> +	else
> +		descptr->len = skb->len;
> +
> +	descptr->skb_ptr = skb;
> +	descptr->buf = cpu_to_le32(pci_map_single(lp->pdev,
> +		skb->data, skb->len, PCI_DMA_TODEVICE));
> +	descptr->status = 0x8000;
> +	/* Trigger the MAC to check the TX descriptor */
> +	iowrite16(0x01, ioaddr + MTPR);
> +	lp->tx_insert_ptr = descptr->vndescp;
> +
> +	/* If no tx resource, stop */
> +	if (!lp->tx_free_desc)
> +		netif_stop_queue(dev);
> +
> +	dev->trans_start = jiffies;
> +	spin_unlock_irqrestore(&lp->lock, flags);
> +	return ret;
> +}
> +
> +static void
> +r6040_multicast_list(struct net_device *dev)
> +{
> +	struct r6040_private *lp = netdev_priv(dev);
> +	void __iomem *ioaddr = lp->base;
> +	u16 *adrp;
> +	u16 reg;
> +	unsigned long flags;
> +	struct dev_mc_list *dmi = dev->mc_list;
> +	int i;
> +
> +	/* MAC Address */
> +	adrp = (u16 *)dev->dev_addr;
> +	iowrite16(adrp[0], ioaddr + MID_0L);
> +	iowrite16(adrp[1], ioaddr + MID_0M);
> +	iowrite16(adrp[2], ioaddr + MID_0H);
> +
> +	/* Promiscous Mode */
> +	spin_lock_irqsave(&lp->lock, flags);
> +
> +	/* Clear AMCP & PROM bits */
> +	reg = ioread16(ioaddr) & ~0x0120;
> +	if (dev->flags & IFF_PROMISC) {
> +		reg |= 0x0020;
> +		lp->mcr0 |= 0x0020;
> +	}
> +	/* Too many multicast addresses
> +	 * accept all traffic */
> +	else if ((dev->mc_count > MCAST_MAX)
> +		|| (dev->flags & IFF_ALLMULTI))
> +		reg |= 0x0020;
> +
> +	iowrite16(reg, ioaddr);
> +	spin_unlock_irqrestore(&lp->lock, flags);
> +
> +	/* Build the hash table */
> +	if (dev->mc_count > MCAST_MAX) {
> +		u16 hash_table[4];
> +		u32 crc;
> +
> +		for (i = 0; i < 4; i++)
> +			hash_table[i] = 0;
> +
> +		for (i = 0; i < dev->mc_count; i++) {
> +			char *addrs = dmi->dmi_addr;
> +
> +			dmi = dmi->next;
> +
> +			if (!(*addrs & 1))
> +				continue;
> +
> +			crc = ether_crc_le(6, addrs);
> +			crc >>= 26;
> +			hash_table[crc >> 4] |= 1 << (15 - (crc & 0xf));
> +		}
> +		/* Write the index of the hash table */
> +		for (i = 0; i < 4; i++)
> +			iowrite16(hash_table[i] << 14, ioaddr + MCR1);
> +		/* Fill the MAC hash tables with their values */
> +		iowrite16(hash_table[0], ioaddr + MAR0);
> +		iowrite16(hash_table[1], ioaddr + MAR1);
> +		iowrite16(hash_table[2], ioaddr + MAR2);
> +		iowrite16(hash_table[3], ioaddr + MAR3);
> +	}
> +	/* Multicast Address 1~4 case */
> +	for (i = 0, dmi; (i < dev->mc_count) && (i < MCAST_MAX); i++) {
> +		adrp = (u16 *)dmi->dmi_addr;
> +		iowrite16(adrp[0], ioaddr + MID_1L + 8*i);
> +		iowrite16(adrp[1], ioaddr + MID_1M + 8*i);
> +		iowrite16(adrp[2], ioaddr + MID_1H + 8*i);
> +		dmi = dmi->next;
> +	}
> +	for (i = dev->mc_count; i < MCAST_MAX; i++) {
> +		iowrite16(0xffff, ioaddr + MID_0L + 8*i);
> +		iowrite16(0xffff, ioaddr + MID_0M + 8*i);
> +		iowrite16(0xffff, ioaddr + MID_0H + 8*i);
> +	}
> +}
> +
> +static void netdev_get_drvinfo(struct net_device *dev,
> +			struct ethtool_drvinfo *info)
> +{
> +	struct r6040_private *rp = netdev_priv(dev);
> +
> +	strcpy(info->driver, DRV_NAME);
> +	strcpy(info->version, DRV_VERSION);
> +	strcpy(info->bus_info, pci_name(rp->pdev));
> +}
> +
> +static int netdev_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +	struct r6040_private *rp = netdev_priv(dev);
> +	int rc;
> +
> +	spin_lock_irq(&rp->lock);
> +	rc = mii_ethtool_gset(&rp->mii_if, cmd);
> +	spin_unlock_irq(&rp->mii_if);
	
Shouldn't this be?
	spin_unlock_irq(&rp->lock)


> +static struct pci_device_id r6040_pci_tbl[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_RDC, PCI_DEVICE_ID_RDC_R6040) },
> +	{0 }
Balance space?

	{ 0 }
> +};
> +MODULE_DEVICE_TABLE(pci, r6040_pci_tbl);
> +


More warnings from checkpatch.pl

WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#739: FILE: drivers/net/r6040.c:632:
+       tmp_addr = virt_to_bus((volatile void *)tmp_addr);

WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#743: FILE: drivers/net/r6040.c:636:
+       tmp_addr = virt_to_bus((volatile void *)tmp_addr);

total: 0 errors, 2 warnings, 1135 lines checked


^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Mark Lord @ 2007-11-13 19:12 UTC (permalink / raw)
  To: Adrian Bunk
  Cc: Ingo Molnar, Andrew Morton, David Miller, protasnb, linux-kernel,
	netdev, alsa-devel, linux-ide, linux-pcmcia, linux-input,
	bugme-daemon
In-Reply-To: <20071113190428.GH4250@stusta.de>

Adrian Bunk wrote:
> On Tue, Nov 13, 2007 at 01:47:10PM -0500, Mark Lord wrote:
>> Adrian Bunk wrote:
>> ...
>>> I did bisecting myself, and I know that it costs time and work.
>>>
>>> But the first point is the above one that it makes otherwise nearly 
>>> undebuggable problems debuggable and fixable.
>> ..
>>
>> Definitely useful, no question.
>>
>> But the problem is now that kernel devs are addicted to it,
>> many won't even consider resolving a problem any other way.
>>
>> That's not "maintaining" (or supporting) one's code.
> 
> What you replaced with two dots contained the answer to this:
> 
> Another point is that it shifts the work from the few experienced 
> developers to the many users. Users (and voluntary testers) we have
> many, but developer time for debugging bug reports is a quite scarce 
> resource.
> 
>> And when a "maintainer" is too busy to find/fix their own bugs,
>> that could be a sign that they've bitten off too big of a chunk
>> of the kernel, and it's time for them to distribute code maintainership.
> 
> The problem is: Maintainers don't grow on trees.
> 
> You need people who are both technically capable and willing to spend 
> time on the non-sexy task of debugging problems.
> 
> Where do you plan to find them?
> 
> If you don't believe me, please find a maintainer for the currently 
> unmaintained parallel port support.
> 
> Or if you want a harder task, find a maintainer for the floppy driver...
..

Again, the problem is:

> But the problem is now that kernel devs are addicted to it,
> many won't even consider resolving a problem any other way.

And that's simply not good enough.


^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Mark Lord @ 2007-11-13 19:26 UTC (permalink / raw)
  To: Adrian Bunk
  Cc: Ingo Molnar, Andrew Morton, David Miller, protasnb, linux-kernel,
	netdev, alsa-devel, linux-ide, linux-pcmcia, linux-input,
	bugme-daemon
In-Reply-To: <20071113190428.GH4250@stusta.de>

Adrian Bunk wrote:
> On Tue, Nov 13, 2007 at 01:47:10PM -0500, Mark Lord wrote:
>> Adrian Bunk wrote:
..
> Another point is that it shifts the work from the few experienced 
> developers to the many users. Users (and voluntary testers) we have
> many, but developer time for debugging bug reports is a quite scarce 
> resource.
> 
>> And when a "maintainer" is too busy to find/fix their own bugs,
>> that could be a sign that they've bitten off too big of a chunk
>> of the kernel, and it's time for them to distribute code maintainership.
> 
> The problem is: Maintainers don't grow on trees.
..

Hey, if somebody has time to break things, then they damn well ought
to be able to make time to fix them again.  And the best developers
here on LKML do just that (fix what they break).

You broke it, you fix it.  A simple rule.

Translation for the particularly daft:

If you've been making significant updates to a driver/subsystem,
and people are reporting that it is now broken for them,
then it's your job to make it right.  The reporters can help,
and many may even git-bisect or send patches.  

But you cannot *expect* or *insist* upon them doing your job.

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Adrian Bunk @ 2007-11-13 19:30 UTC (permalink / raw)
  To: Mark Lord
  Cc: Ingo Molnar, Andrew Morton, David Miller, protasnb, linux-kernel,
	netdev, alsa-devel, linux-ide, linux-pcmcia, linux-input,
	bugme-daemon
In-Reply-To: <4739F739.5040708@rtr.ca>

On Tue, Nov 13, 2007 at 02:12:57PM -0500, Mark Lord wrote:
> Adrian Bunk wrote:
>> On Tue, Nov 13, 2007 at 01:47:10PM -0500, Mark Lord wrote:
>>> Adrian Bunk wrote:
>>> ...
>>>> I did bisecting myself, and I know that it costs time and work.
>>>>
>>>> But the first point is the above one that it makes otherwise nearly 
>>>> undebuggable problems debuggable and fixable.
>>> ..
>>>
>>> Definitely useful, no question.
>>>
>>> But the problem is now that kernel devs are addicted to it,
>>> many won't even consider resolving a problem any other way.
>>>
>>> That's not "maintaining" (or supporting) one's code.
>>
>> What you replaced with two dots contained the answer to this:
>>
>> Another point is that it shifts the work from the few experienced 
>> developers to the many users. Users (and voluntary testers) we have
>> many, but developer time for debugging bug reports is a quite scarce 
>> resource.
>>
>>> And when a "maintainer" is too busy to find/fix their own bugs,
>>> that could be a sign that they've bitten off too big of a chunk
>>> of the kernel, and it's time for them to distribute code maintainership.
>>
>> The problem is: Maintainers don't grow on trees.
>>
>> You need people who are both technically capable and willing to spend time 
>> on the non-sexy task of debugging problems.
>>
>> Where do you plan to find them?
>>
>> If you don't believe me, please find a maintainer for the currently 
>> unmaintained parallel port support.
>>
>> Or if you want a harder task, find a maintainer for the floppy driver...
> ..
>
> Again, the problem is:
>
>> But the problem is now that kernel devs are addicted to it,
>> many won't even consider resolving a problem any other way.
>
> And that's simply not good enough.

There is this silly limit that noone can work more than 168 hours per 
week on the Linux kernel, and some kernel developers seem to take the 
liberty of spending even less time on kernel development...

Considering our problems to cope with the amount of incoming bug 
reports, everything that would require a kernel developer to spend more 
time for getting a bug fixed would be a horrible mistake.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Russell King @ 2007-11-13 19:32 UTC (permalink / raw)
  To: David Miller
  Cc: akpm, alsa-devel, netdev, linux-pcmcia, linux-kernel, protasnb,
	linux-ide, bugme-daemon, linux-input
In-Reply-To: <20071113.043207.44732743.davem@davemloft.net>

On Tue, Nov 13, 2007 at 04:32:07AM -0800, David Miller wrote:
> Luckily if the report being ignored isn't chaff, it will show up again
> (and again and again) and this triggers a reprioritization because not
> only is the bug no longer chaff, it also now got a lot of information
> tagged to it so it's a double worthwhile investment to work on the
> problem.

Strongly agree.

This is exactly what happened to that ARM NO_HZ bug report.  The report
in bugzilla was rather lacking (and wrong) in ways that have already
been described.  HPET on ARM? 8)

Then on the morning of 6th November, someone reported on the mailing list
that "pxa270 doesn't work with oneshot timer" and that was the trigger to
getting the bug resolved - because it was a narrowly defined bug report.

Since it was a narrowly defined bug report, it became very easy to
investigate and resolve.  About half an hour of time for an initial
patch.


There's another issue I want to raise concerning bugzilla.  We have the
classic case of "not enough people reading bugzilla bugs" - which is one
of the biggest problems with bugzilla.  Virtually no one in the ARM
community looks for ARM bugs in bugzilla.

Let's not forget that it would be a waste of time for people to manually
check bugzilla for ARM bugs.  There's soo few people reporting ARM bugs
into bugzilla that a weekly manual check by every maintainer would just
return the same old boring results for months and months at a time.

It would be far more productive if the ARM category was deleted from
bugzilla and the few people who use bugzilla reported their bugs on the
mailing list.  We've a couple of thousand people on the ARM kernel
mailing list at the moment - that's 3 orders of magnitude more of eyes
than look at bugzilla.

(I'm not saying that if the ARM NO_HZ bug as reported in bugzilla had
been reported on the correct mailing list would've been solved earlier;
I doubt there'd be much difference.  However, the probability of a
question being asked of the reporter would've been much higher, and
_that_ might have led to an earlier resolution.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply

* [RFC 6/7] LTTng instrumentation net
From: Mathieu Desnoyers @ 2007-11-13 19:33 UTC (permalink / raw)
  To: akpm, linux-kernel; +Cc: Mathieu Desnoyers, netdev
In-Reply-To: <20071113193349.214098508@polymtl.ca>

[-- Attachment #1: lttng-instrumentation-net.patch --]
[-- Type: text/plain, Size: 3739 bytes --]

Network core events.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
CC: netdev@vger.kernel.org
---
 net/core/dev.c     |    5 +++++
 net/ipv4/devinet.c |    5 +++++
 net/socket.c       |   18 ++++++++++++++++++
 3 files changed, 28 insertions(+)

Index: linux-2.6-lttng/net/core/dev.c
===================================================================
--- linux-2.6-lttng.orig/net/core/dev.c	2007-11-13 09:25:26.000000000 -0500
+++ linux-2.6-lttng/net/core/dev.c	2007-11-13 09:49:37.000000000 -0500
@@ -1637,6 +1637,8 @@ int dev_queue_xmit(struct sk_buff *skb)
 	}
 
 gso:
+	trace_mark(net_dev_xmit, "skb %p protocol #2u%hu", skb, skb->protocol);
+
 	spin_lock_prefetch(&dev->queue_lock);
 
 	/* Disable soft irqs for various locks below. Also
@@ -2037,6 +2039,9 @@ int netif_receive_skb(struct sk_buff *sk
 
 	__get_cpu_var(netdev_rx_stat).total++;
 
+	trace_mark(net_dev_receive, "skb %p protocol #2u%hu",
+		skb, skb->protocol);
+
 	skb_reset_network_header(skb);
 	skb_reset_transport_header(skb);
 	skb->mac_len = skb->network_header - skb->mac_header;
Index: linux-2.6-lttng/net/ipv4/devinet.c
===================================================================
--- linux-2.6-lttng.orig/net/ipv4/devinet.c	2007-11-13 09:25:26.000000000 -0500
+++ linux-2.6-lttng/net/ipv4/devinet.c	2007-11-13 09:49:37.000000000 -0500
@@ -262,6 +262,8 @@ static void __inet_del_ifa(struct in_dev
 		struct in_ifaddr **ifap1 = &ifa1->ifa_next;
 
 		while ((ifa = *ifap1) != NULL) {
+			trace_mark(net_del_ifa_ipv4, "label %s",
+				ifa->ifa_label);
 			if (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
 			    ifa1->ifa_scope <= ifa->ifa_scope)
 				last_prim = ifa;
@@ -368,6 +370,9 @@ static int __inet_insert_ifa(struct in_i
 			}
 			ifa->ifa_flags |= IFA_F_SECONDARY;
 		}
+		trace_mark(net_insert_ifa_ipv4, "label %s address #4u%lu",
+			ifa->ifa_label,
+			(unsigned long)ifa->ifa_address);
 	}
 
 	if (!(ifa->ifa_flags & IFA_F_SECONDARY)) {
Index: linux-2.6-lttng/net/socket.c
===================================================================
--- linux-2.6-lttng.orig/net/socket.c	2007-11-13 09:25:26.000000000 -0500
+++ linux-2.6-lttng/net/socket.c	2007-11-13 09:49:37.000000000 -0500
@@ -563,6 +563,11 @@ int sock_sendmsg(struct socket *sock, st
 	struct sock_iocb siocb;
 	int ret;
 
+	trace_mark(net_socket_sendmsg,
+		"sock %p family %d type %d protocol %d size %zu",
+		sock, sock->sk->sk_family, sock->sk->sk_type,
+		sock->sk->sk_protocol, size);
+
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
 	ret = __sock_sendmsg(&iocb, sock, msg, size);
@@ -646,7 +651,13 @@ int sock_recvmsg(struct socket *sock, st
 	struct sock_iocb siocb;
 	int ret;
 
+	trace_mark(net_socket_recvmsg,
+		"sock %p family %d type %d protocol %d size %zu",
+		sock, sock->sk->sk_family, sock->sk->sk_type,
+		sock->sk->sk_protocol, size);
+
 	init_sync_kiocb(&iocb, NULL);
+
 	iocb.private = &siocb;
 	ret = __sock_recvmsg(&iocb, sock, msg, size, flags);
 	if (-EIOCBQUEUED == ret)
@@ -1212,6 +1223,11 @@ asmlinkage long sys_socket(int family, i
 	if (retval < 0)
 		goto out_release;
 
+	trace_mark(net_socket_create,
+		"sock %p family %d type %d protocol %d fd %d",
+		sock, sock->sk->sk_family, sock->sk->sk_type,
+		sock->sk->sk_protocol, retval);
+
 out:
 	/* It may be already another descriptor 8) Not kernel problem. */
 	return retval;
@@ -2021,6 +2037,8 @@ asmlinkage long sys_socketcall(int call,
 	a0 = a[0];
 	a1 = a[1];
 
+	trace_mark(net_socket_call, "call %d a0 %lu", call, a0);
+
 	switch (call) {
 	case SYS_SOCKET:
 		err = sys_socket(a0, a1, a[2]);

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Russell King @ 2007-11-13 19:37 UTC (permalink / raw)
  To: Mark Lord
  Cc: Ingo Molnar, alsa-devel, netdev, linux-pcmcia, linux-kernel,
	protasnb, linux-ide, bugme-daemon, linux-input, Andrew Morton,
	David Miller
In-Reply-To: <4739AFE0.20705@rtr.ca>

On Tue, Nov 13, 2007 at 09:08:32AM -0500, Mark Lord wrote:
> Ingo Molnar wrote:
> ..
> > This is all QA-101 that _cannot be argued against on a rational basis_, 
> > it's just that these sorts of things have been largely ignored for 
> > years, in favor of the all-too-easy "open source means many eyeballs and 
> > that is our QA" answer, which is a _good_ answer but by far not the most 
> > intelligent answer! Today "many eyeballs" is simply not good enough and 
> > nature (and other OS projects) will route us around if we dont change.
> ..
> 
> QA-101 and "many eyeballs" are not at all in opposition.
> The latter is how we find out about bugs on uncommon hardware,
> and the former is what we need to track them and overall quality.
> 
> A HUGE problem I have with current "efforts", is that once someone
> reports a bug, the onus seems to be 99% on the *reporter* to find
> the exact line of code or commit.  Ghad what a repressive method.

99% on the reporter?  Is that why I always try to understand the
reporters problem (*provided* it's in an area I know about) and come
up with a patch to test a theory or fix the issue?

I'm _less_ inclined to provide such a "service" for lazy maintainers
who've moved off into new and wonderfully exciting technologies, to
churn out more patches for me to merge (and eventually provide a free
to them bug fixing service for.)

That's "less" inclined, not "won't".

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply

* nf_conntrack_max appears twice in /proc/sys/net
From: Chuck Ebbert @ 2007-11-13 19:46 UTC (permalink / raw)
  To: Netdev

With 2.6.23.1:

# pwd ; find . -name nf_conntrack_max
/proc/sys/net
./netfilter/nf_conntrack_max
./nf_conntrack_max

Both sysctls operate on the same underlying value, though, so no real
problem.

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Russell King @ 2007-11-13 19:46 UTC (permalink / raw)
  To: Adrian Bunk
  Cc: Mark Lord, alsa-devel, netdev, linux-pcmcia, linux-kernel,
	protasnb, David Miller, linux-ide, bugme-daemon, linux-input,
	Ingo Molnar, Andrew Morton
In-Reply-To: <20071113193035.GI4250@stusta.de>

On Tue, Nov 13, 2007 at 08:30:35PM +0100, Adrian Bunk wrote:
> There is this silly limit that noone can work more than 168 hours per 
> week on the Linux kernel, and some kernel developers seem to take the 
> liberty of spending even less time on kernel development...

That limit of 168 hours applies all around the world to everyone.
Moreover, not all kernel developers are employed to hack on the
kernel for 168 hours a week.

For me, personally, that figure is in reality about 24 hours a
week.  Yes, just 24.  The rest of the time (like *now*) is time I'm
volunteering because I happen to be reading my email...

... and happen to be wasting replying to discussions like this rather
than reading that message which has just arrived on the ARM kernel
mailing list from someone having problems using copy_from_user()
with a kernel pointer.

So, please, stop this idea that somehow kernel developers can
somehow spend infinite amounts of time solving lots and lots of
bugs.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Adrian Bunk @ 2007-11-13 20:00 UTC (permalink / raw)
  To: Mark Lord
  Cc: Ingo Molnar, Andrew Morton, David Miller, protasnb, linux-kernel,
	netdev, alsa-devel, linux-ide, linux-pcmcia, linux-input,
	bugme-daemon
In-Reply-To: <4739FA4D.1050900@rtr.ca>

On Tue, Nov 13, 2007 at 02:26:05PM -0500, Mark Lord wrote:
> Adrian Bunk wrote:
>> On Tue, Nov 13, 2007 at 01:47:10PM -0500, Mark Lord wrote:
>>> Adrian Bunk wrote:
> ..
>> Another point is that it shifts the work from the few experienced 
>> developers to the many users. Users (and voluntary testers) we have
>> many, but developer time for debugging bug reports is a quite scarce 
>> resource.
>>
>>> And when a "maintainer" is too busy to find/fix their own bugs,
>>> that could be a sign that they've bitten off too big of a chunk
>>> of the kernel, and it's time for them to distribute code maintainership.
>>
>> The problem is: Maintainers don't grow on trees.
> ..
>
> Hey, if somebody has time to break things, then they damn well ought
> to be able to make time to fix them again.  And the best developers
> here on LKML do just that (fix what they break).
>
> You broke it, you fix it.  A simple rule.
>
> Translation for the particularly daft:
>
> If you've been making significant updates to a driver/subsystem,
> and people are reporting that it is now broken for them,

What are "significant updates"?

Sometimes one person makes one small patch and this patch contains
a typo.

> then it's your job to make it right.

We have some open drivers/ata/ regressions.

I see some person named "Mark Lord" being responsible for 4 commits.

What pubishment do you plan for him if 2.6.24 ships with any libata 
regressions?

Let George W. Bush wrongly accuse him of possessing weapons of 
mass destructions and invade Canada?

> The reporters can help,
> and many may even git-bisect or send patches.  
> But you cannot *expect* or *insist* upon them doing your job.

Bullshit.

Bug fixing is not about finding someone to blame, it's about getting the 
bug fixed.

The bug reporter is the person who can reproduce the problem, and if 
it's a regression then bisecting is the natural way of getting nearer 
at getting it fixed.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Adrian Bunk @ 2007-11-13 20:04 UTC (permalink / raw)
  To: Mark Lord, alsa-devel, netdev, linux-pcmcia, linux-kernel,
	protasnb, David Miller
In-Reply-To: <20071113194649.GE1356@flint.arm.linux.org.uk>

On Tue, Nov 13, 2007 at 07:46:49PM +0000, Russell King wrote:
> On Tue, Nov 13, 2007 at 08:30:35PM +0100, Adrian Bunk wrote:
> > There is this silly limit that noone can work more than 168 hours per 
> > week on the Linux kernel, and some kernel developers seem to take the 
> > liberty of spending even less time on kernel development...
> 
> That limit of 168 hours applies all around the world to everyone.
> Moreover, not all kernel developers are employed to hack on the
> kernel for 168 hours a week.
> 
> For me, personally, that figure is in reality about 24 hours a
> week.  Yes, just 24.  The rest of the time (like *now*) is time I'm
> volunteering because I happen to be reading my email...
> 
> ... and happen to be wasting replying to discussions like this rather
> than reading that message which has just arrived on the ARM kernel
> mailing list from someone having problems using copy_from_user()
> with a kernel pointer.
> 
> So, please, stop this idea that somehow kernel developers can
> somehow spend infinite amounts of time solving lots and lots of
> bugs.

Sorry, that happens when using irony in a non-native language...

What I wanted to express:
Noone has unlimited time for kernel development.

> Russell King

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

^ permalink raw reply

* Re: [PATCH 2/2] [e1000 VLAN] Disable vlan hw accel when promiscuous mode
From: Kok, Auke @ 2007-11-13 19:59 UTC (permalink / raw)
  To: Patrick McHardy, joonwpark81
  Cc: Herbert Xu, David Miller, w, cfriesen, netdev,
	djohnson+linux-kernel, linux-kernel, e1000-devel
In-Reply-To: <4739DE29.2030507@trash.net>

Patrick McHardy wrote:
> Kok, Auke wrote:
>> Patrick McHardy wrote:
>>
>>> I already posted a patch for this, not sure what happened to it.
>>> Auke, any news on merging the secondary unicast address support?
>>
>> I dropped the ball on that one. Care to resend it and send me one for
>> e1000e as well?
> 
> Patch for e1000 attached.
> 
> Does e1000e also work with PCI cards if I add the proper IDs?
> Otherwise I could only send an untested patch.


Johnwoo,

your patch unfortunately does not apply after patrick's unicast patch,

also, ich8lan support is removed from e1000 in the e1000 version in
jgarzik/netdev-2.6 #upstream as planned (moved over to e1000e!).

Can you resend your patch so that it applies to jgarzik/netdev-2.6 #upstream with
Patrick's patch applied? That would help a lot. And possibly do the e1000e patch
as well :)

Thanks,

Auke




---
[E1000]: Secondary unicast address support

Add support for configuring secondary unicast addresses. Unicast
addresses take precendece over multicast addresses when filling
the exact address filters to avoid going to promiscous mode.
When more unicast addresses are present than filter slots,
unicast filtering is disabled and all slots can be used for
multicast addresses.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 5d2e80a9c326ca529d278da823c8e4a4da91f612
tree 97a8ac20070b101c250e79912636124167a6dd07
parent 325d22df7b19e0116aff3391d3a03f73d0634ded
author Patrick McHardy <kaber@trash.net> Tue, 13 Nov 2007 18:23:34 +0100
committer Patrick McHardy <kaber@trash.net> Tue, 13 Nov 2007 18:23:34 +0100

 drivers/net/e1000/e1000_main.c |   47 ++++++++++++++++++++++++++--------------
 1 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 72deff0..5fd5f51 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -153,7 +153,7 @@ static void e1000_clean_tx_ring(struct e1000_adapter *adapter,
                                 struct e1000_tx_ring *tx_ring);
 static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
                                 struct e1000_rx_ring *rx_ring);
-static void e1000_set_multi(struct net_device *netdev);
+static void e1000_set_rx_mode(struct net_device *netdev);
 static void e1000_update_phy_info(unsigned long data);
 static void e1000_watchdog(unsigned long data);
 static void e1000_82547_tx_fifo_stall(unsigned long data);
@@ -514,7 +514,7 @@ static void e1000_configure(struct e1000_adapter *adapter)
 	struct net_device *netdev = adapter->netdev;
 	int i;

-	e1000_set_multi(netdev);
+	e1000_set_rx_mode(netdev);

 	e1000_restore_vlan(adapter);
 	e1000_init_manageability(adapter);
@@ -926,7 +926,7 @@ e1000_probe(struct pci_dev *pdev,
 	netdev->stop = &e1000_close;
 	netdev->hard_start_xmit = &e1000_xmit_frame;
 	netdev->get_stats = &e1000_get_stats;
-	netdev->set_multicast_list = &e1000_set_multi;
+	netdev->set_rx_mode = &e1000_set_rx_mode;
 	netdev->set_mac_address = &e1000_set_mac;
 	netdev->change_mtu = &e1000_change_mtu;
 	netdev->do_ioctl = &e1000_ioctl;
@@ -2409,21 +2409,22 @@ e1000_set_mac(struct net_device *netdev, void *p)
 }

 /**
- * e1000_set_multi - Multicast and Promiscuous mode set
+ * e1000_set_rx_mode - Secondary Unicast, Multicast and Promiscuous mode set
  * @netdev: network interface device structure
  *
- * The set_multi entry point is called whenever the multicast address
- * list or the network interface flags are updated.  This routine is
- * responsible for configuring the hardware for proper multicast,
+ * The set_rx_mode entry point is called whenever the unicast or multicast
+ * address lists or the network interface flags are updated. This routine is
+ * responsible for configuring the hardware for proper unicast, multicast,
  * promiscuous mode, and all-multi behavior.
  **/

 static void
-e1000_set_multi(struct net_device *netdev)
+e1000_set_rx_mode(struct net_device *netdev)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
-	struct dev_mc_list *mc_ptr;
+	struct dev_addr_list *uc_ptr;
+	struct dev_addr_list *mc_ptr;
 	uint32_t rctl;
 	uint32_t hash_value;
 	int i, rar_entries = E1000_RAR_ENTRIES;
@@ -2446,9 +2447,16 @@ e1000_set_multi(struct net_device *netdev)
 		rctl |= (E1000_RCTL_UPE | E1000_RCTL_MPE);
 	} else if (netdev->flags & IFF_ALLMULTI) {
 		rctl |= E1000_RCTL_MPE;
-		rctl &= ~E1000_RCTL_UPE;
 	} else {
-		rctl &= ~(E1000_RCTL_UPE | E1000_RCTL_MPE);
+		rctl &= ~E1000_RCTL_MPE;
+	}
+
+	uc_ptr = NULL;
+	if (netdev->uc_count > rar_entries - 1) {
+		rctl |= E1000_RCTL_UPE;
+	} else if (!(netdev->flags & IFF_PROMISC)) {
+		rctl &= ~E1000_RCTL_UPE;
+		uc_ptr = netdev->uc_list;
 	}

 	E1000_WRITE_REG(hw, RCTL, rctl);
@@ -2458,7 +2466,10 @@ e1000_set_multi(struct net_device *netdev)
 	if (hw->mac_type == e1000_82542_rev2_0)
 		e1000_enter_82542_rst(adapter);

-	/* load the first 14 multicast address into the exact filters 1-14
+	/* load the first 14 addresses into the exact filters 1-14. Unicast
+	 * addresses take precedence to avoid disabling unicast filtering
+	 * when possible.
+	 *
 	 * RAR 0 is used for the station MAC adddress
 	 * if there are not 14 addresses, go ahead and clear the filters
 	 * -- with 82571 controllers only 0-13 entries are filled here
@@ -2466,8 +2477,11 @@ e1000_set_multi(struct net_device *netdev)
 	mc_ptr = netdev->mc_list;

 	for (i = 1; i < rar_entries; i++) {
-		if (mc_ptr) {
-			e1000_rar_set(hw, mc_ptr->dmi_addr, i);
+		if (uc_ptr) {
+			e1000_rar_set(hw, uc_ptr->da_addr, i);
+			uc_ptr = uc_ptr->next;
+		} else if (mc_ptr) {
+			e1000_rar_set(hw, mc_ptr->da_addr, i);
 			mc_ptr = mc_ptr->next;
 		} else {
 			E1000_WRITE_REG_ARRAY(hw, RA, i << 1, 0);
@@ -2476,6 +2490,7 @@ e1000_set_multi(struct net_device *netdev)
 			E1000_WRITE_FLUSH(hw);
 		}
 	}
+	WARN_ON(uc_ptr != NULL);

 	/* clear the old settings from the multicast hash table */

@@ -2487,7 +2502,7 @@ e1000_set_multi(struct net_device *netdev)
 	/* load any remaining addresses into the hash table */

 	for (; mc_ptr; mc_ptr = mc_ptr->next) {
-		hash_value = e1000_hash_mc_addr(hw, mc_ptr->dmi_addr);
+		hash_value = e1000_hash_mc_addr(hw, mc_ptr->da_addr);
 		e1000_mta_set(hw, hash_value);
 	}

@@ -5104,7 +5119,7 @@ e1000_suspend(struct pci_dev *pdev, pm_message_t state)

 	if (wufc) {
 		e1000_setup_rctl(adapter);
-		e1000_set_multi(netdev);
+		e1000_set_rx_mode(netdev);

 		/* turn on all-multi mode if wake on multicast is enabled */
 		if (wufc & E1000_WUFC_MC) {

^ permalink raw reply related

* Re: [BUG] New Kernel Bugs
From: Larry Finger @ 2007-11-13 20:07 UTC (permalink / raw)
  To: Theodore Tso, Larry Finger, Benoit Boissinot, Mark Lord,
	Ingo Molnar, Andrew Morton <ak
In-Reply-To: <20071113185547.GC25824@thunk.org>

Theodore Tso wrote:
> 
> Heh. I hadn't enabled CONFIG_BCM43XX_DEBUG myself, but I just changed
> it for my next kernel build.  This is a slightly different issue,
> which is that sometimes _DEBUG options shouldn't be turned on by
> default (because they really trash performance and bloat log size),
> and sometimes they are painless to turn on and don't cost much.
> 
> If that is the case, I'd suggest removing the option and just making
> it compiled in by default with a run-time option to enable it.

I am taking your suggestion and will produce the necessary patches for ssb, b43 and b43legacy. As
bcm43xx is likely to be removed from 2.6.25, which is the earliest such a non-bug fix patch would be
accepted, I hope that your future distribution and testing kernels will include the debug option.

Thanks,

Larry

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Adrian Bunk @ 2007-11-13 20:13 UTC (permalink / raw)
  To: David Miller, akpm, alsa-devel, netdev, linux-pcmcia,
	linux-kernel, pro
In-Reply-To: <20071113193219.GC1356@flint.arm.linux.org.uk>

On Tue, Nov 13, 2007 at 07:32:19PM +0000, Russell King wrote:
>...
> There's another issue I want to raise concerning bugzilla.  We have the
> classic case of "not enough people reading bugzilla bugs" - which is one
> of the biggest problems with bugzilla.  Virtually no one in the ARM
> community looks for ARM bugs in bugzilla.
> 
> Let's not forget that it would be a waste of time for people to manually
> check bugzilla for ARM bugs.  There's soo few people reporting ARM bugs
> into bugzilla that a weekly manual check by every maintainer would just
> return the same old boring results for months and months at a time.
>...

What about having all ARM bugs in Bugzilla by default assigned to 
linux-arm-kernel@lists.arm.linux.org.uk? [1]

> Russell King

cu
Adrian

[1] Either directly or through a pseudo address, but that's just a
    technical detail.

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed


^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Mark Lord @ 2007-11-13 20:13 UTC (permalink / raw)
  To: Adrian Bunk
  Cc: Ingo Molnar, Andrew Morton, David Miller, protasnb, linux-kernel,
	netdev, alsa-devel, linux-ide, linux-pcmcia, linux-input,
	bugme-daemon
In-Reply-To: <20071113200028.GJ4250@stusta.de>

Adrian Bunk wrote:
> On Tue, Nov 13, 2007 at 02:26:05PM -0500, Mark Lord wrote:
..
>> If you've been making significant updates to a driver/subsystem,
>> and people are reporting that it is now broken for them,
> 
> What are "significant updates"?
> 
> Sometimes one person makes one small patch and this patch contains
> a typo.
..

Then that person should double check their changes against
the problems reported, and re-convince themselves that the
breakage wasn't from those.  Simple. 

>> then it's your job to make it right.
> 
> We have some open drivers/ata/ regressions.
..

Yup, but they're more specific than just that entire subsystem,
and the maintainers are actively pursuing the problems.
Exactly what should be happening.

> I see some person named "Mark Lord" being responsible for 4 commits.
> 
> What pubishment do you plan for him if 2.6.24 ships with any libata 
> regressions?
..

If the code I'm touching breaks, then I'll fix it ASAP,
exactly what the users of that code might expect.

>> The reporters can help,
>> and many may even git-bisect or send patches.  
>> But you cannot *expect* or *insist* upon them doing your job.
> 
> Bullshit.
> 
> Bug fixing is not about finding someone to blame, it's about getting the 
> bug fixed.
..

It's not about blame, it's about paying attention to breakages in code that a
person claims to be supporting, and then doing their best to resolve the issues.

Again, if one has the time to actively write/modify code such that something breaks,
then that person should also make time to fix the breakages.

> The bug reporter is the person who can reproduce the problem, and if 
> it's a regression then bisecting is the natural way of getting nearer 
> at getting it fixed.
..
For the third time, no disagreement here.  git-bsect can help in many cases,
but not in all cases.  And it requires a great time commitment from somebody
who's system used to work and now doesn't work.  The person who broke it has
a fair bit of responsibility there, too.

cheers

^ permalink raw reply

* Re: [BUG] New Kernel Bugs
From: Mark Lord @ 2007-11-13 20:18 UTC (permalink / raw)
  To: Ingo Molnar, alsa-devel, netdev, linux-pcmcia, linux-kernel,
	protasnb, linux-ide
In-Reply-To: <20071113193750.GD1356@flint.arm.linux.org.uk>

Russell King wrote:
> On Tue, Nov 13, 2007 at 09:08:32AM -0500, Mark Lord wrote:
>> Ingo Molnar wrote:
>> ..
>>> This is all QA-101 that _cannot be argued against on a rational basis_, 
>>> it's just that these sorts of things have been largely ignored for 
>>> years, in favor of the all-too-easy "open source means many eyeballs and 
>>> that is our QA" answer, which is a _good_ answer but by far not the most 
>>> intelligent answer! Today "many eyeballs" is simply not good enough and 
>>> nature (and other OS projects) will route us around if we dont change.
>> ..
>>
>> QA-101 and "many eyeballs" are not at all in opposition.
>> The latter is how we find out about bugs on uncommon hardware,
>> and the former is what we need to track them and overall quality.
>>
>> A HUGE problem I have with current "efforts", is that once someone
>> reports a bug, the onus seems to be 99% on the *reporter* to find
>> the exact line of code or commit.  Ghad what a repressive method.
> 
> 99% on the reporter?  Is that why I always try to understand the
> reporters problem (*provided* it's in an area I know about) and come
> up with a patch to test a theory or fix the issue?
..

Same here.

I just find it weird that something can be known broken for several -rc*
kernels before I happen to install it, discover it's broken on my own machine,
and then I track it down, fix it, and submit the patch, generally all within a
couple of hours.  Where the heck was the dude(ess) that broke it ??  AWOL.

And when I receive hostility from the "maintainers" of said code for fixing
their bugs, well.. that really motivates me to continue reporting new ones..

> I'm _less_ inclined to provide such a "service" for lazy maintainers
> who've moved off into new and wonderfully exciting technologies, to
> churn out more patches for me to merge (and eventually provide a free
> to them bug fixing service for.)
> 
> That's "less" inclined, not "won't".
> 


^ permalink raw reply

* Re: [PATCH 2/2] [e1000 VLAN] Disable vlan hw accel when promiscuous mode
From: Stephen Hemminger @ 2007-11-13 20:43 UTC (permalink / raw)
  To: Joonwoo Park
  Cc: e1000-devel, netdev, 'Dave Johnson',
	'David Miller', linux-kernel
In-Reply-To: <47365200.0f10240a.0686.2173@mx.google.com>

On Sun, 11 Nov 2007 09:51:20 +0900
"Joonwoo Park" <joonwpark81@gmail.com> wrote:

> IMHO even though netdevice is in the promiscuous mode, we should receive all of ingress packets.
> This disable the vlan filtering feature when a vlan hw accel configured e1000 device goes into promiscuous mode.
> This make packets visible to sniffers though it's not vlan id of itself.
> Any check, comments will be appreciated.
> Thanks.
> 
> Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>

Promiscuous has other uses such as bridging. Would this patch break any
existing users startup scripts?

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

* Re: [BUG] New Kernel Bugs
From: Andrew Morton @ 2007-11-13 20:52 UTC (permalink / raw)
  To: Russell King
  Cc: David Miller, alsa-devel, netdev, linux-pcmcia, linux-kernel,
	protasnb, linux-ide, bugme-daemon, linux-input
In-Reply-To: <20071113193219.GC1356@flint.arm.linux.org.uk>

On Tue, 13 Nov 2007 19:32:19 +0000 Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> There's another issue I want to raise concerning bugzilla.  We have the
> classic case of "not enough people reading bugzilla bugs" - which is one
> of the biggest problems with bugzilla.  Virtually no one in the ARM
> community looks for ARM bugs in bugzilla.

Nor should they.

> Let's not forget that it would be a waste of time for people to manually
> check bugzilla for ARM bugs.  There's soo few people reporting ARM bugs
> into bugzilla that a weekly manual check by every maintainer would just
> return the same old boring results for months and months at a time.

I screen all bugzilla reports.  100% of them.

- I'll try to establish whether it is a regression

- I'll solicit any extra information which I believe the reveloper will need

- I'll ensure that an appropriate developer has seen the report

And yes, the number of arm-specific reports in there is very small.

> It would be far more productive if the ARM category was deleted from
> bugzilla and the few people who use bugzilla reported their bugs on the
> mailing list.  We've a couple of thousand people on the ARM kernel
> mailing list at the moment - that's 3 orders of magnitude more of eyes
> than look at bugzilla.

Is that linux-arm-kernel@lists.arm.linux.org.uk?

If so, MANITAINERS claims that it is subscribers-only.  That would cause
some bug reporters to give up and go away.

^ permalink raw reply

* Re: [PATCH 05/05] ipv6: RFC4214 Support (3)
From: Stephen Hemminger @ 2007-11-13 20:53 UTC (permalink / raw)
  To: osprey67; +Cc: osprey67, netdev
In-Reply-To: <4734FCEF.3080301@yahoo.com>

On Fri, 09 Nov 2007 16:35:59 -0800
osprey67 <osprey67@yahoo.com> wrote:

> From: Fred L. Templin <fred.l.templin@boeing.com>
> 
> This message attaches the combined diffs from
> messages 01/05 through 04/05. This file should be
> suitable for use with the patch utility.
> 
> Signed-off-by: Fred L. Templin <fred.l.templin@boeing.com>
> 

Isn't increasing the size of struct ip_tunnel_parm
going to cause kernel ABI changes?

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox