* Re: [PATCH] iproute2: enable setting rto_min, rtt and rttvar in tc-esque units
From: Stephen Hemminger @ 2007-09-19 21:51 UTC (permalink / raw)
To: Rick Jones; +Cc: netdev
In-Reply-To: <200709182015.NAA25590@tardy.cup.hp.com>
On Tue, 18 Sep 2007 13:15:26 -0700 (PDT)
Rick Jones <rick.jones2@hp.com> wrote:
> Enable users of ip to specify the times for rtt, rttvar and rto_min
> in human-friendly terms a la "tc" while maintaining backwards
> compatability with the previous "raw" mechanism. Builds upon
> David Miller's uncommited patch to set rto_min.
>
> Signed-off-by: Rick Jones <rick.jones2@hp.com>
Thanks, I'll put it in the 2.6.24 version of iproute2 (assuming dave
takes the rto_min patch).
^ permalink raw reply
* net-2.6.24 - build failure
From: Joe Perches @ 2007-09-19 21:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev
allyesconfig
CC drivers/net/wireless/p54pci.o
LD drivers/net/wireless/built-in.o
drivers/net/wireless/b43legacy/built-in.o: In function `tsf_read_file':
drivers/net/wireless/b43legacy/debugfs.c:80: multiple definition of `tsf_read_file'
drivers/net/wireless/b43/built-in.o:net-2.6.24/drivers/net/wireless/b43/debugfs.c:80: first defined here
drivers/net/wireless/b43legacy/built-in.o: In function `ucode_regs_read_file':
net-2.6.24/drivers/net/wireless/b43legacy/debugfs.c:106: multiple definition of `ucode_regs_read_file'
drivers/net/wireless/b43/built-in.o:net-2.6.24/drivers/net/wireless/b43/debugfs.c:106: first defined here
drivers/net/wireless/b43legacy/built-in.o: In function `restart_write_file':
net-2.6.24/drivers/net/wireless/b43legacy/debugfs.c:187: multiple definition of `restart_write_file'
drivers/net/wireless/b43/built-in.o:net-2.6.24/drivers/net/wireless/b43/debugfs.c:266: first defined here
drivers/net/wireless/b43legacy/built-in.o: In function `tsf_write_file':
net-2.6.24/drivers/net/wireless/b43legacy/debugfs.c:94: multiple definition of `tsf_write_file'
drivers/net/wireless/b43/built-in.o:net-2.6.24/drivers/net/wireless/b43/debugfs.c:94: first defined here
drivers/net/wireless/b43legacy/built-in.o: In function `txstat_read_file':
net-2.6.24/drivers/net/wireless/b43legacy/debugfs.c:139: multiple definition of `txstat_read_file'
drivers/net/wireless/b43/built-in.o:net-2.6.24/drivers/net/wireless/b43/debugfs.c:139: first defined here
drivers/net/wireless/b43legacy/built-in.o: In function `ssb_write32':
net-2.6.24/include/linux/ssb/ssb.h:385: multiple definition of `rootdir'
drivers/net/wireless/b43/built-in.o:net-2.6.24/include/linux/ssb/ssb.h:385: first defined here
drivers/net/wireless/b43legacy/built-in.o: In function `shm_read_file':
net-2.6.24/drivers/net/wireless/b43legacy/debugfs.c:120: multiple definition of `shm_read_file'
drivers/net/wireless/b43/built-in.o:net-2.6.24/drivers/net/wireless/b43/debugfs.c:120: first defined here
make[3]: *** [drivers/net/wireless/built-in.o] Error 1
make[2]: *** [drivers/net/wireless] Error 2
make[1]: *** [drivers/net] Error 2
make: *** [drivers] Error 2
^ permalink raw reply
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: Stephen Hemminger @ 2007-09-19 21:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Ariel.Hendel, greg.onufer, jeff
In-Reply-To: <20070918.151528.84360712.davem@davemloft.net>
O
> +#define DRV_MODULE_NAME "niu"
> +#define PFX DRV_MODULE_NAME ": "
> +#define DRV_MODULE_VERSION "0.06"
> +#define DRV_MODULE_RELDATE "September 18, 2007"
> +
> +static char version[] __devinitdata =
> + DRV_MODULE_NAME ".c:v" DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
> +
> +MODULE_AUTHOR("David S. Miller (davem@davemloft.net)");
> +MODULE_DESCRIPTION("NIU ethernet driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_MODULE_VERSION);
> +
> +#ifndef DMA_44BIT_MASK
> +#define DMA_44BIT_MASK 0x00000fffffffffffULL
> +#endif
> +
> +#ifndef PCI_DEVICE_ID_SUN_NEPTUNE
> +#define PCI_DEVICE_ID_SUN_NEPTUNE 0xabcd
> +#endif
Why bother defining the ID, and what good does driver_data do you?
> +static struct pci_device_id niu_pci_tbl[] = {
> + {PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_NEPTUNE),
> + .driver_data = 0xff},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, niu_pci_tbl);
> +
> +#define NIU_TX_TIMEOUT (5 * HZ)
> +
> +#define nr64(reg) readq(np->regs + (reg))
> +#define nw64(val, reg) writeq((val), np->regs + (reg))
Macro's that make assumptions about context (ie variable name np)
are evil and bad style.
> +#define nr64_mac(reg) readq(np->mac_regs + (reg))
> +#define nw64_mac(val, reg) writeq((val), np->mac_regs + (reg))
> +
> +#define nr64_ipp(reg) readq(np->regs + np->ipp_off + (reg))
> +#define nw64_ipp(val, reg) writeq((val), np->regs + np->ipp_off + (reg))
> +
> +#define nr64_pcs(reg) readq(np->regs + np->pcs_off + (reg))
> +#define nw64_pcs(val, reg) writeq((val), np->regs + np->pcs_off + (reg))
> +
> +#define nr64_xpcs(reg) readq(np->regs + np->xpcs_off + (reg))
> +#define nw64_xpcs(val, reg) writeq((val), np->regs + np->xpcs_off + (reg))
> +
> +static unsigned int niu_debug;
> +#define NIU_DEBUG_INTERRUPT 0x00000001
> +#define NIU_DEBUG_TX_WORK 0x00000002
> +#define NIU_DEBUG_RX_WORK 0x00000004
> +#define NIU_DEBUG_POLL 0x00000008
> +#define NIU_DEBUG_PROBE 0x00010000
> +#define NIU_DEBUG_MDIO 0x00020000
> +#define NIU_DEBUG_MII 0x00040000
> +#define NIU_DEBUG_INIT_HW 0x00080000
> +#define NIU_DEBUG_STOP_HW 0x00080000
Please use or extend already existing netif_msg debug methods.
> +module_param(niu_debug, int, 0);
> +MODULE_PARM_DESC(niu_debug,
> +"NIU bitmapped debugging message enable value:\n"
> +" 0x00000001 Log interrupt events\n"
> +" 0x00000002 Log TX work\n"
> +" 0x00000004 Log RX work\n"
> +" 0x00000008 Log NAPI poll\n"
> +" 0x00010000 Log device probe events\n"
> +" 0x00020000 Log MDIO reads and writes\n"
> +" 0x00040000 Log MII reads and writes\n"
> +" 0x00080000 Log HW initialization\n"
> +" 0x00100000 Log HW shutdown\n"
> +);
> +
> +#define niudbg(TYPE, f, a...) \
> +do { if (niu_debug & NIU_DEBUG_##TYPE) \
> + printk(KERN_ERR PFX f, ## a); \
> +} while (0)
> +
> +#define niu_lock_parent(np, flags) \
> + spin_lock_irqsave(&np->parent->lock, flags)
> +#define niu_unlock_parent(np, flags) \
> + spin_unlock_irqrestore(&np->parent->lock, flags)
>
Wrapping locking is poor style and makes code review harder.
> +static int niu_wait_bits_clear_mac(struct niu *np, unsigned long reg, u64 bits,
> + int limit, int delay)
> +{
> + BUILD_BUG_ON(limit <= 0 || delay < 0);
There is no way compiler can evaluate limit or delay.
> + while (--limit >= 0) {
> + u64 val = nr64_mac(reg);
> +
> + if (!(val & bits))
> + break;
> + udelay(delay);
> + }
> + if (limit < 0)
> + return -ENODEV;
> + return 0;
> +}
> +
> +static int niu_set_and_wait_clear_mac(struct niu *np, unsigned long reg,
> + u64 bits, int limit, int delay,
> + const char *reg_name)
> +{
> + int err;
> +
> + nw64_mac(bits, reg);
> + err = niu_wait_bits_clear_mac(np, reg, bits, limit, delay);
> + if (err)
> + printk(KERN_ERR PFX "%s: bits (%lx) of register %s "
> + "would not clear, val[%lx]\n",
> + np->dev->name, bits, reg_name,
> + nr64_mac(reg));
> + return err;
> +}
> +
> +static int niu_wait_bits_clear_ipp(struct niu *np, unsigned long reg, u64 bits,
> + int limit, int delay)
> +{
> + BUILD_BUG_ON(limit <= 0 || delay < 0);
> + while (--limit >= 0) {
> + u64 val = nr64_ipp(reg);
> +
> + if (!(val & bits))
> + break;
> + udelay(delay);
> + }
> + if (limit < 0)
> + return -ENODEV;
> + return 0;
> +}
> +
> +static int niu_set_and_wait_clear_ipp(struct niu *np, unsigned long reg,
> + u64 bits, int limit, int delay,
> + const char *reg_name)
> +{
> + int err;
> + u64 val;
> +
> + val = nr64_ipp(reg);
> + val |= bits;
> + nw64_ipp(val, reg);
> +
> + err = niu_wait_bits_clear_ipp(np, reg, bits, limit, delay);
> + if (err)
> + printk(KERN_ERR PFX "%s: bits (%lx) of register %s "
> + "would not clear, val[%lx]\n",
> + np->dev->name, bits, reg_name,
> + nr64_ipp(reg));
> + return err;
> +}
> +
> +static int niu_wait_bits_clear(struct niu *np, unsigned long reg, u64 bits,
> + int limit, int delay)
> +{
> + BUILD_BUG_ON(limit <= 0 || delay < 0);
> + while (--limit >= 0) {
> + u64 val = nr64(reg);
> +
> + if (!(val & bits))
> + break;
> + udelay(delay);
> + }
> + if (limit < 0)
> + return -ENODEV;
> + return 0;
> +}
> +
> +static int niu_set_and_wait_clear(struct niu *np, unsigned long reg,
> + u64 bits, int limit, int delay,
> + const char *reg_name)
> +{
> + int err;
> +
> + nw64(bits, reg);
> + err = niu_wait_bits_clear(np, reg, bits, limit, delay);
> + if (err)
> + printk(KERN_ERR PFX "%s: bits (%lx) of register %s "
> + "would not clear, val[%lx]\n",
> + np->dev->name, bits, reg_name,
> + nr64(reg));
> + return err;
> +}
> +
> +static void niu_ldg_rearm(struct niu *np, struct niu_ldg *lp, int on)
> +{
> + u64 val = (u64) lp->timer;
> +
> + if (on)
> + val |= LDG_IMGMT_ARM;
> +
> + nw64(val, LDG_IMGMT(lp->ldg_num));
> +}
> +
> +static int niu_ldn_irq_enable(struct niu *np, int ldn, int on)
> +{
> + unsigned long mask_reg, bits;
> + u64 val;
> +
> + if (ldn < 0 || ldn > LDN_MAX)
> + return -EINVAL;
> +
> + if (ldn < 64) {
> + mask_reg = LD_IM0(ldn);
> + bits = LD_IM0_MASK;
> + } else {
> + mask_reg = LD_IM1(ldn - 64);
> + bits = LD_IM1_MASK;
> + }
> +
> + val = nr64(mask_reg);
> + if (on)
> + val &= ~bits;
> + else
> + val |= bits;
> + nw64(val, mask_reg);
> +
> + return 0;
> +}
> +
> +static int niu_enable_ldn_in_ldg(struct niu *np, struct niu_ldg *lp, int on)
> +{
> + struct niu_parent *parent = np->parent;
> + int i;
> +
> + for (i = 0; i <= LDN_MAX; i++) {
> + int err;
> +
> + if (parent->ldg_map[i] != lp->ldg_num)
> + continue;
> +
> + err = niu_ldn_irq_enable(np, i, on);
> + if (err)
> + return err;
> + }
> + return 0;
> +}
> +
> +static int niu_enable_interrupts(struct niu *np, int on)
> +{
> + int i;
> +
> + for (i = 0; i < np->num_ldg; i++) {
> + struct niu_ldg *lp = &np->ldg[i];
> + int err;
> +
> + err = niu_enable_ldn_in_ldg(np, lp, on);
> + if (err)
> + return err;
> + }
> + for (i = 0; i < np->num_ldg; i++)
> + niu_ldg_rearm(np, &np->ldg[i], on);
> +
> + return 0;
> +}
> +
> +static __u32 phy_encode(__u32 type, int port)
> +{
> + return (type << (port * 2));
> +}
> +
> +static __u32 phy_decode(__u32 val, int port)
> +{
> + return (val >> (port * 2)) & PORT_TYPE_MASK;
> +}
Why are you using __u32? That is reserved for values going out
to user space.
^ permalink raw reply
* Re: Please pull 'p54' branch of wireless-2.6
From: Jeff Garzik @ 2007-09-19 22:01 UTC (permalink / raw)
To: John W. Linville
Cc: David Miller, hch-wEGCiKHe2LqWVfeAwA7xHQ,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
flamingice-R9e9/4HEdknk1uMJSBkQmQ
In-Reply-To: <20070919212607.GA3036-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
John W. Linville wrote:
> On Wed, Sep 19, 2007 at 02:12:00PM -0700, David Miller wrote:
>> From: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
>> Date: Wed, 19 Sep 2007 22:04:05 +0100
>>
>>> PLease put it into a separate subdirectory, thanks.
>>>
>>> Also net2280.h seems rather oddly named.
>> The next time I rebase I'll try to take care of this for both
>> this driver and the iwlwifi stuff.
>
> Fine by me, although I swear I remember complaints about adding separate
> directories for drivers...*shrug*...
There were complaints about the nonsensical wireless/mac80211 directory.
But if there are enough files, a driver should definitely get its own
directory. Like life, it's a balance. If it's just one or two C files,
a directory might be overkill. If it's more, it definitely warrants a
sub-dir.
Jeff
^ permalink raw reply
* Re: new NAPI quota synchronization issue
From: Stephen Hemminger @ 2007-09-19 22:01 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20070919.103508.59469052.davem@davemloft.net>
On Wed, 19 Sep 2007 10:35:08 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 19 Sep 2007 09:58:25 -0700 (PDT)
>
> > Probably a good way to deal with this is to simply make the quota
> > handling stateless.
> >
> > The only reason we handle partial quota usage is to handle the
> > global netdev_budget. But we can simply "round up" and let
> > netdev_budget get oversubscribed by one napi->quota's worth
> > if necessary.
> >
> > At that point, n->quota only holds two states when used, full
> > and empty. And at that point there is no reason to maintain
> > it's value at all. Only the weight is necessary.
> >
> > I'll try to post a patch which implements this later today.
>
> Ok, here is the patch and I've checked it into net-2.6.24 as well.
>
> There really shouldn't be any fundamental synchronization issues
> in the new NAPI stuff any longer. I'm pretty sure any problems
> remaining can only be caused by drivers bugs but we'll see :-)
>
> I went over the list handling several times and it looks bulletproof.
> Only the thread of control that sets the NAPI_STATE_SCHED bit
> atomically will do list modifications, until the thread of control
> that decides unconditionally to clear the bit, which will do a
> list_del() immediately before clearing that bit.
>
>
There might be a future problem if some driver decided to change weight
often on the fly? Perhaps you need to sample it once in start of napi_schedule.
^ permalink raw reply
* Re: [PATCH] iproute2: enable setting rto_min, rtt and rttvar in tc-esque units
From: David Miller @ 2007-09-19 22:03 UTC (permalink / raw)
To: shemminger; +Cc: rick.jones2, netdev
In-Reply-To: <20070919145133.7aff0a5b@freepuppy.rosehill.hemminger.net>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 19 Sep 2007 14:51:33 -0700
> On Tue, 18 Sep 2007 13:15:26 -0700 (PDT)
> Rick Jones <rick.jones2@hp.com> wrote:
>
> > Enable users of ip to specify the times for rtt, rttvar and rto_min
> > in human-friendly terms a la "tc" while maintaining backwards
> > compatability with the previous "raw" mechanism. Builds upon
> > David Miller's uncommited patch to set rto_min.
> >
> > Signed-off-by: Rick Jones <rick.jones2@hp.com>
>
> Thanks, I'll put it in the 2.6.24 version of iproute2 (assuming dave
> takes the rto_min patch).
If you're talking about the kernel side, Linus's tree already
has the per-route rto_min stuff.
^ permalink raw reply
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: Jeff Garzik @ 2007-09-19 22:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev, Ariel.Hendel, greg.onufer
In-Reply-To: <20070919145900.759ef19e@freepuppy.rosehill.hemminger.net>
Stephen Hemminger wrote:
> O
>> +#define DRV_MODULE_NAME "niu"
>> +#define PFX DRV_MODULE_NAME ": "
>> +#define DRV_MODULE_VERSION "0.06"
>> +#define DRV_MODULE_RELDATE "September 18, 2007"
>> +
>> +static char version[] __devinitdata =
>> + DRV_MODULE_NAME ".c:v" DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
>> +
>> +MODULE_AUTHOR("David S. Miller (davem@davemloft.net)");
>> +MODULE_DESCRIPTION("NIU ethernet driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION(DRV_MODULE_VERSION);
>> +
>> +#ifndef DMA_44BIT_MASK
>> +#define DMA_44BIT_MASK 0x00000fffffffffffULL
>> +#endif
>> +
>> +#ifndef PCI_DEVICE_ID_SUN_NEPTUNE
>> +#define PCI_DEVICE_ID_SUN_NEPTUNE 0xabcd
>> +#endif
>
> Why bother defining the ID
Yes, its pointless to use anything but the hex number for PCI IDs that
are only used in a single place.
>> +static struct pci_device_id niu_pci_tbl[] = {
>> + {PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_NEPTUNE),
>> + .driver_data = 0xff},
>> + {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(pci, niu_pci_tbl);
>> +
>> +#define NIU_TX_TIMEOUT (5 * HZ)
>> +
>> +#define nr64(reg) readq(np->regs + (reg))
>> +#define nw64(val, reg) writeq((val), np->regs + (reg))
>
> Macro's that make assumptions about context (ie variable name np)
> are evil and bad style.
No, that's a common and encouraged convenience that makes the code a lot
easier to read.
That said, the 2-arg macro arg order is wrong. It should be (reg,val)
like tg3 and other drivers.
Jeff
^ permalink raw reply
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: David Miller @ 2007-09-19 22:07 UTC (permalink / raw)
To: shemminger; +Cc: netdev, Ariel.Hendel, greg.onufer, jeff
In-Reply-To: <20070919145900.759ef19e@freepuppy.rosehill.hemminger.net>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 19 Sep 2007 14:59:00 -0700
> > +#ifndef PCI_DEVICE_ID_SUN_NEPTUNE
> > +#define PCI_DEVICE_ID_SUN_NEPTUNE 0xabcd
> > +#endif
>
> Why bother defining the ID, and what good does driver_data do you?
Good point, I'll put this 0xabcd thing directly into the
table.
> > +#define nr64(reg) readq(np->regs + (reg))
> > +#define nw64(val, reg) writeq((val), np->regs + (reg))
>
> Macro's that make assumptions about context (ie variable name np)
> are evil and bad style.
Too bad, it saves developers who write 20,000 line drivers from carpel
tunnel symdrome.
It's so repetitive to state things over and over again, and this macro
scheme is 1) established widely (tg3 amonst many other drivers do it)
and 2) forces the driver to use a consistent naming convention for
primary driver private struct variables.
> Wrapping locking is poor style and makes code review harder.
I disagree, because in this case the macro says exactly what the thing
is doing, locking the parent of the port. The implementation is
arbitrary and can be reviewed independently of the uses.
> > +static int niu_wait_bits_clear_mac(struct niu *np, unsigned long reg, u64 bits,
> > + int limit, int delay)
> > +{
> > + BUILD_BUG_ON(limit <= 0 || delay < 0);
>
> There is no way compiler can evaluate limit or delay.
Check the callers, they all pass contant values.
> Why are you using __u32? That is reserved for values going out
> to user space.
Right, I'll fix those up, thanks.
^ permalink raw reply
* Re: Please pull 'z1211' branch of wireless-2.6
From: Daniel Drake @ 2007-09-19 22:08 UTC (permalink / raw)
To: John W. Linville
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, jeff-o2qLIJkoznsdnm+yROfE0A,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
kune-hUSrv6EASfkEnNRfnnE9gw
In-Reply-To: <20070919181005.GB5483-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
John W. Linville wrote:
> The maintainers have identified some (IMHO minor) issues with the
> mac80211 port of this driver. Some of them are summarized here:
>
> http://linuxwireless.org/en/users/Drivers/zd1211rw/mac80211Issues
>
> Still, I think we would be better-off having this driver upstream.
I apologise for being inactive over the last few weeks, I've been moving
back from the US to the UK and then moving back to university. However I
am now just-about-settled and will be active again soon.
I would like to this until 2.6.25 until I have had time to clear up some
final issues and do more testing myself of zd1211rw-mac80211. I also
think we need to discuss the rename...
(just to clarify to others: this is the first I heard of this merge
before John posted it).
John, thanks a lot for your efforts, I hope you don't mind waiting one
extra release cycle for me to sort a few things out.
Thanks,
Daniel
^ permalink raw reply
* Re: Please pull 'p54' branch of wireless-2.6
From: David Miller @ 2007-09-19 22:08 UTC (permalink / raw)
To: jeff-o2qLIJkoznsdnm+yROfE0A
Cc: linville-2XuSBdqkA4R54TAoqtyWWQ, hch-wEGCiKHe2LqWVfeAwA7xHQ,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
flamingice-R9e9/4HEdknk1uMJSBkQmQ
In-Reply-To: <46F19C3C.6090605-o2qLIJkoznsdnm+yROfE0A@public.gmane.org>
From: Jeff Garzik <jeff-o2qLIJkoznsdnm+yROfE0A@public.gmane.org>
Date: Wed, 19 Sep 2007 18:01:32 -0400
> There were complaints about the nonsensical wireless/mac80211
> directory. But if there are enough files, a driver should
> definitely get its own directory. Like life, it's a balance. If
> it's just one or two C files, a directory might be overkill. If
> it's more, it definitely warrants a sub-dir.
That's generally how I see things too.
^ permalink raw reply
* Re: new NAPI quota synchronization issue
From: David Miller @ 2007-09-19 22:10 UTC (permalink / raw)
To: shemminger; +Cc: netdev
In-Reply-To: <20070919150129.7b0967f7@freepuppy.rosehill.hemminger.net>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 19 Sep 2007 15:01:29 -0700
> There might be a future problem if some driver decided to change
> weight often on the fly? Perhaps you need to sample it once in
> start of napi_schedule.
Fair enough, I checked the following into net-2.6.24
commit 3b4eed9f46e4b0405a0d8921c2319f2b7c6a6b4a
Author: David S. Miller <davem@sunset.davemloft.net>
Date: Wed Sep 19 15:09:55 2007 -0700
[NAPI]: Sample weight into a local var in case it changes.
Noted by Stephen Hemminger.
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/net/core/dev.c b/net/core/dev.c
index 0b9f82e..91c31e6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2136,7 +2136,7 @@ static void net_rx_action(struct softirq_action *h)
while (!list_empty(list)) {
struct napi_struct *n;
- int work;
+ int work, weight;
/* If softirq window is exhuasted then punt.
*
@@ -2159,9 +2159,11 @@ static void net_rx_action(struct softirq_action *h)
have = netpoll_poll_lock(n);
- work = n->poll(n, n->weight);
+ weight = n->weight;
- WARN_ON_ONCE(work > n->weight);
+ work = n->poll(n, weight);
+
+ WARN_ON_ONCE(work > weight);
budget -= work;
@@ -2172,7 +2174,7 @@ static void net_rx_action(struct softirq_action *h)
* still "owns" the NAPI instance and therefore can
* move the instance around on the list at-will.
*/
- if (unlikely(work == n->weight))
+ if (unlikely(work == weight))
list_move_tail(&n->poll_list, list);
netpoll_poll_unlock(have);
^ permalink raw reply related
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: David Miller @ 2007-09-19 22:11 UTC (permalink / raw)
To: jeff; +Cc: shemminger, netdev, Ariel.Hendel, greg.onufer
In-Reply-To: <46F19D25.2010403@garzik.org>
From: Jeff Garzik <jeff@garzik.org>
Date: Wed, 19 Sep 2007 18:05:25 -0400
> That said, the 2-arg macro arg order is wrong. It should be (reg,val)
> like tg3 and other drivers.
I actually wanted to mirror the ordering that
writel() uses.
So the choice is arbitrary, either be consistent with
our well established asm/io.h interfaces or a bunch
of network driver :-)
^ permalink raw reply
* Re: Please pull 'z1211' branch of wireless-2.6
From: David Miller @ 2007-09-19 22:14 UTC (permalink / raw)
To: dsd-aBrp7R+bbdUdnm+yROfE0A
Cc: linville-2XuSBdqkA4R54TAoqtyWWQ, jeff-o2qLIJkoznsdnm+yROfE0A,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
kune-hUSrv6EASfkEnNRfnnE9gw
In-Reply-To: <46F19DD0.2050605-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
From: Daniel Drake <dsd-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Date: Wed, 19 Sep 2007 23:08:16 +0100
> John, thanks a lot for your efforts, I hope you don't mind waiting one
> extra release cycle for me to sort a few things out.
Having "a driver" is always better for users than having "no driver".
Please I ask you not to block this merge even if you think it is too
early.
Distribution vendors are going to merge this driver into their kernel
trees anyways. And as Linus stated at the recent kernel summit, when
this happens and upstream doesn't merge the driver too, the process
has failed.
We have had huge issues with shit piling up in the wireless tree and
it has to stop sooner rather than later, and I'm going to make sure
it stops now.
So please don't feel compelled to fix "all the issues" before a merge.
Drivers will always have issues, and if we merge now you will get more
useful feedback, sooner, and users will have something to use rather
than no driver at all.
Thanks.
^ permalink raw reply
* Re: net-2.6.24 - build failure
From: David Miller @ 2007-09-19 22:17 UTC (permalink / raw)
To: joe; +Cc: netdev, linux-wireless
In-Reply-To: <1190238802.26101.24.camel@localhost>
From: Joe Perches <joe@perches.com>
Date: Wed, 19 Sep 2007 14:53:22 -0700
> drivers/net/wireless/b43legacy/built-in.o: In function `tsf_read_file':
> drivers/net/wireless/b43legacy/debugfs.c:80: multiple definition of `tsf_read_file'
Can one of the wireless folks fix b43legacy to not use the same
global variable and function names as the b43 driver?
Thanks.
^ permalink raw reply
* Re: net-2.6.24 plans
From: David Miller @ 2007-09-19 22:19 UTC (permalink / raw)
To: linville; +Cc: akpm, netdev, jgarzik
In-Reply-To: <20070919210700.GF5483@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Wed, 19 Sep 2007 17:07:00 -0400
> And the latest incarnation:
>
> http://www.kernel.org/pub/linux/kernel/people/linville/git-wireless-2007-09-19-16-57-43-0500.patch
>
> And is even smaller...down an additional 30%, or almost 1/6th of the
> original! :-)
Yes, great progress, thanks John :-)
So it looks like what's left is:
1) ATH5K driver
2) ATMEL USB driver
3) NL80211
4) misc bits sprinkled around mac80211
^ permalink raw reply
* Re: [PATCH] iproute2: enable setting rto_min, rtt and rttvar in tc-esque units
From: Rick Jones @ 2007-09-19 22:21 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, netdev
In-Reply-To: <20070919.150312.31444471.davem@davemloft.net>
>>>Enable users of ip to specify the times for rtt, rttvar and rto_min
>>>in human-friendly terms a la "tc" while maintaining backwards
>>>compatability with the previous "raw" mechanism. Builds upon
>>>David Miller's uncommited patch to set rto_min.
>>>
>>>Signed-off-by: Rick Jones <rick.jones2@hp.com>
>>
>>Thanks, I'll put it in the 2.6.24 version of iproute2 (assuming dave
>>takes the rto_min patch).
>
> If you're talking about the kernel side, Linus's tree already
> has the per-route rto_min stuff.
I was testing the patch using a 2.6.23-rc5 kernel after I noticed from
reading the changelog that the rto_min change went in there. It would
be very nice if it were possible to be in the .23 version of iproute2,
but I can easily see where things might be a bit late for that.
Meanwhile I'll let the customer know that a way (short of writing their
own util I guess :) to set the rto_min will be in at least .24
thanks,
rick jones
^ permalink raw reply
* Re: Please pull 'z1211' branch of wireless-2.6
From: Daniel Drake @ 2007-09-19 22:27 UTC (permalink / raw)
To: David Miller; +Cc: linville, jeff, netdev, linux-wireless, kune
In-Reply-To: <20070919.151446.116351108.davem@davemloft.net>
David Miller wrote:
> From: Daniel Drake <dsd@gentoo.org>
> Date: Wed, 19 Sep 2007 23:08:16 +0100
>
>> John, thanks a lot for your efforts, I hope you don't mind waiting one
>> extra release cycle for me to sort a few things out.
>
> Having "a driver" is always better for users than having "no driver".
> Please I ask you not to block this merge even if you think it is too
> early.
I don't understand your point. We already have this driver in mainline
Linux.
zd1211rw-mac80211 does not currently add anything to the user experience
over the existing driver, other than automatic rate adaption which is
nice but hasn't proven itself to be of much importance given that the
existing driver has lots of happy users and has never done it.
Also, this merge has been proposed in a form other than what me and
Ulrich were envisioning (at least from earlier discussions). We want to
convert drivers/net/wireless/zd1211rw to mac80211 directly. This would
also avoid a rename and I think would save some hassle.
Sorry to be a pain, but I really would like some more time to catch up
and make some last changes. I haven't yet even looked at Johannes'
minimal filter flags implementation in the driver or Michael's fix.
Would you agree to let this wait until 2.6.25? You can hold me to that,
although Ulrich should also comment on if he thinks he'd be ready by then.
Thanks and apologies,
Daniel
^ permalink raw reply
* Re: Please pull 'z1211' branch of wireless-2.6
From: David Miller @ 2007-09-19 22:32 UTC (permalink / raw)
To: dsd-aBrp7R+bbdUdnm+yROfE0A
Cc: linville-2XuSBdqkA4R54TAoqtyWWQ, jeff-o2qLIJkoznsdnm+yROfE0A,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
kune-hUSrv6EASfkEnNRfnnE9gw
In-Reply-To: <46F1A23D.8080606-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
From: Daniel Drake <dsd-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Date: Wed, 19 Sep 2007 23:27:09 +0100
> zd1211rw-mac80211 does not currently add anything to the user experience
> over the existing driver, other than automatic rate adaption which is
> nice but hasn't proven itself to be of much importance given that the
> existing driver has lots of happy users and has never done it.
Ok.
> Also, this merge has been proposed in a form other than what me and
> Ulrich were envisioning (at least from earlier discussions). We want to
> convert drivers/net/wireless/zd1211rw to mac80211 directly. This would
> also avoid a rename and I think would save some hassle.
>
> Sorry to be a pain, but I really would like some more time to catch up
> and make some last changes. I haven't yet even looked at Johannes'
> minimal filter flags implementation in the driver or Michael's fix.
>
> Would you agree to let this wait until 2.6.25? You can hold me to that,
> although Ulrich should also comment on if he thinks he'd be ready by then.
Fair enough, I'll remove the Z1211 driver during my next rebase.
^ permalink raw reply
* Re: Please pull 'z1211' branch of wireless-2.6
From: Daniel Drake @ 2007-09-19 22:12 UTC (permalink / raw)
To: John W. Linville; +Cc: davem, jeff, netdev, linux-wireless, kune
In-Reply-To: <20070919181005.GB5483@tuxdriver.com>
John W. Linville wrote:
> Jeff & Dave,
>
> This patch adds z1211 (previously known as zd1211rw-mac80211),
> a driver for Zydas ZD1211 hardware. This driver has proven very
> robust -- Fedora 7 uses this driver and I don't think I have any open
> bugzillas for it.
BTW: I fairly regularly get email from F7 users complaining about
connection intermittancy and other bugs that we don't seem to have for
the softmac driver (maybe stack related issues, of which I've fixed a
couple that affected me personally, I'm a little surprised that F7
jumped so early).
I've been suggesting that such users move to zd1211rw-softmac until both
Ulrich and myself are happy and we've made the jump upstream.
Daniel
^ permalink raw reply
* [PATCH 2.6.23-rc6 Resending] NETWORKING : Edge Triggered EPOLLOUT events get missed for TCP sockets
From: Nagendra Tomar @ 2007-09-19 22:37 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, davem, Davide Libenzi
The tcp_check_space() function calls tcp_new_space() only if the
SOCK_NOSPACE bit is set in the socket flags. This is causing Edge Triggered
EPOLLOUT events to be missed for TCP sockets, as the ep_poll_callback()
is not called from the wakeup routine.
The SOCK_NOSPACE bit indicates the user's intent to perform writes
on that socket (set in tcp_sendmsg and tcp_poll). I believe the idea
behind the SOCK_NOSPACE check is to optimize away the tcp_new_space call
in cases when user is not interested in writing to the socket. These two
take care of all possible scenarios in which a user can convey his intent
to write on that socket.
Case 1: tcp_sendmsg detects lack of sndbuf space
Case 2: tcp_poll returns not writable
This is fine if we do not deal with epoll's Edge Triggered events (EPOLLET).
With ET events we can have a scenario where the SOCK_NOSPACE bit is not set,
as the user has neither done a sendmsg nor a poll/epoll call that returned
with the POLLOUT condition not set.
In this case the user will _never_ get an ET POLLOUT event since
tcp_check_space() will not call tcp_new_space() (as the SOCK_NOSPACE bit is
not set), which does the real work. THIS IS AGAINST THE EPOLL ET PROMISE OF
DELIVERING AN EVENT WHENEVER THE EVENT ACTUALLY HAPPENS.
This ET event will be very helpful to implement user level memory management
for mmap+sendfile zero copy Tx. So typically the application does this
void *alloc_sendfile_buf(void)
{
while(!next_free_buffer)
{
/*
* No free buffers (all are dispatched to sendfile and are
* in use). Wait for one or more buffers to become free
* The socket fd is registered with EPOLLET|EPOLLOUT events.
* EPOLLET enables us to check for SIOCOUTQ only when some
* more space becomes available.
*
* One would expect the ET EPOLLOUT event to be notified
* when TCP space is freed due to some ack coming in.
*/
epoll_wait(...); /* wait for some incoming ack to free some
buffer from the retransmit queue */
ioctl(fd, SIOCOUTQ, &in_outq);
/*
* see if we can mark some more "complete" buffers free
* If it can mark one or more buffer free, it will set
* next_free_buffer to point to the available buffer to use
*/
rehash_free_buffers(in_outq);
}
return next_free_buffer;
}
With the SOCK_NOSPACE check in tcp_check_space(), this epoll_wait call will
not return, even when the incoming acks free the buffers.
Note that this patch assumes that the SOCK_NOSPACE check in
tcp_check_space is a trivial optimization which can be safely removed.
Thanx,
Tomar
Signed-off-by: Nagendra Singh Tomar <nagendra_tomar@adaptec.com>
---
--- linux-2.6.23-rc6/net/ipv4/tcp_input.c.orig 2007-09-19 13:58:44.000000000 +0530
+++ linux-2.6.23-rc6/net/ipv4/tcp_input.c 2007-09-19 10:17:36.000000000 +0530
@@ -3929,8 +3929,7 @@ static void tcp_check_space(struct sock
{
if (sock_flag(sk, SOCK_QUEUE_SHRUNK)) {
sock_reset_flag(sk, SOCK_QUEUE_SHRUNK);
- if (sk->sk_socket &&
- test_bit(SOCK_NOSPACE, &sk->sk_socket->flags))
+ if (sk->sk_socket)
tcp_new_space(sk);
}
}
___________________________________________________________
Yahoo! Answers - Got a question? Someone out there knows the answer. Try it
now.
http://uk.answers.yahoo.com/
^ permalink raw reply
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: Jeff Garzik @ 2007-09-19 22:37 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, netdev, Ariel.Hendel, greg.onufer
In-Reply-To: <20070919.151144.59833230.davem@davemloft.net>
David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Wed, 19 Sep 2007 18:05:25 -0400
>
>> That said, the 2-arg macro arg order is wrong. It should be (reg,val)
>> like tg3 and other drivers.
>
> I actually wanted to mirror the ordering that
> writel() uses.
>
> So the choice is arbitrary, either be consistent with
> our well established asm/io.h interfaces or a bunch
> of network driver :-)
IMO (val,reg) was always unnatural. From looking at the register
read/write interfaces people continually reinvent, humans definitely
lean towards (reg,val).
Jeff
^ permalink raw reply
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: Jeff Garzik @ 2007-09-19 22:40 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, netdev, Ariel.Hendel, greg.onufer
In-Reply-To: <20070919.150755.38714639.davem@davemloft.net>
David Miller wrote:
>> Macro's that make assumptions about context (ie variable name np)
>> are evil and bad style.
>
> Too bad, it saves developers who write 20,000 line drivers from carpel
> tunnel symdrome.
>
> It's so repetitive to state things over and over again, and this macro
> scheme is 1) established widely (tg3 amonst many other drivers do it)
> and 2) forces the driver to use a consistent naming convention for
> primary driver private struct variables.
It also makes review easier -- IMO an important factor.
Which is more clear at a glance:
status = tr32(MAC_STAT_1);
or
status = foo_mac_read32(priv, MAC_STAT_1);
?
tg3 and other net drivers show that this technique improves code
readability.
Jeff
^ permalink raw reply
* Re: [PATCH 2.6.23-rc6 Resending] NETWORKING : Edge Triggered EPOLLOUT events get missed for TCP sockets
From: David Miller @ 2007-09-19 22:44 UTC (permalink / raw)
To: tomer_iisc; +Cc: netdev, linux-kernel, davidel
In-Reply-To: <130356.85796.qm@web53703.mail.re2.yahoo.com>
From: Nagendra Tomar <tomer_iisc@yahoo.com>
Date: Wed, 19 Sep 2007 15:37:09 -0700 (PDT)
> With the SOCK_NOSPACE check in tcp_check_space(), this epoll_wait call will
> not return, even when the incoming acks free the buffers.
> Note that this patch assumes that the SOCK_NOSPACE check in
> tcp_check_space is a trivial optimization which can be safely removed.
I already replied to your patch posting explaining that whatever is
not setting SOCK_NOSPACE should be fixed instead.
Please address that, thanks.
^ permalink raw reply
* Re: [PATCH 2.6.23-rc6 Resending] NETWORKING : Edge Triggered EPOLLOUT events get missed for TCP sockets
From: Nagendra Tomar @ 2007-09-19 22:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, davidel
In-Reply-To: <20070919.154402.34760338.davem@davemloft.net>
--- David Miller <davem@davemloft.net> wrote:
> From: Nagendra Tomar <tomer_iisc@yahoo.com>
> Date: Wed, 19 Sep 2007 15:37:09 -0700 (PDT)
>
> > With the SOCK_NOSPACE check in tcp_check_space(), this epoll_wait call will
> > not return, even when the incoming acks free the buffers.
> > Note that this patch assumes that the SOCK_NOSPACE check in
> > tcp_check_space is a trivial optimization which can be safely removed.
>
> I already replied to your patch posting explaining that whatever is
> not setting SOCK_NOSPACE should be fixed instead.
>
> Please address that, thanks.
Dave,
I agree that setting SOCK_NOSPACE would have been a more elegant
fix. Infact I thought a lot about that before deciding on this fix.
But the point here is that the SOCK_NOSPACE bit can be set when
the sndbuf space is really less (less than sk_stream_min_wspace())
and some user action (sendmsg or poll) indicated his intent to write.
In the case mentioned none of these is true. Since user wants
to manage his tranmit buffers himself, his definition of less may
not match with what kernel feels is less. f.e. user might have
dynamically changing mmap'ed buffer resources at his disposal
which he wants to use as sendfile buffers. He wants to be notified
whenever a new incoming ack frees up one or more of his buffers,
so that he can reuse that buffer.
The bigger problem is that user is not indicating his intent
to write, to the kernel. He is just watching the sendbuf space and
when it matches his needs he will send new data.
Thanx,
Tomar
___________________________________________________________
Want ideas for reducing your carbon footprint? Visit Yahoo! For Good http://uk.promotions.yahoo.com/forgood/environment.html
^ permalink raw reply
* Re: [PATCH]: Preliminary release of Sun Neptune driver
From: Stephen Hemminger @ 2007-09-19 23:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Ariel.Hendel, greg.onufer, jeff
In-Reply-To: <20070919.150755.38714639.davem@davemloft.net>
On Wed, 19 Sep 2007 15:07:55 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Wed, 19 Sep 2007 14:59:00 -0700
>
...
> > > +static int niu_wait_bits_clear_mac(struct niu *np, unsigned long reg, u64 bits,
> > > + int limit, int delay)
> > > +{
> > > + BUILD_BUG_ON(limit <= 0 || delay < 0);
> >
> > There is no way compiler can evaluate limit or delay.
>
> Check the callers, they all pass contant values.
GCC is not as smart as you think... Try the following test:
--------------------
#include <stdio.h>
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
#define ENODEV -10
static int niu_wait_bits_clear_mac(void *np, unsigned long reg,
int limit, int delay)
{
BUILD_BUG_ON(limit <= 0 || delay < 0);
if (limit < 0)
return -ENODEV;
while (limit > 0)
sleep(delay);
return 0;
}
int main(int argc, char **argv) {
printf("start\n");
niu_wait_bits_clear_mac(NULL, 0, 10, 1);
niu_wait_bits_clear_mac(NULL, 0, -1, 0);
niu_wait_bits_clear_mac(NULL, 0, 0, -1);
return 0;
}
^ 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