| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 09 Sep 2008 14:19:47 +0100
From: Ben Hutchings <bhutchings@...arflare.com>
To: Steve Glendinning <steve.glendinning@...c.com>
Cc: netdev@...r.kernel.org, Ian Saturley <ian.saturley@...c.com>,
Catalin Marinas <catalin.marinas@....com>,
David Brownell <dbrownell@...rs.sourceforge.net>,
linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/1] SMSC LAN9500 USB2.0 10/100 ethernet adapter driver
On Tue, 2008-09-09 at 12:36 +0100, Steve Glendinning wrote:
[...]
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> new file mode 100644
> index 0000000..60ffd90
> --- /dev/null
> +++ b/drivers/net/usb/smsc95xx.c
[...]
> +static int smsc95xx_read_reg(struct usbnet *dev, u32 index, u32 *data)
> +{
> + u32 *buf = kmalloc(4, GFP_KERNEL);
> + int ret;
> +
> + BUG_ON(!dev);
> +
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> + USB_VENDOR_REQUEST_READ_REGISTER,
> + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 00, index, buf, 4, USB_CTRL_GET_TIMEOUT);
> +
> + if (unlikely(ret < 0))
> + SMSC_WARNING("Failed to read register index 0x%08x", index);
> +
> + le32_to_cpus(buf);
> + *data = *buf;
> + kfree(buf);
> +
> + return ret;
> +}
Why are you allocating a buffer on the heap? What's wrong with
static int smsc95xx_read_reg(struct usbnet *dev, u32 index, u32 *data)
{
int ret;
BUG_ON(!dev);
ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
USB_VENDOR_REQUEST_READ_REGISTER,
USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
00, index, data, 4, USB_CTRL_GET_TIMEOUT);
if (unlikely(ret < 0))
SMSC_WARNING("Failed to read register index 0x%08x", index);
le32_to_cpus(data);
return ret;
}
?
Similarly for smsc95xx_write_reg().
> +static void smsc95xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
> + int regval)
> +{
[...]
> + return;
> +}
Don't put an explicit "return" at the end of a void function.
> +static int smsc95xx_eeprom_is_busy(struct usbnet *dev)
> +{
> + u32 val;
> + int i;
> +
> + for (i = 0; i < 1000; i++) {
> + smsc95xx_read_reg(dev, E2P_CMD, &val);
> + if (!(val & E2P_CMD_BUSY_) || (val & E2P_CMD_TIMEOUT_))
> + break;
> + udelay(40);
> + }
> +
> + if (val & (E2P_CMD_TIMEOUT_ | E2P_CMD_BUSY_)) {
> + SMSC_WARNING(KERN_WARNING "EEPROM read operation timeout");
> + return -EIO;
> + }
> +
> + return 0;
> +}
This function name implies that the function tests once and returns a
boolean. It should be named something like smsc95xx_wait_eeprom().
[...]
> +static int smsc95xx_write_eeprom(struct usbnet *dev, u32 offset, u32 length,
> + u8 *data)
> +{
> + u32 val;
> + int i, ret;
> +
> + BUG_ON(!dev);
> + BUG_ON(!data);
> +
> + /* confirm eeprom not busy */
> + for (i = 0; i < 1000; i++) {
> + smsc95xx_read_reg(dev, E2P_CMD, &val);
> + if (!(val & E2P_CMD_BUSY_))
> + break;
> + udelay(40);
> + }
> +
> + if (val & E2P_CMD_BUSY_) {
> + SMSC_WARNING("EEPROM is busy");
> + return -EIO;
> + }
Do you not want to check E2P_CMD_LOADED_ here, as in
smsc95xx_read_eeprom()?
> + /* Issue write/erase enable command */
> + val = E2P_CMD_BUSY_ | E2P_CMD_EWEN_;
> + smsc95xx_write_reg(dev, E2P_CMD, val);
> +
> + ret = smsc95xx_eeprom_is_busy(dev);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < length; i++) {
> +
> + /* Fill data register */
> + val = data[i];
> + smsc95xx_write_reg(dev, E2P_DATA, val);
> +
> + /* Send "write" command */
> + val = E2P_CMD_BUSY_ | E2P_CMD_WRITE_ | (offset & E2P_CMD_ADDR_);
> + smsc95xx_write_reg(dev, E2P_CMD, val);
> +
> + ret = smsc95xx_eeprom_is_busy(dev);
> + if (ret < 0)
> + return ret;
> +
> + offset++;
> + }
> +
> + return 0;
> +}
[...]
> +static void smsc95xx_validate_mac(struct usbnet *dev)
> +{
> + /* try reading mac address from EEPROM */
> + if (smsc95xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
> + dev->net->dev_addr) == 0) {
> + if (is_valid_ether_addr(dev->net->dev_addr)) {
> + /* eeprom values are valid so use them */
> + SMSC_TRACE(DBG_INIT, "Mac Address read from EEPROM");
> + return;
> + }
> + }
> +
> + /* no eeprom, or eeprom values are invalid. generate random MAC */
> + random_ether_addr(dev->net->dev_addr);
> + SMSC_TRACE(DBG_INIT, "MAC Address set to random_ether_addr");
> +}
This function doesn't just validate a MAC address - it reads, validates
and potentially replaces it. It should be named something like
smsc95xx_init_mac_address().
> +static int smsc95xx_reset(struct usbnet *dev)
> +{
[...]
> + smsc95xx_start_tx_path(dev);
If there's an error after this point the TX path is left enabled. Is that safe?
> + /* Init Rx */
> + /* Set Vlan */
> + write_buf = (u32)ETH_P_8021Q;
> + ret = smsc95xx_write_reg(dev, VLAN1, write_buf);
> + if (ret < 0) {
> + SMSC_WARNING("Failed to write VAN1: %d", ret);
> + return ret;
> + }
> +
> + /* Enable or disable Rx checksum offload engine */
> + ret = smsc95xx_set_rx_csum(dev, pdata->use_rx_csum);
> + if (ret < 0) {
> + SMSC_WARNING("Failed to set Rx csum offload: %d", ret);
> + return ret;
> + }
> +
> + smsc95xx_start_rx_path(dev);
[...]
Similarly the RX path is left enabled if there's an error after this
point.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists