[lkml]   [2002]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [patch] ns83820 0.17 (Re: Broadcom 5700/5701 Gigabit Ethernet Adapters)
Benjamin LaHaise wrote:

>>3) Seeing "volatile" in your code. Cruft? volatile's meaning change in
>>recent gcc versions implies to me that your code may need some addition
>>rmb/wmb calls perhaps, which are getting hidden via the driver's
>>dependency on a compiler-version-specific implementation of "volatile."
>Paranoia during writing. I'll reaudit. That said, volatile behaviour
>is not compiler version specific.
gcc 3.1 volatile behavior changes, so, yes, it is...

>>4) Do you really mean to allocate memory for "REAL_RX_BUF_SIZE + 16"?
>> Why not plain old REAL_RX_BUF_SIZE?
>The +16 is for alignment (just like the comment says). The hardware
>requires that rx buffers be 64 bit aligned.
Cool... just checking. Both RX_BUF_SIZE and REAL_RX_BUF_SIZE are defined as
foo + magic_number

so I wasn't sure if the alignment space was -already- accounted for, in
the definition of RX_BUF_SIZE, thus making the addition op next to
allocations of REAL_RX_BUF_SIZE superfluous. But, I stand corrected,

>5) Random question, do you call netif_carrier_{on,off,ok} for link
>> status manipulation? (if not, you should...)
>Ah, api updates. Added to the todo.
More than just api updates... You have a bunch of hack-y logic for when
the link goes down and up, messing around with netif_stop_queue and
netif_wake_queue. That stuff will be simplified or simply go away. The
basic idea is, if netif_carrier_ok(dev) is not true, then the net stack
will not be sending you any packets. So those extra
netif_{stop,wake}_queue calls are superflouous.

We're also about to start sending link up/down messages async-ly via
netlink, so that's even more added value as well.

>>6) skb_mangle_for_davem is pretty gross... curious: what sort of NIC
>>alignment restrictions are there on rx and tx buffers (not descriptors)?
>> None? 32-bit? Ignore CPU alignment for a moment here...
>tx descriptors have no alignment restriction, rx descriptors must be
>64 bit aligned. Someone chose not to include the transistors for a
>barrel shifter in the rx engine.

Sigh :)

>>7) What are the criteria for netif_wake_queue? If you are waking when
>>the TX is still "mostly full" you probably generate excessive wakeups...
>Hrm? Currently it will do a wakeup when at least one packet (8 sg
>descriptors) can be sent. Given that the tx done code is only called
>when a tx desc (every 1/4 or so of the tx queue) or txidle interrupt
>occurs, it shouldn't be that often.

Cool. As FYI (_not_ advice on your driver), here's the logic I was
referring to:

if (free slots < MAX_SKB_FRAGS)
queue packet
if (free slots < MAX_SKB_FRAGS)

if (some tx interrupt)
complete as many TX's as possible
if (netif_queue_stopped && (free slots > (TX_RING_SIZE / 4)))

But as long as your TX interrupts are well mitigated (and it sounds like
they are), you can get by with your current scheme just fine.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

 \ /
  Last update: 2005-03-22 13:25    [W:0.247 / U:6.588 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site