lkml.org 
[lkml]   [2002]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    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,
    thanks.

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

    dev->hard_start_xmit()
    if (free slots < MAX_SKB_FRAGS)
    BUG()
    queue packet
    if (free slots < MAX_SKB_FRAGS)
    netif_stop_queue(dev)

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

    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.

    Jeff




    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 13:25    [W:0.026 / U:61.676 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site