netdev
[Top] [All Lists]

Re: Submission #3 for S2io 10GbE driver

To: raghavendra.koushik@xxxxxxxxx
Subject: Re: Submission #3 for S2io 10GbE driver
From: Jeff Garzik <jgarzik@xxxxxxxxx>
Date: Tue, 02 Mar 2004 13:47:09 -0500
Cc: leonid.grossman@xxxxxxxx, netdev@xxxxxxxxxxx, shemminger@xxxxxxxx, hch@xxxxxxxxxxxxx, ravinandan.arakali@xxxxxxxx, raghavendra.koushik@xxxxxxxx
In-reply-to: <4223A04BF7D1B941A25246ADD0462FF50115AFCF@xxxxxxxxxxxxxxxxxxxx>
References: <4223A04BF7D1B941A25246ADD0462FF50115AFCF@xxxxxxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030703
raghavendra.koushik@xxxxxxxxx wrote:
Hi Jeff,

        Really sorry about that "confidentiality notice" that gets attached.
I have asked my sysAdmin to get rid of it. He has promised to do so ASAP.
Hope this mail does not have it attached at the end :-). If at all the
message still persists please ignore it as inconsequential to our discussion.

I Have a few more questions.


4) just delete the SET_NETDEV_DEV(), FREE_NETDEV, and IRQ_NONE
compatibility defines.  these are in 2.4 just like 2.6.


Not all 2.4 kernels have them yet right? but since this driver is going

Correct. But when I merge this driver into 2.4, it will be latest 2.4 (which contains these definitions).

The vendor (s2io) is expected to maintain any old-kernel compatibility outside the kernel. For example, if you submitting the s2io driver for inclusion in my employer's product, Red Hat Enterprise Linux, then you would submit with the compatibility gunk.


into 2.6 kernel if you want all these backward compatibility macros eliminated
I can do that.

Yes, please.


13) in s2io_xmit, kfree the skb (drop it) if you don't have enough free
space to queue it.  this is normally a BUG condition, since proper use
of netif_{start,stop,wake}_queue() will guarantee that s2io_xmit will
only be called when there is free space to queue another skb.


On returning error (non zero) from s2io_xmit, I think the calling function frees
the skb.

Not correct in all cases, this is why the driver should drop the skb and free it. Several existing drivers get this wrong, in fact :/


How s2io_xmit works is when I get a packet for Tx and I find that all the
Tx descriptors are owned by the NIC, I stop the queue and return error.
So I wouldn't know before hand whether free queue space is available or not.

This is incorrect, and definitely an issue that needs to be addressed.

As I said, the model is, the driver calls netif_stop_queue() after queueing a packet, when it knows there is no more room for a full packet. The tg3 driver does it like this:
        
        ...queue an skb to hardware...
        if (TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1))
                netif_stop_queue(dev);

Therefore you guarantee the queue is stopped until you are 100% certain that another skb (up to MAX_SKB_FRAGS + "main frag" fragments) may be queued to hardware.

You do -not- want to figure out "after the fact" that you cannot queue the skb you were just passed.

        Jeff




<Prev in Thread] Current Thread [Next in Thread>