Re: [PATCH 1/1] SMSC LAN9500 USB2.0 10/100 ethernet adapter driver

[Ben Hutchings <bhutchings@solarflare.com>]

On Mon, 2008-09-29 at 15:30 +0100, Steve Glendinning wrote:
[...] 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> new file mode 100644
> index 0000000..b7c5fd4
> --- /dev/null
> +++ b/drivers/net/usb/smsc95xx.c
[...] 
> +u32 debug_mode;
> +module_param(debug_mode, uint, 0);
> +MODULE_PARM_DESC(debug_mode, "enable/disable debug messages");

This parameter doesn't appear to be used.

> +int turbo_mode = true;
> +module_param(turbo_mode, bool, 0);
> +MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");

Why is this parameter not exposed through sysfs (perm = 0)?

[...]
> +/* Loop until the read is completed with timeout
> + * called with phy_mutex held */
> +static int smsc95xx_phy_wait_not_busy(struct usbnet *dev)
> +{
> +       int i;
> +       u32 val;
> +
> +       for (i = 0; i < 100; i++) {
> +               smsc95xx_read_reg(dev, MII_ADDR, &val);
> +               if (!(val & MII_BUSY_))
> +                       return 0;
> +               udelay(1);
> +       }
> +
> +       return -EIO;
> +}

Doesn't a register read over USB normally take much longer than 1 us,
meaning that this can wait much longer than 100 us?  If so, please
consider setting and checking a timeout based on jiffies.

There may be a similar issue with EEPROM access.

[...]
> +static int smsc95xx_link_reset(struct usbnet *dev)
> +{
> +	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> +	struct ethtool_cmd ecmd;
> +	unsigned long flags;
> +	u32 intdata;
> +
> +	/* clear interrupt status */
> +	smsc95xx_mdio_read(dev->net, dev->mii.phy_id, PHY_INT_SRC);
> +	intdata = 0xFFFFFFFF;
> +	smsc95xx_write_reg(dev, INT_STS, intdata);
> +
> +	mii_check_media(&dev->mii, 1, 1);
> +	mii_ethtool_gset(&dev->mii, &ecmd);
> +
> +	if (netif_msg_link(dev))
> +		devdbg(dev, "speed: %d duplex: %d", ecmd.speed, ecmd.duplex);
> +
> +	spin_lock_irqsave(&pdata->mac_cr_lock, flags);
> +	if (ecmd.duplex != DUPLEX_FULL) {
> +		pdata->mac_cr &= ~MAC_CR_FDPX_;
> +		pdata->mac_cr |= MAC_CR_RCVOWN_;
> +	} else {
> +		pdata->mac_cr &= ~MAC_CR_RCVOWN_;
> +		pdata->mac_cr |= MAC_CR_FDPX_;
> +	}
> +	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
> +
> +	smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
> +
> +	smsc95xx_phy_update_flowcontrol(dev, &ecmd);

Doesn't this force flow control settings to be whatever we were
advertising, regardless of link partner abilities?

[...]
> +static u32 smsc95xx_ethtool_get_rx_csum(struct net_device *netdev)
> +{
> +	struct usbnet *dev = netdev_priv(netdev);
> +	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> +
> +	return pdata->use_rx_csum ? true : false;
> +}

Delete "? true : false"; it's just silly.

> +static int smsc95xx_ethtool_set_rx_csum(struct net_device *netdev, u32 val)
> +{
> +	struct usbnet *dev = netdev_priv(netdev);
> +	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> +
> +	pdata->use_rx_csum = val ? true : false;

The idiomatic way to write this is !!val.

> +	return smsc95xx_set_rx_csum(dev, pdata->use_rx_csum);
> +}
[...]

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.