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:jgarzik@xxxxxxxxx]
> Sent: Saturday, February 28, 2004 12:22 PM
> To: Leonid Grossman
> Cc: netdev@xxxxxxxxxxx; 'Stephen Hemminger'; 'Christoph
> Hellwig'; 'ravinandan arakali'; raghavendra.koushik@xxxxxxxx
> 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/*
>
>
>
s2ioDriver.patch
Description: Binary data
|