netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: Submission #3  for S2io 10GbE driver
@ 2004-03-01 13:05 raghavendra.koushik
  2004-03-01 15:24 ` Leonid Grossman
  0 siblings, 1 reply; 16+ messages in thread
From: raghavendra.koushik @ 2004-03-01 13:05 UTC (permalink / raw)
  To: leonid.grossman, netdev, ravinandan.arakali; +Cc: sriram.rapuru

Hi Leonid,
 
   This automated signature will be gone from tomorrow. Our IMG guys
are working on it and hopefully by then all mails going out from 
the s2io guys will not have this "Confidentiality Notice" :-).
I have a few more questions to Jeff but I'am holding on to them
till this is addressed. Just wanted to keep you posted on this
issue.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* RE: Submission #3  for S2io 10GbE driver
@ 2004-03-02 21:47 Feldman, Scott
  2004-03-02 22:21 ` Ben Greear
  0 siblings, 1 reply; 16+ messages in thread
From: Feldman, Scott @ 2004-03-02 21:47 UTC (permalink / raw)
  To: Jeff Garzik, Ben Greear; +Cc: netdev

> Let's fix the broken drivers first.

Ok, e100 v3 is same model as tg3, and we think tg3 is correct, so we
think e100 v3 is correct as well.

Ben, e100 v3 probably isn't given you the effect you had with previous
driver versions.  It'll stop the queue and never return a skb (unless we
hit that BUG case).

e1000/ixgb need some work to bring them into this model.

-scott

^ permalink raw reply	[flat|nested] 16+ messages in thread
* RE: Submission #3  for S2io 10GbE driver
@ 2004-03-02 21:16 Feldman, Scott
  2004-03-02 21:21 ` Jeff Garzik
  0 siblings, 1 reply; 16+ messages in thread
From: Feldman, Scott @ 2004-03-02 21:16 UTC (permalink / raw)
  To: Jeff Garzik, raghavendra.koushik
  Cc: leonid.grossman, netdev, shemminger, hch, ravinandan.arakali,
	raghavendra.koushik

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

But tg3 checks this case also and returns 1:

        /* This is a hard error, log it. */
        if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags +
1))) {
                netif_stop_queue(dev);
                ...
                return 1;
        }

Does this code path happen?

Using tg3 for reference, can we say this is the ideal model?

dev->hard_start

	if(!space_avail)
		stop_queue
		return 1;

	queue skb

	if(!space_avail)
		stop_queue

	return 0;

tx_clean

	if(queue_stopped && space_avail)
		wake_queue

-scott

^ permalink raw reply	[flat|nested] 16+ messages in thread
* RE: Submission #3  for S2io 10GbE driver
@ 2004-03-02 13:46 raghavendra.koushik
  2004-03-02 18:47 ` Jeff Garzik
  0 siblings, 1 reply; 16+ messages in thread
From: raghavendra.koushik @ 2004-03-02 13:46 UTC (permalink / raw)
  To: jgarzik
  Cc: leonid.grossman, netdev, shemminger, hch, ravinandan.arakali,
	raghavendra.koushik

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


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

^ permalink raw reply	[flat|nested] 16+ messages in thread
* RE: Submission #3  for S2io 10GbE driver
@ 2004-03-01  6:21 raghavendra.koushik
  2004-03-01  6:53 ` Jeff Garzik
  0 siblings, 1 reply; 16+ messages in thread
From: raghavendra.koushik @ 2004-03-01  6:21 UTC (permalink / raw)
  To: jgarzik, leonid.grossman
  Cc: netdev, shemminger, hch, ravinandan.arakali, raghavendra.koushik

Jeff,
 Regarding Point # 37

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


Regards

Koushik




-----Original Message-----
From: Jeff Garzik [mailto:jgarzik@pobox.com] 
Sent: Sunday, February 29, 2004 1:52 AM
To: Leonid Grossman
Cc: netdev@oss.sgi.com; 'Stephen Hemminger'; 'Christoph Hellwig'; 'ravinandan arakali'; raghavendra.koushik@s2io.com
Subject: Re: Submission #3 for S2io 10GbE driver


Looking a lot better.  A few merge issues remain, and some operational 
ones as well.  There are 39 issues in this review, but IMO they are 
mostly minor issues that don't require much thought or work.


Comments:

0) to repeat myself from an earlier review...  grumble...
You CANNOT use NETIF_F_HW_CSUM, when your hardware does not provide the 
checksum value.  You must use NETIF_F_IP_CSUM.  Your use of 
NETIF_F_HW_CSUM + CHECKSUM_UNNECESSARY is flat out incorrect.

1) the makefile is for out-of-tree stuff.  The proper makefile will be 
much smaller.  So just submit a proper in-tree Makefile.

2) (in general) we don't want the compatibility stuff in-tree, that's 
for out-of-tree as well.

3) just submit a patch to include/linux/pci_ids.h instead of the following

+/* VENDOR and DEVICE ID of XENA. */
+#ifndef PCI_VENDOR_ID_S2IO
+#define PCI_VENDOR_ID_S2IO      0x17D5
+#define PCI_DEVICE_ID_S2IO_WIN  0x5731
+#define PCI_DEVICE_ID_S2IO_UNI  0x5831
+#endif

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

5) Many PCI posting bugs remain.  FixMacAddress is an excellent 
illustration:

+       write64(&bar0->gpio_control, 0x0040600000000000ULL);
+       udelay(10);
+       write64(&bar0->gpio_control, 0x0000600000000000ULL);
+       udelay(10);
+       write64(&bar0->gpio_control, 0x0020600000000000ULL);
+       udelay(10);
+       write64(&bar0->gpio_control, 0x0060600000000000ULL);
+       udelay(10);

The delay is _not_ guaranteed at all, because you do not know when that 
write64() will actually be sent to the PCI bus.  Only a read[bwl,64] is 
guaranteed to flush the write to the PCI device.

So, the above code does not function as you would expect, on all platforms.

6) More examples of PCI posting bugs, in startNic:

+       write64(&bar0->mc_rldram_mrs, val64);
+       set_current_state(TASK_UNINTERRUPTIBLE);
+       schedule_timeout(HZ / 10);

and

+       write64(&bar0->dtx_control, 0x8007051500000000ULL);
+       udelay(50);
+       write64(&bar0->dtx_control, 0x80070515000000E0ULL);
+       udelay(50);
+       write64(&bar0->dtx_control, 0x80070515001F00E4ULL);
+       udelay(50);

7) for fragmented skb's, you should be using pci_map_page() not 
pci_map_single().  Example in drivers/net/tg3.c.

8) (style) in alarmIntrHandler, due to line wrapping, Lindent has 
rendered the 'do' loop rather unreadable.

9) you cannot sleep inside the interrupt handler.  Therefore the 
schedule_timeout() in alarmIntrHandler is very wrong.

10) never use a plain constant when calling schedule_timeout(), such as 
in waitForCmdComplete.  Always calculate the desired delay based on the 
HZ constant.  Otherwise, your delay varies depending on platform.  HZ 
represents one second, in jiffies.  So half a second delay would be "HZ 
/ 2", etc.  Also, when fixing, be careful that your HZ-based calculation 
will never evaluate to zero.

11) ditto s2io_reset

12) ditto s2io_close.  etc.

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.

14) spin_lock(), not spin_lock_irqsave(), in your interrupt handler. 
spin_lock_irqsave() is normally used in any of three cases:  (1) don't 
know whether you're in an ISR or not, (2) definitely not in an ISR, or 
(3) your ISR is called from more than one hardware interrupt.  None of 
these three is the case.

15) does s2io_get_stats need locking?

16) (style) If you are going to comment each function, you might as well 
do it in the "kernel-doc" style, which allows the comments to be picked 
up by automated tools.  The format is

	/**
	 *	function_name - short description
	 *	@argument1: argument 1 description
	 *	@argument2: argument 2 description
	 *	...
	 * SOMETHING:
	 * blah blah blah
	 * SOMETHING ELSE:
	 * blah blah blah

The "ALL_CAPS:" indicates a new section/paragraph, in the document.

Once this is done, you may add a stub document to Documentation/DocBook/ 
and then create your driver's nicely-formatted documentation using "make 
pdfdocs", "make psdocs", or "make htmldocs".

17) this define belongs in include/linux/ethtool.h, if it's not there 
already...
+#define SPEED_10000 10000

18) remove #ifdefs such as
+#if defined(ETHTOOL_GREGS)
+       info->regdump_len = XENA_REG_SPACE;
+#endif

since this exists in both 2.4 and 2.6 kernels.

19) ditto:
+#ifdef ETHTOOL_PHYS_ID

20) for the ethtool EEPROM and register dumps, it would be nice to 
submit a patch to me for ethtool (http://sf.net/projects/gkernel/), 
which generates a verbose dump rather than a bunch of hex numbers 
incomprehensible to the user.  This is a long, boring, but easy task 
suitable to an intern, so I understand if it's not done immediately ;-)

21) s2io_ethtool_nway_reset should restart PHY autonegotiation, not 
reset the entire card

22) eliminate s2io_ethtool_get_link, it duplicates a generic (and 
equivalent) function in net/core/ethtool.c

23) ditto, for the s2io_ethtool_{get,set}_{sg,rx,tx}_csum stuff

24) don't explicitly set members to NULL in netdev_ethtool_ops

25) the update to s2io_tx_watchdog still leaves something to be desired. 
  You are no longer performing the could-take-a-long-time card reset 
inside of spin_lock_bh()... you are now doing it inside the timer 
interrupt :(  Move this to process context by using schedule_work() 
[2.6] or schedule_task [2.4]

27) Unconditional netif_wake_queue() in s2io_link() still unfixed.  You 
must check for room for additional TX, before calling 
netif_{start,wake}_queue().  Consider what happens if the link goes down 
under the TX-full condition [netif_stop_queue]... instant bug.

28) do NOT specify PCI latency timer value as non-zero. 
pci_set_master() chooses an appropriate latency timer value.  It is 
acceptable to leave this in as an option, as long as the module option's 
default is zero:

+static u8 latency_timer = 0xff;

29) (style) don't bother casting a void pointer:

+/*  Private member variable initialized to s2io NIC structure */
+       sp = (nic_t *) dev->priv;

30) redundant assignment of 'sp':

+       dev->irq = pdev->irq;
+       dev->base_addr = (unsigned long) sp->bar0;
+       sp = (nic_t *) dev->priv;

31) kill the #ifdef

+#ifdef SET_ETHTOOL_OPS
+       SET_ETHTOOL_OPS(dev, &netdev_ethtool_ops);
+#endif

This one is particularly silly, because SET_ETHTOOL_OPS() is -designed- 
to eliminate ifdefs.  A driver wishing to be compatible will provide its 
own SET_ETHTOOL_OPS definition, guaranteeing it can be used 
unconditionally in the driver code here.

32) mark s2io_starter with "__init"

33) kill this:

+#ifndef ETH_ALEN
+#define ETH_ALEN    6
+#endif

34) the definitions of SUCCESS and FAILURE are incorrect.  The driver 
should return 0 on success, and a negative errno-based error code on 
failure.  -EBUSY, -EOPNOTSUPP, etc.

35) kill this:

+#ifndef SUPPORTED_10000baseT_Full
+#define SUPPORTED_10000baseT_Full (1 << 12)
+#endif

36) (feature addition) you have a ton of NIC-specific statistics.  You 
should make those available to users, via ethtool.

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);
+
+       return ret;
+}
+#else
+static inline u64 read64(void *addr)
+{
+       u64 ret = readq(addr);
+       return ret;
+}
+#endif
+#define read32(addr, ret)  ret =  readl(addr);
+#define read16(addr, ret)  ret =  readw(addr);
+#define read8(addr, ret)   ret =  readb(addr);
+
+#ifndef writeq
+static inline void write64(void *addr, u64 val)
+{
+       writel((u32) (val), addr);
+       writel((u32) (val >> 32), (addr + 4));
+}
+#else
+#define write64(addr, ret) writeq(ret,(void *)addr)
+#endif
+#define write32(addr, ret) writel(ret,(void *)addr);
+#define write16(addr, ret) writew(ret,(void *)addr);
+#define write8(addr, ret)  writeb(ret,(void *)addr);

38) sysctl_xframe.conf belongs somewhere in Documentation/*

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: Submission for S2io 10GbE driver
@ 2004-02-17  0:11 Christoph Hellwig
  2004-02-28 15:08 ` Submission #3 " Leonid Grossman
  0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2004-02-17  0:11 UTC (permalink / raw)
  To: Leonid Grossman
  Cc: netdev, 'Andi Kleen', 'Jeff Garzik',
	'Stephen Hemminger', 'Francois Romieu',
	'jamal', 'Grant Grundler',
	'Anton Blanchard', 'Jes Sorensen',
	raghavendra.koushik, 'ravinandan arakali'

A bunch of comments:

 - if you want to submit the driver for inclusion please submit a patch against a kernel tree,
   not a tarball.
 - please try to avoid version ifdefs by provoding the newer APIs on older kernels, e.g.:

#ifndef IRQ_RETVAL
#define irqreturn_t                     void
#define IRQ_RETVAL(foo)
#endif

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,00)
#define free_netdev kfree
#endif

 - your AS_A_MODULE ifdef is bogs - everything under it is fine for a
   non-modular driver, too
 - your XENA_ARCH_64 is not good - just cast 64bit values to
   (unsigned long long) always and use the ll format specifier always.
   You missed a few 64bit arches, btw :)
 - can you get rid of all those BOOL/TRUE/FALSE/SUCCESS/etc.. ifdefs?
 - s2io_driver wants to be converted to C99 initializers (.foo instead of foo:)
 - In Linux comments usually are on the same indentation level as surrounding
   code
 - CONFIGURE_NAPI_SUPPORT should probably become CONFIG_XENA_NAPI or whatever
 - you want to use ethtool_ops instead of the ioctl variant
 - there's lots of non-static symbols in s2io.c - these shouldn't exist in a
   single source-file driver
 - you can't return -ENOMEM from the isr - just IRQ_HANDLED or IRQ_NONE
 - having the RCS log at the end of the source files looks ... odd

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2004-03-13  2:30 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-03-01 13:05 Submission #3 for S2io 10GbE driver raghavendra.koushik
2004-03-01 15:24 ` Leonid Grossman
  -- strict thread matches above, loose matches on Subject: below --
2004-03-02 21:47 Feldman, Scott
2004-03-02 22:21 ` Ben Greear
2004-03-02 21:16 Feldman, Scott
2004-03-02 21:21 ` Jeff Garzik
2004-03-02 21:33   ` Ben Greear
2004-03-02 21:38     ` Jeff Garzik
2004-03-02 13:46 raghavendra.koushik
2004-03-02 18:47 ` Jeff Garzik
2004-03-01  6:21 raghavendra.koushik
2004-03-01  6:53 ` Jeff Garzik
2004-02-17  0:11 Submission " Christoph Hellwig
2004-02-28 15:08 ` Submission #3 " Leonid Grossman
2004-02-28 20:21   ` Jeff Garzik
2004-03-12 21:55     ` ravinandan arakali
2004-03-13  2:30       ` Jeff Garzik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).