* Re: patch tulip-natsemi-dp83840a-phy-fix.patch added to -mm tree
From: Francois Romieu @ 2005-05-20 19:15 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Grant Grundler, akpm, T-Bone, varenet, Linux Kernel, Netdev
In-Reply-To: <428E3372.403@pobox.com>
Jeff Garzik <jgarzik@pobox.com> :
[snip]
I should start chewing it later today: it just reached the top of the
pile (right before the "see if Chelsio publish something within 5 days"
item).
--
Ueimor
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Jeff Garzik @ 2005-05-20 19:42 UTC (permalink / raw)
To: Michael Chan; +Cc: davem, netdev, ffan, lusinsky
In-Reply-To: <1116609329.31523.16.camel@rh4>
On Fri, May 20, 2005 at 10:15:29AM -0700, Michael Chan wrote:
> A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
> over 500K, I've put it on the ftp server:
>
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch
>
> The patch also includes new 1000BASE-X advertisement bit definitions in
> mii.h
Here's my in-depth review:
0) Please use 'jgarzik@pobox.com' for open source stuff, and
'jgarzik@redhat.com' only for private Red Hat (NDA'd) stuff. I know all
these email addresses can be confusing; I apologize.
1) [style] don't use braces, when a single statement follows an 'if',
'for', etc.
+ bp->duplex = DUPLEX_FULL;
+ }
+ else {
+ bp->duplex = DUPLEX_HALF;
+ }
+
+ if (!(bmcr & BMCR_ANENABLE)) {
+ return 0;
+ }
[several areas of code need this]
2) bnx2_read_phy() returns a value, but the code quite often ignores its
return value.
3) ditto for bnx2_write_phy()
4) ditto for bnx2_reset_phy()
5) ditto for bnx2_init_phy()
6) too many magic numbers in phy init. Please define constants for the
BCM-specific phy registers, and register bits, where reasonable.
+ if (bp->dev->mtu > 1500) {
+ u32 val;
+
+ /* Set extended packet length bit */
+ bnx2_write_phy(bp, 0x18, 0x7);
+ bnx2_read_phy(bp, 0x18, &val);
+ bnx2_write_phy(bp, 0x18, (val & 0xfff8) | 0x4000);
+
+ bnx2_write_phy(bp, 0x1c, 0x6c00);
+ bnx2_read_phy(bp, 0x1c, &val);
+ bnx2_write_phy(bp, 0x1c, (val & 0x3ff) | 0xec02);
+ }
+ else {
+ u32 val;
+
+ bnx2_write_phy(bp, 0x18, 0x7);
+ bnx2_read_phy(bp, 0x18, &val);
+ bnx2_write_phy(bp, 0x18, val & ~0x4007);
+
+ bnx2_write_phy(bp, 0x1c, 0x6c00);
+ bnx2_read_phy(bp, 0x1c, &val);
+ bnx2_write_phy(bp, 0x1c, (val & 0x3fd) | 0xec00);
+ }
7) the delays in bnx2_fw_sync(), bnx2_reset_chip() and bnx2_init_chip()
might work better as sleeps, rather than udelay, since you are always
in process context, and its not a performance critical path.
As a general rule for all driver programming, msleep() is preferred over
udelay() / mdelay(). udelay() spins, while msleep() allows other
[possibly high priority real-time] processes to continue.
Note that tg3 needs fixing for this too, but nobody has been motivated
enough to care.
8) excessive stack size in bnx2_alloc_bad_rbuf():
+ u16 good_mbuf[512];
a 1024-byte object is simply unacceptable, given that that is fully 1/4
of a 4K stack, for users that use the 4K stack option.
Typical solution is kmalloc'ing a temporary buffer.
9) [additional review] DaveM, others: is this correct for all arches?
+ if (unlikely((align = (unsigned long) skb->data & 0x7))) {
+ skb_reserve(skb, 8 - align);
+ }
10) [additional review] doesn't bnx2_rx_int() need to move the rmb()
inside the loop? Are you protecting against the compiler
reordering/caching loads/stores, or against SMP CPUs?
10.1) [additional review] is the rmb() even needed in bnx2_rx_int(),
since its caller also uses rmb()?
11) Don't use IRQ_RETVAL(1), use IRQ_NONE / IRQ_HANDLED for the constant
cases.
12) use a named constant rather than magic number (aka numeric constant):
+ if ((len > (bp->dev->mtu + ETH_HLEN)) &&
+ (htons(skb->protocol) != 0x8100)) {
13) [additional review] why is CHECKSUM_UNNECESSARY used when
cksum==0xffff or cksum==0 ?
+ u16 cksum = rx_hdr->l2_fhdr_tcp_udp_xsum;
+
+ if ((cksum == 0xffff) || (cksum == 0)) {
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ }
14) is D0 guaranteed after 50 usec?
+ /* No more memory access after this point until
+ * device is brought back to D0.
+ */
+ udelay(50);
+ break;
15) the following loop is just silly. use mdelay or (preferably)
msleep.
+ if ((CHIP_ID(bp) == CHIP_ID_5706_A0) ||
+ (CHIP_ID(bp) == CHIP_ID_5706_A1)) {
+
+ for (i = 0; i < 500; i++) {
+ udelay(30);
+ }
+ }
16) [long term; grunt work] it would be nice if somebody would submit
patches to ethtool, for verbose register dump of bnx2 and tg3
17) you may prefer msleep_interruptible() ?
+ for (i = 0; i < 10; i++) {
+ if ((REG_RD(bp, BNX2_PCICFG_INT_ACK_CMD) & 0xffff) !=
+ status_idx) {
+
+ break;
+ }
+
+ current->state = TASK_INTERRUPTIBLE;
+ schedule_timeout(1);
+ }
18) you can eliminate one call to request_irq, by lengthening the 'if' test:
+ if ((CHIP_ID(bp) != CHIP_ID_5706_A0) &&
+ (CHIP_ID(bp) != CHIP_ID_5706_A1) &&
+ !disable_msi) {
+
+ if (pci_enable_msi(bp->pdev) == 0) {
+ bp->flags |= USING_MSI_FLAG;
+ rc = request_irq(bp->pdev->irq, bnx2_msi, 0, dev->name,
+ dev);
+ }
+ else {
+ rc = request_irq(bp->pdev->irq, bnx2_interrupt,
+ SA_SHIRQ, dev->name, dev);
+ }
+ }
+ else {
+ rc = request_irq(bp->pdev->irq, bnx2_interrupt, SA_SHIRQ,
+ dev->name, dev);
+ }
19) [additional review] need flush_scheduled_work(), if using work queues?
20) is there any reason to #ifdef BCM_TSO? 2.4.x kernels I suppose?
21) need to call bnx2_netif_stop() in bnx2_close()
22) [namespace] s/ETH_NUM_STATS/BNX2_NUM_STATS/
23) use msleep_interruptible() in bnx2_phys_id()
+ current->state = TASK_INTERRUPTIBLE;
+ schedule_timeout(HZ / 2);
+ if (signal_pending(current))
+ break;
24) consider using pci_enable_msi() in bnx2_init_board(), rather than
the request_irq() callsite.
25) don't assign dev->mem_{start,end}, it's pointless. Some drivers
use it to pass _information_ these days, even.
26) Note that some PCI buses have speeds other than what is listed
in bnx2_init_board(). Some weirdo platforms even vary the PCI bus
speed (which is legal).
27) isn't 'timer_interval == HZ' too rapid a timer? Does it really need
to fire every second?
28) use SET_ETHTOOL_OPS(), if you are backporting to 2.4.x as item #20
indicates.
29) do not use NETIF_F_SG independent of NETIF_F_xx_CSUM.
+ dev->features |= NETIF_F_SG;
+ if (bp->flags & USING_DAC_FLAG)
+ dev->features |= NETIF_F_HIGHDMA;
+ dev->features |= NETIF_F_IP_CSUM;
30) as David mentioned in a previous email, mark everything 'static':
+int COM_b06FwReleaseMajor = 0x0;
+int COM_b06FwReleaseMinor = 0x0;
+int COM_b06FwReleaseFix = 0x0;
+u32 COM_b06FwStartAddr = 0x080004a0;
+u32 COM_b06FwTextAddr = 0x08000000;
+int COM_b06FwTextLen = 0x4594;
+u32 COM_b06FwDataAddr = 0x080045e0;
+int COM_b06FwDataLen = 0x0;
+u32 COM_b06FwRodataAddr = 0x08004598;
+int COM_b06FwRodataLen = 0x18;
+u32 COM_b06FwBssAddr = 0x08004600;
+int COM_b06FwBssLen = 0x88;
+u32 COM_b06FwSbssAddr = 0x080045e0;
+int COM_b06FwSbssLen = 0x1c;
+u32 COM_b06FwText[(0x4594/4) + 1] = {
31) use arrays, as requested in a previous email:
+ u16 status_tx_quick_consumer_index0;
+ u16 status_tx_quick_consumer_index1;
+ u16 status_tx_quick_consumer_index2;
+ u16 status_tx_quick_consumer_index3;
+ u16 status_rx_quick_consumer_index0;
+ u16 status_rx_quick_consumer_index1;
+ u16 status_rx_quick_consumer_index2;
+ u16 status_rx_quick_consumer_index3;
+ u16 status_rx_quick_consumer_index4;
+ u16 status_rx_quick_consumer_index5;
+ u16 status_rx_quick_consumer_index6;
+ u16 status_rx_quick_consumer_index7;
+ u16 status_rx_quick_consumer_index8;
+ u16 status_rx_quick_consumer_index9;
+ u16 status_rx_quick_consumer_index10;
+ u16 status_rx_quick_consumer_index11;
+ u16 status_rx_quick_consumer_index12;
+ u16 status_rx_quick_consumer_index13;
+ u16 status_rx_quick_consumer_index14;
+ u16 status_rx_quick_consumer_index15;
32) [speling] s/fileds/fields/
/* End of fileds used in the performance code paths. */
33) [ack] I ACK the changes to linux/mii.h [I am the maintainer]
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Michael Chan @ 2005-05-20 20:17 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Ben Greear, davem, netdev, ffan, lusinsky
In-Reply-To: <20050520210938.GA23714@havoc.gtf.org>
On Fri, 2005-05-20 at 17:09 -0400, Jeff Garzik wrote:
> On Fri, May 20, 2005 at 02:07:17PM -0700, Ben Greear wrote:
> > I dig having the chipset manufacturer
> > solidly behind the driver, like e1000!
>
> Clearly you have been asleep :) Broadcom has been contributing actively
> to tg3.
>
Yes, we are solidly behind tg3 and bnx2.
^ permalink raw reply
* Re: [Netem] [PATCH] (3/3) netem: allow random reordering
From: Julio Kriger @ 2005-05-20 20:28 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, netdev, netem
In-Reply-To: <20050519151254.79afe7e7@dxpl.pdx.osdl.net>
Hi!
How do I use this feature? Shouldn't tc be modified to accept this new feature?
Regards,
Julio
On 5/19/05, Stephen Hemminger <shemminger@osdl.org> wrote:
> Enhance the reorder feature of netem to allow random percent to be reordered.
> Has expected backwards compatibility behaviour.
>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
>
> Index: netem-2.6.12-rc4/include/linux/pkt_sched.h
> ===================================================================
> --- netem-2.6.12-rc4.orig/include/linux/pkt_sched.h
> +++ netem-2.6.12-rc4/include/linux/pkt_sched.h
> @@ -427,6 +427,7 @@ enum
> TCA_NETEM_UNSPEC,
> TCA_NETEM_CORR,
> TCA_NETEM_DELAY_DIST,
> + TCA_NETEM_REORDER,
> __TCA_NETEM_MAX,
> };
>
> @@ -437,7 +438,7 @@ struct tc_netem_qopt
> __u32 latency; /* added delay (us) */
> __u32 limit; /* fifo limit (packets) */
> __u32 loss; /* random packet loss (0=none ~0=100%) */
> - __u32 gap; /* re-ordering gap (0 for delay all) */
> + __u32 gap; /* re-ordering gap (0 for none) */
> __u32 duplicate; /* random packet dup (0=none ~0=100%) */
> __u32 jitter; /* random jitter in latency (us) */
> };
> @@ -449,6 +450,12 @@ struct tc_netem_corr
> __u32 dup_corr; /* duplicate correlation */
> };
>
> +struct tc_netem_reorder
> +{
> + __u32 probability;
> + __u32 correlation;
> +};
> +
> #define NETEM_DIST_SCALE 8192
>
> #endif
> Index: netem-2.6.12-rc4/net/sched/sch_netem.c
> ===================================================================
> --- netem-2.6.12-rc4.orig/net/sched/sch_netem.c
> +++ netem-2.6.12-rc4/net/sched/sch_netem.c
> @@ -62,11 +62,12 @@ struct netem_sched_data {
> u32 gap;
> u32 jitter;
> u32 duplicate;
> + u32 reorder;
>
> struct crndstate {
> unsigned long last;
> unsigned long rho;
> - } delay_cor, loss_cor, dup_cor;
> + } delay_cor, loss_cor, dup_cor, reorder_cor;
>
> struct disttable {
> u32 size;
> @@ -185,18 +186,18 @@ static int netem_enqueue(struct sk_buff
> * of the queue.
> * gap == 0 is special case for no-reordering.
> */
> - if (q->gap == 0 || q->counter != q->gap) {
> + if (q->gap == 0 && q->counter < q->gap &&
> + q->reorder < get_crandom(&q->reorder_cor)) {
> psched_time_t now;
> PSCHED_GET_TIME(now);
> - PSCHED_TADD2(now,
> - tabledist(q->latency, q->jitter, &q->delay_cor, q->delay_dist),
> + PSCHED_TADD2(now, tabledist(q->latency, q->jitter,
> + &q->delay_cor, q->delay_dist),
> cb->time_to_send);
> -
> ++q->counter;
> ret = q->qdisc->enqueue(skb, q->qdisc);
> } else {
> - q->counter = 0;
> PSCHED_GET_TIME(cb->time_to_send);
> + q->counter = 0;
> ret = q->qdisc->ops->requeue(skb, q->qdisc);
> }
>
> @@ -351,6 +352,19 @@ static int get_correlation(struct Qdisc
> return 0;
> }
>
> +static int get_reorder(struct Qdisc *sch, const struct rtattr *attr)
> +{
> + struct netem_sched_data *q = qdisc_priv(sch);
> + const struct tc_netem_reorder *r = RTA_DATA(attr);
> +
> + if (RTA_PAYLOAD(attr) != sizeof(*r))
> + return -EINVAL;
> +
> + q->reorder = r->probability;
> + init_crandom(&q->reorder_cor, r->correlation);
> + return 0;
> +}
> +
> static int netem_change(struct Qdisc *sch, struct rtattr *opt)
> {
> struct netem_sched_data *q = qdisc_priv(sch);
> @@ -371,9 +385,15 @@ static int netem_change(struct Qdisc *sc
> q->jitter = qopt->jitter;
> q->limit = qopt->limit;
> q->gap = qopt->gap;
> + q->counter = 0;
> q->loss = qopt->loss;
> q->duplicate = qopt->duplicate;
>
> + /* for compatiablity with earlier versions.
> + * if gap is set, need to assume 100% probablity
> + */
> + q->reorder = ~0;
> +
> /* Handle nested options after initial queue options.
> * Should have put all options in nested format but too late now.
> */
> @@ -395,6 +415,11 @@ static int netem_change(struct Qdisc *sc
> if (ret)
> return ret;
> }
> + if (tb[TCA_NETEM_REORDER-1]) {
> + ret = get_reorder(sch, tb[TCA_NETEM_REORDER-1]);
> + if (ret)
> + return ret;
> + }
> }
>
>
> @@ -412,7 +437,6 @@ static int netem_init(struct Qdisc *sch,
> init_timer(&q->timer);
> q->timer.function = netem_watchdog;
> q->timer.data = (unsigned long) sch;
> - q->counter = 0;
>
> q->qdisc = qdisc_create_dflt(sch->dev, &pfifo_qdisc_ops);
> if (!q->qdisc) {
> @@ -444,6 +468,7 @@ static int netem_dump(struct Qdisc *sch,
> struct rtattr *rta = (struct rtattr *) b;
> struct tc_netem_qopt qopt;
> struct tc_netem_corr cor;
> + struct tc_netem_reorder reorder;
>
> qopt.latency = q->latency;
> qopt.jitter = q->jitter;
> @@ -457,6 +482,11 @@ static int netem_dump(struct Qdisc *sch,
> cor.loss_corr = q->loss_cor.rho;
> cor.dup_corr = q->dup_cor.rho;
> RTA_PUT(skb, TCA_NETEM_CORR, sizeof(cor), &cor);
> +
> + reorder.probability = q->reorder;
> + reorder.correlation = q->reorder_cor.rho;
> + RTA_PUT(skb, TCA_NETEM_REORDER, sizeof(reorder), &reorder);
> +
> rta->rta_len = skb->tail - b;
>
> return skb->len;
>
>
> _______________________________________________
> Netem mailing list
> Netem@lists.osdl.org
> http://lists.osdl.org/mailman/listinfo/netem
>
>
>
--
----------------------------
Julio Kriger
mailto:juliokriger@gmail.com
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Jeff Garzik @ 2005-05-20 20:51 UTC (permalink / raw)
To: Michael Chan, davem; +Cc: netdev, ffan, lusinsky
In-Reply-To: <20050520194220.GA18259@havoc.gtf.org>
Note that I only consider a very few of these items, highlighted below,
to be merge-stoppers. The rest are minor things that can be fixed up at
leisure.
> 8) excessive stack size in bnx2_alloc_bad_rbuf():
> 9) [additional review] DaveM, others: is this correct for all arches?
> 13) [additional review] why is CHECKSUM_UNNECESSARY used when
> cksum==0xffff or cksum==0 ?
> 15) the following loop is just silly. use mdelay or (preferably)
> msleep.
> 19) [additional review] need flush_scheduled_work(), if using work queues?
> 21) need to call bnx2_netif_stop() in bnx2_close()
> 27) isn't 'timer_interval == HZ' too rapid a timer? Does it really need
> to fire every second?
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Ben Greear @ 2005-05-20 21:07 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Michael Chan, davem, netdev, ffan, lusinsky
In-Reply-To: <428E4DE2.1020002@pobox.com>
Jeff Garzik wrote:
>
> Note that I only consider a very few of these items, highlighted below,
> to be merge-stoppers. The rest are minor things that can be fixed up at
> leisure.
Out of curiousity: Is the intended goal to replace tg3 with
this new driver eventually? I dig having the chipset manufacturer
solidly behind the driver, like e1000!
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Jeff Garzik @ 2005-05-20 21:09 UTC (permalink / raw)
To: Ben Greear; +Cc: Michael Chan, davem, netdev, ffan, lusinsky
In-Reply-To: <428E5185.1070602@candelatech.com>
On Fri, May 20, 2005 at 02:07:17PM -0700, Ben Greear wrote:
> Jeff Garzik wrote:
> >
> >Note that I only consider a very few of these items, highlighted below,
> >to be merge-stoppers. The rest are minor things that can be fixed up at
> >leisure.
>
> Out of curiousity: Is the intended goal to replace tg3 with
> this new driver eventually?
No, different hardware.
> I dig having the chipset manufacturer
> solidly behind the driver, like e1000!
Clearly you have been asleep :) Broadcom has been contributing actively
to tg3.
Jeff
^ permalink raw reply
* Re: patch tulip-natsemi-dp83840a-phy-fix.patch added to -mm tree
From: Grant Grundler @ 2005-05-20 21:12 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Grant Grundler, akpm, T-Bone, varenet, Linux Kernel, Netdev
In-Reply-To: <428E3372.403@pobox.com>
On Fri, May 20, 2005 at 02:58:58PM -0400, Jeff Garzik wrote:
> Grant Grundler wrote:
> >After three years of using/maintaining the (trivial) tulip patch
> >in parisc-linux tree (and shipped with RH/SuSe ia64 releases),
> >I don't recall anyone complaining that udelays in tulip phy reset
> >caused them problems. Sorry, I'm unmotivated to revisit this.
> >Convince someone else to make tulip to use workqueues and I'll
> >resubmit a clean patch on top of that for the phy init sequences.
>
>
> Long delays are unacceptable in new drivers,
Agreed. But that didn't stop tg3 from having a 1.5 *SECOND*
spin delay during fiber phy init with interrupts off.
That is certainly a much newer driver than tulip.
(It's not obvious to me by code inspection which context
tg3_setup_fiber_by_hand() gets called from now.)
> and we are working to remove them from older drivers.
>
> Lack of complaints is irrelevant -- its
> a design requirement of all drivers.
It's totally relevant.
Complaints (bug reports) and patches drive change. That's how both
commercial *and* non-commercial developers prioritize.
"Ingo and the real-time crowd" are a good example of a change
in priorities driven by non-commercial users.
> Ingo and the real-time crowd are fighting against every delay, because
> every delay causes a spin, a blip in latency, an increase in CPU usage,
> and a complete stoppage of ALL work on a uniprocessor machine.
Understood. But they were not the first ones. Donald Becker has a fairly
well known digust for use of CPU spin loops. But we can't eliminate
*all* udelay just becuase of the way HW is designed and has bugs.
I think it would be reasonable to get rid of many udelay calls
(replace them with PCI read delay loops like Donald has advocated)
and get the rest into a context where it matters less.
I have no objection to people fixing those sorts of issues.
> Your patch is not a special case. We have been communicating this
> message on udelay/mdelay for -years-. All your patch [as-is] does is
> cause more work for someone else.
Not true. This patch brings the tulip driver into compliance with
published specs and makes the driver functional for parisc/mips/ia64 users.
> This also presents a problem that Andrew points out on occasion:
> what happens when a patch is useful, but the patch author isn't (for
> whatever reason) doing the legwork necessary to get it into the mainline
> kernel?
The "whatever reason" is clearly decided by the mainline kernel maintainer.
If we treat other people's labor as "free", then the right answer is
to drop the patch and wait for some subset of "we" to do the extra legwork.
Several people care if tulip phy init works right. OTOH, I'm only aware of
one person who specifically cares if tulip is holding the CPU hostage for
1 or 2 ms during the occasional phy init.
Is being a technology purist more important than moving forward with
what people care about?
> We certainly DON'T want to lose this patch, as the changes are
> useful.
I think we also agree debugging the problem and providing a patch
that works is a worthy contribution.
We won't lose it. I maintain it in CVS on cvs.parisc-linux.org.
thanks,
grant
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Jeff Garzik @ 2005-05-20 21:18 UTC (permalink / raw)
To: Michael Chan; +Cc: davem, netdev, ffan, lusinsky
In-Reply-To: <1116609329.31523.16.camel@rh4>
Michael Chan wrote:
> A new driver bnx2 for Broadcom bcm5706 is available. Since the patch is
> over 500K, I've put it on the ftp server:
>
> ftp://Net_sys_anon@ftp1.broadcom.com/bnx2-2.patch
>
> The patch also includes new 1000BASE-X advertisement bit definitions in
> mii.h
>
> Thanks to David Miller and Jeff Garzik for reviewing and their valuable
> feedback.
For what it's worth, DaveM and I decided that the patch submission
procedure will be the same for bnx2 and tg3: DaveM will merge the bnx2
driver and patches, and send them upstream.
(this deviates from the normal procedure of me merging all net driver
changes)
Jeff
^ permalink raw reply
* Re: patch tulip-natsemi-dp83840a-phy-fix.patch added to -mm tree
From: Jeff Garzik @ 2005-05-20 21:34 UTC (permalink / raw)
To: Grant Grundler; +Cc: akpm, T-Bone, varenet, Linux Kernel, Netdev
In-Reply-To: <20050520211229.GA2411@colo.lackof.org>
Grant Grundler wrote:
> On Fri, May 20, 2005 at 02:58:58PM -0400, Jeff Garzik wrote:
>
>>Grant Grundler wrote:
>>
>>>After three years of using/maintaining the (trivial) tulip patch
>>>in parisc-linux tree (and shipped with RH/SuSe ia64 releases),
>>>I don't recall anyone complaining that udelays in tulip phy reset
>>>caused them problems. Sorry, I'm unmotivated to revisit this.
>>>Convince someone else to make tulip to use workqueues and I'll
>>>resubmit a clean patch on top of that for the phy init sequences.
>>
>>
>>Long delays are unacceptable in new drivers,
>
>
> Agreed. But that didn't stop tg3 from having a 1.5 *SECOND*
> spin delay during fiber phy init with interrupts off.
> That is certainly a much newer driver than tulip.
>
> (It's not obvious to me by code inspection which context
> tg3_setup_fiber_by_hand() gets called from now.)
Yes, tg3 is awful in this regard. I have made a bit of progress by
moving some of the stuff into a workqueue.
I got multiple reports from embedded tg3 platform developers, who had to
really muck up the driver to fix the delay issue. It was apparently
_really_ noticeable on one embedded platform.
>>and we are working to remove them from older drivers.
>>
>>Lack of complaints is irrelevant -- its
>>a design requirement of all drivers.
>
>
> It's totally relevant.
> Complaints (bug reports) and patches drive change. That's how both
> commercial *and* non-commercial developers prioritize.
>
> "Ingo and the real-time crowd" are a good example of a change
> in priorities driven by non-commercial users.
No, these are commercial users. Embedded space really cares about this
stuff.
>>Ingo and the real-time crowd are fighting against every delay, because
>>every delay causes a spin, a blip in latency, an increase in CPU usage,
>>and a complete stoppage of ALL work on a uniprocessor machine.
>
>
> Understood. But they were not the first ones. Donald Becker has a fairly
> well known digust for use of CPU spin loops. But we can't eliminate
> *all* udelay just becuase of the way HW is designed and has bugs.
> I think it would be reasonable to get rid of many udelay calls
> (replace them with PCI read delay loops like Donald has advocated)
> and get the rest into a context where it matters less.
> I have no objection to people fixing those sorts of issues.
If you recognize the issue, you should object to new changes adding new
issues!
>>Your patch is not a special case. We have been communicating this
>>message on udelay/mdelay for -years-. All your patch [as-is] does is
>>cause more work for someone else.
>
>
> Not true. This patch brings the tulip driver into compliance with
> published specs and makes the driver functional for parisc/mips/ia64 users.
Ok, yes, it fixes the tulip issue AND causes work for others.
>>This also presents a problem that Andrew points out on occasion:
>>what happens when a patch is useful, but the patch author isn't (for
>>whatever reason) doing the legwork necessary to get it into the mainline
>>kernel?
>
>
> The "whatever reason" is clearly decided by the mainline kernel maintainer.
> If we treat other people's labor as "free", then the right answer is
> to drop the patch and wait for some subset of "we" to do the extra legwork.
Um, this is how all kernel development works :)
Maintainers are not supposed to merge patches into upstream, if they
have flaws still remaining to be corrected.
> Several people care if tulip phy init works right. OTOH, I'm only aware of
> one person who specifically cares if tulip is holding the CPU hostage for
> 1 or 2 ms during the occasional phy init.
>
> Is being a technology purist more important than moving forward with
> what people care about?
There will _always_ be ugly patches that get hardware going for some users.
Tons of changes are kept outside the kernel until they are ready. This
is just one more example.
Merging code into the kernel is a big deal. That code will have to be
maintained for years, maybe decades. "when in doubt, don't merge" is
generally the right answer.
I don't want your patch to become an issue that embedded developers must
address (like the tg3 example above), causing further patching and
headache in that area.
Jeff
^ permalink raw reply
* Re: Perf data with recent tg3 patches
From: Arthur Kepner @ 2005-05-20 21:52 UTC (permalink / raw)
To: David S.Miller; +Cc: mchan, netdev
In-Reply-To: <20050513.222007.78719997.davem@davemloft.net>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 4383 bytes --]
Here are a couple more data points with the recent
interrupt coalescing patches for the tg3 driver.
Please see the attached graphs, which show how CPU
utilization, and number of received packets per
interrupt vary with link utilization. The workload
here is receive-as-fast-as-you-can-over-TCP, with
a single sending and a single receiving process.
The data in the graphs is labelled as follows:
----------------------------------------------
2.6.12-rc3: unmodified 2.6.12-rc3
dflt coal : 2.6.12-rc3 + [1] + [2] + [3] + [4]
using the default intr coalescence
values (rx-frames = rx-usecs-irq = 5)
4x coal : 2.6.12-rc3 + [1] + [2] + [3] + [4] + [5]
using 4 times the default values
for rx-frames and rx-frames-irq
[1] http://marc.theaimsgroup.com/?l=linux-netdev&m=111446723510962&w=2
(This is one of a series of 3 patches - the others can't be
found in the archive. But they're all in 2.6.12-rc4.)
[2] http://marc.theaimsgroup.com/?l=linux-netdev&m=111567944730302&w=2
("tagged status" patch)
[3] http://marc.theaimsgroup.com/?l=linux-netdev&m=111586526522981&w=2
("hw coalescing infrastructure" patch)
[4] http://marc.theaimsgroup.com/?l=linux-netdev&m=111604846510646&w=2
("tagged status update")
[5] the patches below which allow "ethtool -[cC]" to work
Patch [4] almost entirely eliminates updates to the tag in
the status block between when it's been saved in tg3_poll()
and when it's written back to the NIC in tg3_restart_ints().
It still happens, but the frequency is a few times in a
thousand, so it doesn't significantly affect the interrupt
rate.
I had to make a couple of changes to allow setting/
retrieving the coalescence parameters with ethtool. Those
patches are at the end.
When the default coalescence parameters are used (rx-frames,
rx-frames-irq both set to 5) the maximum number of received
packets per interrupt is ~4.2. Setting rx-frames and
rx-frames-irq to 20 caused the maximum number of received
packets per interrupt to rise to ~19.6. Maximum CPU
utilization went down from ~52% to ~35%. Very nice.
Fix typo in ethtool_set_coalesce()
Signed-off-by: Arthur Kepner <akepner@sgi.com>
--- linux.save/net/core/ethtool.c 2005-05-20 12:40:04.426385446 -0700
+++ linux/net/core/ethtool.c 2005-05-20 12:49:34.087515306 -0700
@@ -347,7 +347,7 @@ static int ethtool_set_coalesce(struct n
{
struct ethtool_coalesce coalesce;
- if (!dev->ethtool_ops->get_coalesce)
+ if (!dev->ethtool_ops->set_coalesce)
return -EOPNOTSUPP;
if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
Changes to allow setting/getting coalescence parameters
with tg3.
Signed-off-by: Arthur Kepner <akepner@sgi.com>
--- linux.save/drivers/net/tg3.c 2005-05-20 13:02:41.610865448 -0700
+++ linux/drivers/net/tg3.c 2005-05-20 13:11:36.467011288 -0700
@@ -5094,8 +5094,11 @@ static void tg3_set_bdinfo(struct tg3 *t
}
static void __tg3_set_rx_mode(struct net_device *);
-static void tg3_set_coalesce(struct tg3 *tp, struct ethtool_coalesce *ec)
+static int tg3_set_coalesce(struct net_device *dev,
+ struct ethtool_coalesce *ec)
{
+ struct tg3 *tp = netdev_priv(dev);
+
tw32(HOSTCC_RXCOL_TICKS, ec->rx_coalesce_usecs);
tw32(HOSTCC_TXCOL_TICKS, ec->tx_coalesce_usecs);
tw32(HOSTCC_RXMAX_FRAMES, ec->rx_max_coalesced_frames);
@@ -5114,6 +5117,9 @@ static void tg3_set_coalesce(struct tg3
tw32(HOSTCC_STAT_COAL_TICKS, val);
}
+
+ memcpy(&tp->coal, ec, sizeof(tp->coal));
+ return 0;
}
/* tp->lock is held. */
@@ -5437,7 +5443,7 @@ static int tg3_reset_hw(struct tg3 *tp)
udelay(10);
}
- tg3_set_coalesce(tp, &tp->coal);
+ tg3_set_coalesce(tp->dev, &tp->coal);
/* set status block DMA address */
tw32(HOSTCC_STATUS_BLK_HOST_ADDR + TG3_64BIT_REG_HIGH,
@@ -7302,6 +7308,8 @@ static int tg3_get_coalesce(struct net_d
return 0;
}
+static int tg3_set_coalesce(struct net_device *, struct ethtool_coalesce *);
+
static struct ethtool_ops tg3_ethtool_ops = {
.get_settings = tg3_get_settings,
.set_settings = tg3_set_settings,
@@ -7335,6 +7343,7 @@ static struct ethtool_ops tg3_ethtool_op
.get_stats_count = tg3_get_stats_count,
.get_ethtool_stats = tg3_get_ethtool_stats,
.get_coalesce = tg3_get_coalesce,
+ .set_coalesce = tg3_set_coalesce,
};
static void __devinit tg3_get_eeprom_size(struct tg3 *tp)
--
Arthur
[-- Attachment #2: CPU utilization --]
[-- Type: IMAGE/png, Size: 3303 bytes --]
[-- Attachment #3: Received Pkts/Intr --]
[-- Type: IMAGE/png, Size: 3195 bytes --]
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Ben Greear @ 2005-05-20 21:58 UTC (permalink / raw)
To: Michael Chan; +Cc: Jeff Garzik, davem, netdev, ffan, lusinsky
In-Reply-To: <1116620256.31523.23.camel@rh4>
Michael Chan wrote:
> On Fri, 2005-05-20 at 17:09 -0400, Jeff Garzik wrote:
>
>>On Fri, May 20, 2005 at 02:07:17PM -0700, Ben Greear wrote:
>>
>>>I dig having the chipset manufacturer
>>>solidly behind the driver, like e1000!
>>
>>Clearly you have been asleep :) Broadcom has been contributing actively
>>to tg3.
>>
>
>
> Yes, we are solidly behind tg3 and bnx2.
Excellent...sorry for my sleepiness :)
With regard to tg3, I (with the gracious help of the Intel driver folks)
have added the ability to receive the ethernet checksum on the end of the
skb, as well as tell the e1000 chipset to receive packets with bad checksums,
etc....
Assuming the tg3 chipset can do this, are there documents or code snippets
available to explain how to enable the feature?
I would like to add generic support for this to the ethtool API and allow
drivers to support it as they wish.... Of all my hacks, this one actually
doesn't require new ioctls or /proc file API..so maybe it has a chance of
getting accepted? :)
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* [PATCH] include/linux/if_tr.h clean-up
From: Jon Mason @ 2005-05-20 22:10 UTC (permalink / raw)
To: netdev
I removed the ethernet definitions (which were commented out) and
cleaned up the tabs.
Signed-off-by: Jon Mason <jdmason@us.ibm.com>
--- include/linux/if_tr.h.orig 2005-05-20 15:24:46.723707576 -0500
+++ include/linux/if_tr.h 2005-05-20 15:31:44.657172032 -0500
@@ -9,7 +9,7 @@
*
* Author: Fred N. van Kempen, <waltje@uWalt.NL.Mugnet.ORG>
* Donald Becker, <becker@super.org>
- * Peter De Schrijver, <stud11@cc4.kuleuven.ac.be>
+ * Peter De Schrijver, <stud11@cc4.kuleuven.ac.be>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -19,24 +19,16 @@
#ifndef _LINUX_IF_TR_H
#define _LINUX_IF_TR_H
-
/* IEEE 802.5 Token-Ring magic constants. The frame sizes omit the preamble
and FCS/CRC (frame check sequence). */
-#define TR_ALEN 6 /* Octets in one ethernet addr */
-#define TR_HLEN (sizeof(struct trh_hdr)+sizeof(struct trllc))
-#define AC 0x10
-#define LLC_FRAME 0x40
-#if 0
-#define ETH_HLEN 14 /* Total octets in header. */
-#define ETH_ZLEN 60 /* Min. octets in frame sans FCS */
-#define ETH_DATA_LEN 1500 /* Max. octets in payload */
-#define ETH_FRAME_LEN 1514 /* Max. octets in frame sans FCS */
-#endif
-
+#define TR_ALEN 6 /* Octets in one token-ring addr */
+#define TR_HLEN (sizeof(struct trh_hdr)+sizeof(struct trllc))
+#define AC 0x10
+#define LLC_FRAME 0x40
/* LLC and SNAP constants */
-#define EXTENDED_SAP 0xAA
-#define UI_CMD 0x03
+#define EXTENDED_SAP 0xAA
+#define UI_CMD 0x03
/* This is an Token-Ring frame header. */
struct trh_hdr {
@@ -96,14 +88,13 @@ struct tr_statistics {
};
/* source routing stuff */
-
-#define TR_RII 0x80
-#define TR_RCF_DIR_BIT 0x80
-#define TR_RCF_LEN_MASK 0x1f00
-#define TR_RCF_BROADCAST 0x8000 /* all-routes broadcast */
-#define TR_RCF_LIMITED_BROADCAST 0xC000 /* single-route broadcast */
-#define TR_RCF_FRAME2K 0x20
-#define TR_RCF_BROADCAST_MASK 0xC000
-#define TR_MAXRIFLEN 18
+#define TR_RII 0x80
+#define TR_RCF_DIR_BIT 0x80
+#define TR_RCF_LEN_MASK 0x1f00
+#define TR_RCF_BROADCAST 0x8000 /* all-routes broadcast */
+#define TR_RCF_LIMITED_BROADCAST 0xC000 /* single-route broadcast */
+#define TR_RCF_FRAME2K 0x20
+#define TR_RCF_BROADCAST_MASK 0xC000
+#define TR_MAXRIFLEN 18
#endif /* _LINUX_IF_TR_H */
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: David S. Miller @ 2005-05-20 22:28 UTC (permalink / raw)
To: jgarzik; +Cc: mchan, netdev, ffan, lusinsky
In-Reply-To: <20050520194220.GA18259@havoc.gtf.org>
From: Jeff Garzik <jgarzik@pobox.com>
Date: Fri, 20 May 2005 15:42:20 -0400
> 9) [additional review] DaveM, others: is this correct for all arches?
>
> + if (unlikely((align = (unsigned long) skb->data & 0x7))) {
> + skb_reserve(skb, 8 - align);
> + }
It's probably not even necessary. dev_alloc_skb() should be returning
an SKB with skb->data at least cache_line_size() aligned (see mm/slab.c)
unless the platform defines an ARCH_KMALLOC_MINALIGN override.
> 10) [additional review] doesn't bnx2_rx_int() need to move the rmb()
> inside the loop? Are you protecting against the compiler
> reordering/caching loads/stores, or against SMP CPUs?
This rmb() is needed to strongly order the status block consumer
index read, from that of the descriptor data. I'm pretty sure it's
in the right spot.
> 10.1) [additional review] is the rmb() even needed in bnx2_rx_int(),
> since its caller also uses rmb()?
No, it's guarding status block consumer index read to the first
RX descriptor read, as explained above.
> 13) [additional review] why is CHECKSUM_UNNECESSARY used when
> cksum==0xffff or cksum==0 ?
>
> + u16 cksum = rx_hdr->l2_fhdr_tcp_udp_xsum;
> +
> + if ((cksum == 0xffff) || (cksum == 0)) {
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + }
For UDP, a checksum value of zero means no checksum at all.
> 18) you can eliminate one call to request_irq, by lengthening the 'if' test:
Of just assigning a function pointer and set of flags to a local variable.
Then doing on request_irq call.
> 20) is there any reason to #ifdef BCM_TSO? 2.4.x kernels I suppose?
Yeah, same for MSI stuff as well.
> 27) isn't 'timer_interval == HZ' too rapid a timer? Does it really need
> to fire every second?
The pulse has to be written the the chip at least once every 50 milliseconds,
or else the chip goes into OS absent mode. Anyways, the timer interval can
probably be extended, although link probing at 1 second intervals does seem
reasonable.
^ permalink raw reply
* Re: Perf data with recent tg3 patches
From: David S. Miller @ 2005-05-20 22:33 UTC (permalink / raw)
To: akepner; +Cc: mchan, netdev
In-Reply-To: <Pine.LNX.4.61.0505201442380.11419@linux.site>
From: Arthur Kepner <akepner@sgi.com>
Date: Fri, 20 May 2005 14:52:35 -0700 (PDT)
> When the default coalescence parameters are used (rx-frames,
> rx-frames-irq both set to 5) the maximum number of received
> packets per interrupt is ~4.2. Setting rx-frames and
> rx-frames-irq to 20 caused the maximum number of received
> packets per interrupt to rise to ~19.6. Maximum CPU
> utilization went down from ~52% to ~35%. Very nice.
Yes, but using such a high value makes latency go into the
toilet. :-)
I'd much rather see dynamic settings based upon packet rate.
It's easy to resurrect the ancient code from the early
tg3 days which does this.
Thanks for all your testing, it is very informative and
useful.
^ permalink raw reply
* Re: Perf data with recent tg3 patches
From: Rick Jones @ 2005-05-20 22:52 UTC (permalink / raw)
To: netdev
In-Reply-To: <20050520.153326.08322399.davem@davemloft.net>
> Yes, but using such a high value makes latency go into the
> toilet. :-)
For low packet rates.
>
> I'd much rather see dynamic settings based upon packet rate.
> It's easy to resurrect the ancient code from the early
> tg3 days which does this.
If that is the stuff I think it was, it was giving me _fits_ trying to run
TCP_RR tests. Results bounced all over the place. I think it was trying to
kick-in at pps rates that were below the limits of what a pair of systems could
do on a single, synchronous request/response stream.
Now, modulo an OS that I cannot mention because its EULA forbits discussing
results with third parties, where the netperf TCP_RR perf is 8000 transactions
per second no matter how powerful the CPU... if folks are simply free to set
high coalescing parms on their own, presumably with some knowledge of their
workloads, wouldn't that be enough? That has been "good enough" for one OS I
can discuss - HP-UX - and its bcm570X-based GbE NICs and before to Tigon2.
rick jones
^ permalink raw reply
* Re: Perf data with recent tg3 patches
From: Arthur Kepner @ 2005-05-20 22:54 UTC (permalink / raw)
To: David S.Miller; +Cc: mchan, netdev
In-Reply-To: <20050520.153326.08322399.davem@davemloft.net>
On Fri, 20 May 2005, David S.Miller wrote:
> .....
> Yes, but using such a high value makes latency go into the
> toilet. :-)
> .....
Latency measurements are on my to-do list.
--
Arthur
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Michael Chan @ 2005-05-20 23:04 UTC (permalink / raw)
To: David S.Miller; +Cc: jgarzik, netdev, ffan, lusinsky
In-Reply-To: <20050520.152836.48528379.davem@davemloft.net>
On Fri, 2005-05-20 at 15:28 -0700, David S.Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 20 May 2005 15:42:20 -0400
>
> > 9) [additional review] DaveM, others: is this correct for all arches?
> >
> > + if (unlikely((align = (unsigned long) skb->data & 0x7))) {
> > + skb_reserve(skb, 8 - align);
> > + }
>
> It's probably not even necessary. dev_alloc_skb() should be returning
> an SKB with skb->data at least cache_line_size() aligned (see mm/slab.c)
> unless the platform defines an ARCH_KMALLOC_MINALIGN override.
If I remember correctly, I was seeing some SKB with skb->data that is 4-
byte aligned on some Fedora kernels. I don't remember which kernel. This
device has an alignment requirement of at least 8-bytes for the receive
buffers.
>
> > 10) [additional review] doesn't bnx2_rx_int() need to move the rmb()
> > inside the loop? Are you protecting against the compiler
> > reordering/caching loads/stores, or against SMP CPUs?
>
> This rmb() is needed to strongly order the status block consumer
> index read, from that of the descriptor data. I'm pretty sure it's
> in the right spot.
>
> > 10.1) [additional review] is the rmb() even needed in bnx2_rx_int(),
> > since its caller also uses rmb()?
>
> No, it's guarding status block consumer index read to the first
> RX descriptor read, as explained above.
That's right. We saw rare failures in Anton's PPC environment that
caused the driver to read stale rx buffer descriptors ahead of the rx
index in the status block without the rmb().
>
> > 13) [additional review] why is CHECKSUM_UNNECESSARY used when
> > cksum==0xffff or cksum==0 ?
> >
> > + u16 cksum = rx_hdr->l2_fhdr_tcp_udp_xsum;
> > +
> > + if ((cksum == 0xffff) || (cksum == 0)) {
> > + skb->ip_summed = CHECKSUM_UNNECESSARY;
> > + }
>
> For UDP, a checksum value of zero means no checksum at all.
Yes, but in this case, cksum is the checksum calculated over the entire
TCP/UDP packet including the pseudo IP header. Jeff is right, it should
always be 0xffff when the checksum is correct. Checking for zero is a
bug.
>
> > 18) you can eliminate one call to request_irq, by lengthening the 'if' test:
>
> Of just assigning a function pointer and set of flags to a local variable.
> Then doing on request_irq call.
>
> > 20) is there any reason to #ifdef BCM_TSO? 2.4.x kernels I suppose?
>
> Yeah, same for MSI stuff as well.
>
> > 27) isn't 'timer_interval == HZ' too rapid a timer? Does it really need
> > to fire every second?
>
> The pulse has to be written the the chip at least once every 50 milliseconds,
> or else the chip goes into OS absent mode. Anyways, the timer interval can
> probably be extended, although link probing at 1 second intervals does seem
> reasonable.
>
>
Originally, the driver pulse interval was set at 250 msec, but it's been
extended to a few seconds. So the driver currently will write the pulse
every second and do the serdes related checking at the same time.
David, Do you want me to fix some of these things and send you a new
500K patch or just send incremental patches over the existing driver?
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Michael Chan @ 2005-05-20 23:11 UTC (permalink / raw)
To: Jeff Garzik; +Cc: David S.Miller, netdev, ffan, lusinsky
In-Reply-To: <428E7A53.1030907@pobox.com>
On Fri, 2005-05-20 at 20:01 -0400, Jeff Garzik wrote:
> David S.Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Fri, 20 May 2005 19:30:01 -0400
> >
> >
> >>Sure. What I'm driving at is that a checksum of zero seems to imply
> >>CHECKSUM_NONE not CHECKSUM_UNNECESSARY. tg3 only does the 0xffff check.
> >
> >
> > Sure, both ways are fine.
>
> huh? They are pretty different... one says "Checksum all good, dude"
> and the other says "I didn't checksum, do it in software for me."
>
> right?
>
Yes, if the UDP checksum field in the UDP header is zero - meaning
checksum is not calculated for this packet, the calculated checksum done
by the chip will almost always be something other than 0xffff, and so it
will end up with CHECKSUM_NONE.
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Jeff Garzik @ 2005-05-20 23:30 UTC (permalink / raw)
To: David S.Miller; +Cc: mchan, netdev, ffan, lusinsky
In-Reply-To: <20050520.152836.48528379.davem@davemloft.net>
David S.Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 20 May 2005 15:42:20 -0400
>>10) [additional review] doesn't bnx2_rx_int() need to move the rmb()
>>inside the loop? Are you protecting against the compiler
>>reordering/caching loads/stores, or against SMP CPUs?
>
>
> This rmb() is needed to strongly order the status block consumer
> index read, from that of the descriptor data. I'm pretty sure it's
> in the right spot.
>
>
>>10.1) [additional review] is the rmb() even needed in bnx2_rx_int(),
>>since its caller also uses rmb()?
>
>
> No, it's guarding status block consumer index read to the first
> RX descriptor read, as explained above.
OK
>>13) [additional review] why is CHECKSUM_UNNECESSARY used when
>>cksum==0xffff or cksum==0 ?
>>
>>+ u16 cksum = rx_hdr->l2_fhdr_tcp_udp_xsum;
>>+
>>+ if ((cksum == 0xffff) || (cksum == 0)) {
>>+ skb->ip_summed = CHECKSUM_UNNECESSARY;
>>+ }
>
>
> For UDP, a checksum value of zero means no checksum at all.
Sure. What I'm driving at is that a checksum of zero seems to imply
CHECKSUM_NONE not CHECKSUM_UNNECESSARY. tg3 only does the 0xffff check.
I am also a bit surprised that, if the actual checksum value is
available, why not use CHECKSUM_HW like sunhme?
Jeff
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: David S. Miller @ 2005-05-20 23:45 UTC (permalink / raw)
To: jgarzik; +Cc: mchan, netdev, ffan, lusinsky
In-Reply-To: <428E72F9.3070404@pobox.com>
From: Jeff Garzik <jgarzik@pobox.com>
Date: Fri, 20 May 2005 19:30:01 -0400
> Sure. What I'm driving at is that a checksum of zero seems to imply
> CHECKSUM_NONE not CHECKSUM_UNNECESSARY. tg3 only does the 0xffff check.
Sure, both ways are fine.
> I am also a bit surprised that, if the actual checksum value is
> available, why not use CHECKSUM_HW like sunhme?
CHECKSUM_HW is for a different calculation than what 5706 and tg3 are
providing here. CHECKSUM_HW is for when the chip provides a raw 2's
complement 16-bit sum starting at a fixed offset from the beginning of
the packet. The network stack then "undoes" or subtracts the header
2's complement checksum from the device provided sum to arrive at the
real checksum result.
5706 and tg3 are actually interpreting the headers and running the
checksum algorithm over the proper parts of the packet headers.
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: Jeff Garzik @ 2005-05-21 0:01 UTC (permalink / raw)
To: David S.Miller; +Cc: mchan, netdev, ffan, lusinsky
In-Reply-To: <20050520.164504.31639000.davem@davemloft.net>
David S.Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Fri, 20 May 2005 19:30:01 -0400
>
>
>>Sure. What I'm driving at is that a checksum of zero seems to imply
>>CHECKSUM_NONE not CHECKSUM_UNNECESSARY. tg3 only does the 0xffff check.
>
>
> Sure, both ways are fine.
huh? They are pretty different... one says "Checksum all good, dude"
and the other says "I didn't checksum, do it in software for me."
right?
Jeff
^ permalink raw reply
* [PATCH] tg3: new ID
From: Xose Vazquez Perez @ 2005-05-21 0:11 UTC (permalink / raw)
To: netdev, davem, jgarzik, mchan
[-- Attachment #1: Type: text/plain, Size: 187 bytes --]
Add 0x1601 as 5752M, it's a 5752 but for mobile PCs.
Stolen from Broadcom bcm5700-8.1.55 driver.
Someone forgot to add it to tg3 ;-)
(against linux-2.6.12-rc4-git4)
-thanks-
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tg3_5752M_id.diff --]
[-- Type: text/x-patch; name="tg3_5752M_id.diff", Size: 1047 bytes --]
diff -Nuar o/drivers/net/tg3.c n/drivers/net/tg3.c
--- o/drivers/net/tg3.c 2005-05-21 01:50:40.000000000 +0200
+++ n/drivers/net/tg3.c 2005-05-21 01:42:29.000000000 +0200
@@ -206,6 +206,8 @@
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5752,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
+ { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5752M,
+ PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5753,
PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5753M,
diff -Nuar o/include/linux/pci_ids.h n/include/linux/pci_ids.h
--- o/include/linux/pci_ids.h 2005-05-21 01:50:34.000000000 +0200
+++ n/include/linux/pci_ids.h 2005-05-21 01:50:06.000000000 +0200
@@ -2064,6 +2064,7 @@
#define PCI_VENDOR_ID_BROADCOM 0x14e4
#define PCI_DEVICE_ID_TIGON3_5752 0x1600
+#define PCI_DEVICE_ID_TIGON3_5752M 0x1601
#define PCI_DEVICE_ID_TIGON3_5700 0x1644
#define PCI_DEVICE_ID_TIGON3_5701 0x1645
#define PCI_DEVICE_ID_TIGON3_5702 0x1646
^ permalink raw reply
* Re: patch tulip-natsemi-dp83840a-phy-fix.patch added to -mm tree
From: Grant Grundler @ 2005-05-21 0:51 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Grant Grundler, akpm, T-Bone, varenet, Linux Kernel, Netdev
In-Reply-To: <428E57E2.2090204@pobox.com>
On Fri, May 20, 2005 at 05:34:26PM -0400, Jeff Garzik wrote:
> Yes, tg3 is awful in this regard. I have made a bit of progress by
> moving some of the stuff into a workqueue.
ah good! At some point I should re-install the fiber bcm5701 cards
I have and see if they work better then.
> >It's totally relevant.
> >Complaints (bug reports) and patches drive change. That's how both
> >commercial *and* non-commercial developers prioritize.
> >
> >"Ingo and the real-time crowd" are a good example of a change
> >in priorities driven by non-commercial users.
>
> No, these are commercial users. Embedded space really cares about this
> stuff.
Sorry - maybe VM support for VIVT cache is a better example?
(I'm thinking sparc/parisc support)
Almost everything I think of has some form of commercial interest
either directly (embedded, HA, infiniband, Server IO, ia64/amd64, etc)
or indirectly (sponsorships at Universities or sourceforge projects).
> >Understood. But they were not the first ones. Donald Becker has a fairly
> >well known digust for use of CPU spin loops. But we can't eliminate
> >*all* udelay just becuase of the way HW is designed and has bugs.
> >I think it would be reasonable to get rid of many udelay calls
> >(replace them with PCI read delay loops like Donald has advocated)
> >and get the rest into a context where it matters less.
> >I have no objection to people fixing those sorts of issues.
>
> If you recognize the issue, you should object to new changes adding new
> issues!
And ignore fixes for existing issues.
"The enemy of good is perfect"
...
> >Not true. This patch brings the tulip driver into compliance with
> >published specs and makes the driver functional for parisc/mips/ia64 users.
>
> Ok, yes, it fixes the tulip issue AND causes work for others.
But it's not new work.
In timer.c, tulip_restart_rxtx() is called right after tulip_select_media().
tulip_restart_rxtx() has a potential 1300 usec delay loop. About
the same as the code in tulip_select_media() needs.
I now appreciate much more that a RH employee (IIRC, John Linville)
submitted that patch indirectly on my behalf. I filed the original
bugzilla and provided the patch based on work by Charlie Brett.
> >The "whatever reason" is clearly decided by the mainline kernel maintainer.
> >If we treat other people's labor as "free", then the right answer is
> >to drop the patch and wait for some subset of "we" to do the extra legwork.
>
> Um, this is how all kernel development works :)
*nod*
> Maintainers are not supposed to merge patches into upstream, if they
> have flaws still remaining to be corrected.
Many patches are not this black and white.
All patches have to survive at least one subjective evaluation:
Is the cure worse than the illness?
> >Several people care if tulip phy init works right. OTOH, I'm only aware of
> >one person who specifically cares if tulip is holding the CPU hostage for
> >1 or 2 ms during the occasional phy init.
> >
> >Is being a technology purist more important than moving forward with
> >what people care about?
>
> There will _always_ be ugly patches that get hardware going for some users.
"ugly" is subjective. Especially in the context of tulip phy init sequence.
Anyone who finds beauty in that chunk of code is truly twisted. :^)
(It just reflects the ugly mess of HW that too many vendors shipped.
I don't ever expect tulip driver to be "clean" here.)
> Tons of changes are kept outside the kernel until they are ready. This
> is just one more example.
>
> Merging code into the kernel is a big deal. That code will have to be
> maintained for years, maybe decades. "when in doubt, don't merge" is
> generally the right answer.
Agreed.
> I don't want your patch to become an issue that embedded developers must
> address (like the tg3 example above), causing further patching and
> headache in that area.
Certainly. No problem with that.
BTW, which embedded developers still care about tulip driver?
One could use this patch as a litmus test to see how much they care. :^)
I was expecting they'd be using gigabit NICs, Wi-Fi, GSM,
or bluetooth these days.
thanks,
grant
^ permalink raw reply
* Re: A new driver for Broadcom bcm5706
From: David S. Miller @ 2005-05-21 4:28 UTC (permalink / raw)
To: jgarzik; +Cc: mchan, netdev, ffan, lusinsky
In-Reply-To: <428E7A53.1030907@pobox.com>
From: Jeff Garzik <jgarzik@pobox.com>
Subject: Re: A new driver for Broadcom bcm5706
Date: Fri, 20 May 2005 20:01:23 -0400
> David S.Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Fri, 20 May 2005 19:30:01 -0400
> >
> >
> >>Sure. What I'm driving at is that a checksum of zero seems to imply
> >>CHECKSUM_NONE not CHECKSUM_UNNECESSARY. tg3 only does the 0xffff check.
> >
> >
> > Sure, both ways are fine.
>
> huh? They are pretty different... one says "Checksum all good, dude"
> and the other says "I didn't checksum, do it in software for me."
>
> right?
0x0000 is the UDP "no checksum" case, so if we say "in software"
for that UDP will just let it pass through still, so the effect
is that same.
^ 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;
as well as URLs for NNTP newsgroup(s).