Subject: Submission #4 for S2io 10GbE driver

From: "Leonid Grossman" <[email-masked]>
Date: Fri, 19 Mar 2004 20:35:49 -0800

All the issues are addressed, except for the ones that were discussed
and resolved over e-mail.

Best Regards, 
Leonid

> -----Original Message-----
> From: Jeff Garzik [mailto:[email-masked]]
> Sent: Saturday, February 28, 2004 12:22 PM
> To: Leonid Grossman
> Cc: [email-masked]; 'Stephen Hemminger'; 'Christoph 
> Hellwig'; 'ravinandan arakali'; [email-masked]
> 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/*
> 
> 
>