RE: Submission #3 for S2io 10GbE driver

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.

quoted

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
into 2.6 kernel if you want all these backward compatibility macros eliminated
I can do that.

quoted

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


Regards
Koushik


-----Original Message-----
From: Jeff Garzik [mailto:jgarzik@pobox.com] 
Sent: Monday, March 01, 2004 12:24 PM
To: Raghavendra Koushik (WT01 - EMBEDDED & PRODUCT ENGINEERING SOLUTIONS)
Cc: leonid.grossman@s2io.com; netdev@oss.sgi.com; shemminger@osdl.org; hch@infradead.org; ravinandan.arakali@s2io.com; raghavendra.koushik@s2io.com
Subject: Re: Submission #3 for S2io 10GbE driver


raghavendra.koushik@wipro.com wrote:

Jeff,
 Regarding Point # 37

quoted

quoted

37) kill all of this:

+/*  OS related system calls */
+
+#ifndef readq
+static inline u64 read64(void *addr)
+{
+       u64 ret = 0;
+       ret = readl(addr + 4);
+       (u64) ret <<= 32;
+       (u64) ret |= readl(addr);

	[....]

I agree that read/write(32,16,8) are not used so can be eliminated, 
but the read/write64 macros are essential because not all platforms 
have defined the readq and writeq system calls. i386 for example 
doesn't have readq/writeq and to write into the 64 bit registers of 
the NIC, I use 2 successive 32 bits (readl/writel) operation to 
achieve the 64 bit equivalent. This procedure does work on all the 
platforms that we have tested on.

The code should use the kernel API -- readq/writeq -- not define its own 
API.  With regards to the missing readq/writeq on some architectures...

Short term, if some arches do not provide readq/writeq, provide your own 
definition (i.e. rename your write64 to a conditionally-defined writeq).

Long term, all Linux platforms need to provide readq/writeq, so we need 
to modify the architectures with the missing pieces.

Confidentiality Notice

The information contained in this electronic message and any 
attachments to this message are intended for the exclusive use of the 
addressee(s) and may contain confidential or privileged information. 
If you are not the intended recipient, please notify the sender at 
Wipro or Mailadmin@wipro.com immediately and destroy all copies of 
this message and any attachments.

Oh really?  ;-)  You should talk to your lawyers and sysadmins about 
sending email to open source people and lists...

Regards,

	Jeff

raghavendra.koushik@wipro.com 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.

quoted

quoted

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.

quoted

quoted

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