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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 11 Dec 2007 09:06:48 -0800
From:	Stephen Hemminger <shemminger@...ux-foundation.org>
To:	Claudio Lanconelli <lanconelli.claudio@...ar.com>
Cc:	netdev@...r.kernel.org, jgarzik@...ox.com
Subject: Re: [PATCH 1/2] add driver for enc28j60 ethernet chip

On Tue, 11 Dec 2007 15:57:10 +0100
Claudio Lanconelli <lanconelli.claudio@...ar.com> wrote:

> These patches add support for Microchip enc28j60 ethernet chip 
> controlled via SPI.
> I tested it on my custom board (S162) with ARM9 s3c2442 SoC.
> Any comments are welcome.
> 
> Signed-off-by: Claudio Lanconelli <lanconelli.claudio@...ar.com>

General comments:
  * device driver does no carrier detection. This makes it useless
    for bridging, bonding, or any form of failover.

  * use random_ether_addr rather than hardocoding constant mac address

  * use msglevel method (via ethtool) to control debug messages
    rather than kernel configuration. This allows enabling debugging
    without recompilation which is important in distributions.

  * kernel uses u8 rather than uint8_t for byte variables

  * use netdev_priv(netdev) rather than netdev->priv

  * Please add ethtool support

  * Consider using NAPI


Output from checkpatch:
ERROR: do not initialise statics to 0 or NULL
#148: FILE: drivers/net/enc28j60.c:73:
+static int full_duplex = 0;

ERROR: "foo * bar" should be "foo *bar"
#166: FILE: drivers/net/enc28j60.c:91:
+			const uint8_t * data);

ERROR: "foo * bar" should be "foo *bar"
#245: FILE: drivers/net/enc28j60.c:170:
+			 const uint8_t * data)

ERROR: "foo * bar" should be "foo *bar"
#413: FILE: drivers/net/enc28j60.c:338:
+				     uint16_t addr, int len, uint8_t * data)

WARNING: printk() should include KERN_ facility level
#557: FILE: drivers/net/enc28j60.c:482:
+	printk("Cntrl: ECON1 ECON2 ESTAT  EIR  EIE\n");

WARNING: printk() should include KERN_ facility level
#558: FILE: drivers/net/enc28j60.c:483:
+	printk("       0x%02x  ", enc28j60_regb_read(priv, ECON1));

WARNING: printk() should include KERN_ facility level
#564: FILE: drivers/net/enc28j60.c:489:
+	printk("MAC  : MACON1 MACON3 MACON4 MAC-Address\n");

WARNING: printk() should include KERN_ facility level
#565: FILE: drivers/net/enc28j60.c:490:
+	printk("       0x%02x   ", enc28j60_regb_read(priv, MACON1));

WARNING: printk() should include KERN_ facility level
#575: FILE: drivers/net/enc28j60.c:500:
+	printk("Rx   : ERXST  ERXND  ERXWRPT ERXRDPT ERXFCON EPKTCNT MAMXFL\n");

WARNING: printk() should include KERN_ facility level
#576: FILE: drivers/net/enc28j60.c:501:
+	printk("       0x%04x ", enc28j60_regw_read(priv, ERXSTL));

WARNING: printk() should include KERN_ facility level
#584: FILE: drivers/net/enc28j60.c:509:
+	printk("Tx   : ETXST  ETXND  MACLCON1 MACLCON2 MAPHSUP\n");

WARNING: printk() should include KERN_ facility level
#585: FILE: drivers/net/enc28j60.c:510:
+	printk("       0x%04x ", enc28j60_regw_read(priv, ETXSTL));

WARNING: braces {} are not necessary for single statement blocks
#601: FILE: drivers/net/enc28j60.c:526:
+	if ((next_packet_ptr - 1 < start) || (next_packet_ptr - 1 > end)) {
+		erxrdpt = end;
+	} else

WARNING: line over 80 characters
#810: FILE: drivers/net/enc28j60.c:735:
+		 "ExCollision: %d, LateCollision: %d, Giant: %d, Underrun: %d\n",

WARNING: line over 80 characters
#961: FILE: drivers/net/enc28j60.c:886:
+						 "enc28j60 RX, max_pk_cnt: %d\n",

WARNING: line over 80 characters
#965: FILE: drivers/net/enc28j60.c:890:
+			 * don't need to clear interrupt flag, automatically done

WARNING: printk() should include KERN_ facility level
#987: FILE: drivers/net/enc28j60.c:912:
+			printk("\n%04x: ", k);

WARNING: labels should not be indented
#1423: FILE: drivers/net/enc28j60.c:1348:
+      error_register:

WARNING: labels should not be indented
#1425: FILE: drivers/net/enc28j60.c:1350:
+      error_irq:

WARNING: labels should not be indented
#1427: FILE: drivers/net/enc28j60.c:1352:
+      error_buf:

WARNING: labels should not be indented
#1429: FILE: drivers/net/enc28j60.c:1354:
+      error_alloc:

total: 4 errors, 17 warnings, 1703 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

My comments:

diff --git a/drivers/net/enc28j60.c b/drivers/net/enc28j60.c
new file mode 100644
index 0000000..6182473
--- /dev/null
+++ b/drivers/net/enc28j60.c
@@ -0,0 +1,1400 @@
+/*
+ * Microchip ENC28J60 ethernet driver (MAC + PHY)
+ *
+ * Copyright (C) 2007 Eurek srl
+ * Author: Claudio Lanconelli <lanconelli.claudio@...ar.com>
+ * based on enc28j60.c written by David Anders for 2.4 kernel version
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * $Id: enc28j60.c,v 1.10 2007/12/10 16:59:37 claudio Exp $
+ */
+
+#include <linux/autoconf.h>

Use msglvl instead see netdevice.h

+
+#if CONFIG_ENC28J60_DBGLEVEL > 1
+# define VERBOSE_DEBUG
+#endif
+#if CONFIG_ENC28J60_DBGLEVEL > 0
+# define DEBUG
+#endif
+

...
+
+#define MY_TX_TIMEOUT  ((500*HZ)/1000)

That is a really short TX timeout, should be 2 seconds at least not 1/2 sec.
Having it less than a second causes increased wakeups.

+
+/* Max TX retries in case of collision as suggested by errata datasheet */
+#define MAX_TX_RETRYCOUNT	16
+
+/* Driver local data */
+struct enc28j60_net_local {

Rename something shorter like enc28j60_net or just enc28j60?

+	struct net_device_stats stats;

net_device_stats are now in net_device.

+	struct net_device *netdev;
+	struct spi_device *spi;
+	struct semaphore semlock;	/* protect spi_transfer_buf */
Use mutex (or spin_lock) rather than semaphore

+	uint8_t *spi_transfer_buf;
+	struct sk_buff *tx_skb;
+	struct work_struct tx_work;
+	struct work_struct irq_work;

Not sure why you need to have workqueue's for
tx_work and irq_work, rather than using a spin_lock
and doing directly.

+	int bank;			/* current register bank selected */
bank is really unsigned.
	
+	uint16_t next_pk_ptr;		/* next packet pointer within FIFO */
+	int max_pk_counter;		/* statistics: max packet counter */
+	int tx_retry_count;
these are used as unsigned.

+	int hw_enable;
+};
+
+/* Selects Full duplex vs. Half duplex mode */
+static int full_duplex = 0;

Use ethtool for this.

+
+static int enc28j60_send_packet(struct sk_buff *skb, struct net_device *dev);
+static int enc28j60_net_close(struct net_device *dev);
+static struct net_device_stats *enc28j60_net_get_stats(struct net_device *dev);
+static void enc28j60_set_multicast_list(struct net_device *dev);
+static void enc28j60_net_tx_timeout(struct net_device *ndev);
+
+static int enc28j60_chipset_init(struct net_device *dev);
+static void enc28j60_hw_disable(struct enc28j60_net_local *priv);
+static void enc28j60_hw_enable(struct enc28j60_net_local *priv);
+static void enc28j60_hw_rx(struct enc28j60_net_local *priv);
+static void enc28j60_hw_tx(struct enc28j60_net_local *priv);

If you order functions correctly in code, you don't have to waste lots
of space with all these forward declarations.

...

+			const char *msg);
+
+/*
+ * SPI read buffer
+ * wait for the SPI transfer and copy received data to destination
+ */
+static int
+spi_read_buf(struct enc28j60_net_local *priv, int len, uint8_t *data)
+{
+	uint8_t *rx_buf;
+	uint8_t *tx_buf;
+	struct spi_transfer t;
+	struct spi_message msg;
+	int ret, slen;
+
+	slen = 1;
+	memset(&t, 0, sizeof(t));
+	t.tx_buf = tx_buf = priv->spi_transfer_buf;
+	t.rx_buf = rx_buf = priv->spi_transfer_buf + 4;
+	t.len = slen + len;

If you use structure initializer you can avoid having to do
the memset


+
+	down(&priv->semlock);
+	tx_buf[0] = ENC28J60_READ_BUF_MEM;
+	tx_buf[1] = tx_buf[2] = tx_buf[3] = 0;	/* don't care */
+
+	spi_message_init(&msg);
+	spi_message_add_tail(&t, &msg);
+	ret = spi_sync(priv->spi, &msg);
+	if (ret == 0) {
+		memcpy(data, &rx_buf[slen], len);
+		ret = msg.status;
+	}
+	up(&priv->semlock);
+	if (ret != 0)
+		dev_dbg(&priv->netdev->dev, "%s: failed: ret = %d\n",
+			__FUNCTION__, ret);
+
+	return ret;
+}

...

+/*
+ * Register word read
+ */
+static inline int enc28j60_regw_read(struct enc28j60_net_local *priv,
+				     uint8_t address)
+{

I wouldn't bother marking these as "inline" since the compiler
will decide to inline in most cases.  By telling the compiler to
inline it may generate bigger/slower code.


+	int rl, rh;
+
+	enc28j60_set_bank(priv, address);
+	rl = spi_read_op(priv, ENC28J60_READ_CTRL_REG, address);
+	rh = spi_read_op(priv, ENC28J60_READ_CTRL_REG, address + 1);
+
+	return (rh << 8) | rl;
+}

...

+/*
+ * Program the hardware MAC address from dev->dev_addr.
+ */
+static void enc28j60_set_hw_macaddr(struct enc28j60_net_local *priv)
+{
+	struct net_device *ndev = priv->netdev;
+
+	if (!priv->hw_enable) {
+		/* NOTE: MAC address in ENC28J60 is byte-backward */
+		enc28j60_regb_write(priv, MAADR5, ndev->dev_addr[0]);
+		enc28j60_regb_write(priv, MAADR4, ndev->dev_addr[1]);
+		enc28j60_regb_write(priv, MAADR3, ndev->dev_addr[2]);
+		enc28j60_regb_write(priv, MAADR2, ndev->dev_addr[3]);
+		enc28j60_regb_write(priv, MAADR1, ndev->dev_addr[4]);
+		enc28j60_regb_write(priv, MAADR0, ndev->dev_addr[5]);
+
+		dev_dbg(&ndev->dev,
+			"%s() [%s] Setting MAC address to "
+			"%02x:%02x:%02x:%02x:%02x:%02x\n",
+			__FUNCTION__, ndev->name, ndev->dev_addr[0],
+			ndev->dev_addr[1], ndev->dev_addr[2], ndev->dev_addr[3],
+			ndev->dev_addr[4], ndev->dev_addr[5]);
+	} else
+		dev_dbg(&ndev->dev,
+			"%s() Warning: hw must be disabled to set hw "
+			"Mac address\n", __FUNCTION__);

Should return -EINVAL/-EBUSY/... instead of printing message.

+}
+
+/*
+ * Store the new hardware address in dev->dev_addr, and update the MAC.
+ */
+static int enc28j60_set_mac_address(struct net_device *dev, void *addr)
+{
+	struct sockaddr *address = addr;
+	struct enc28j60_net_local *priv = netdev_priv(dev);
+
+	if (!is_valid_ether_addr(address->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(dev->dev_addr, address->sa_data, dev->addr_len);
+	enc28j60_set_hw_macaddr(priv);
+
+	return 0;
+}
...

+
+/*
+ * Get the current statistics.
+ * This may be called with the card open or closed.
+ */
+static struct net_device_stats *enc28j60_net_get_stats(struct net_device *dev)
+{
+	struct enc28j60_net_local *priv = netdev_priv(dev);
+
+	return &priv->stats;
+}

If you use dev->stats, then you don't need your own get_stats function.


...

+static int enc28j60_hw_init(struct enc28j60_net_local *priv)
+{
+	uint8_t reg;
+
+	dev_dbg(&priv->spi->dev, "%s() - %s\n",
+		__FUNCTION__, full_duplex ? "FullDuplex" : "HalfDuplex");
+	/* first soft reset the chip */
+	enc28j60_soft_reset(priv);
+
+	dev_vdbg(&priv->spi->dev, "%s() bank0\n", __FUNCTION__);
+
+	/* Clear ECON1 */
+	spi_write_op(priv, ENC28J60_WRITE_CTRL_REG, ECON1, 0x00);
+	priv->bank = 0;
+	priv->hw_enable = 0;
+	priv->tx_retry_count = 0;
+
+	enc28j60_regb_write(priv, ECON2, ECON2_AUTOINC);
+	enc28j60_rxfifo_init(priv, RXSTART_INIT, RXEND_INIT);
+	enc28j60_txfifo_init(priv, TXSTART_INIT, TXEND_INIT);
+
+	/*
+	 * Check the RevID.
+	 * If it's 0x00 or 0xFF probably the enc28j60 is not mounted or
+	 * damaged
+	 */
+	reg = enc28j60_regb_read(priv, EREVID);
+	if (reg == 0x00 || reg == 0xff)
+		return 0;
+
+	dev_vdbg(&priv->spi->dev, "%s() bank1\n", __FUNCTION__);
+
+	/* default filter mode: (unicast OR broadcast) AND crc valid */
+	enc28j60_regb_write(priv, ERXFCON,
+			    ERXFCON_UCEN | ERXFCON_CRCEN | ERXFCON_BCEN);
+
+	dev_vdbg(&priv->spi->dev, "%s() bank2\n", __FUNCTION__);
+	/* enable MAC receive */
+	enc28j60_regb_write(priv, MACON1,
+			    MACON1_MARXEN | MACON1_TXPAUS | MACON1_RXPAUS);
+	/* enable automatic padding and CRC operations */
+	if (full_duplex) {
+		enc28j60_regb_write(priv, MACON3,
+				    MACON3_PADCFG0 | MACON3_TXCRCEN |
+				    MACON3_FRMLNEN | MACON3_FULDPX);
+		/* set inter-frame gap (non-back-to-back) */
+		enc28j60_regb_write(priv, MAIPGL, 0x12);
+		/* set inter-frame gap (back-to-back) */
+		enc28j60_regb_write(priv, MABBIPG, 0x15);
+	} else {
+		enc28j60_regb_write(priv, MACON3,
+				    MACON3_PADCFG0 | MACON3_TXCRCEN |
+				    MACON3_FRMLNEN);
+		enc28j60_regb_write(priv, MACON4, 1 << 6);	/* DEFER bit */
+		/* set inter-frame gap (non-back-to-back) */
+		enc28j60_regw_write(priv, MAIPGL, 0x0C12);
+		/* set inter-frame gap (back-to-back) */
+		enc28j60_regb_write(priv, MABBIPG, 0x12);
+	}
+	/*
+	 * MACLCON1 (default)
+	 * MACLCON2 (default)
+	 * Set the maximum packet size which the controller will accept
+	 */
+	enc28j60_regw_write(priv, MAMXFLL, MAX_FRAMELEN);
+
+	dev_vdbg(&priv->spi->dev, "%s() bank3\n", __FUNCTION__);
+	/* NOTE: MAC address in ENC28J60 is byte-backward */
+	enc28j60_regb_write(priv, MAADR5, ENC28J60_MAC0);
+	enc28j60_regb_write(priv, MAADR4, ENC28J60_MAC1);
+	enc28j60_regb_write(priv, MAADR3, ENC28J60_MAC2);
+	enc28j60_regb_write(priv, MAADR2, ENC28J60_MAC3);
+	enc28j60_regb_write(priv, MAADR1, ENC28J60_MAC4);
+	enc28j60_regb_write(priv, MAADR0, ENC28J60_MAC5);

Rather than having same address, please use random_ether_addr()
to avoid problems with two devices with same ethernet address.

...

+static int __devinit enc28j60_probe(struct spi_device *spi)
+{
+	struct net_device *dev;
+	struct enc28j60_net_local *priv;
+	int ret = 0;
+
+	dev_dbg(&spi->dev, "%s() start\n", __FUNCTION__);
+
+	dev = alloc_etherdev(sizeof(struct enc28j60_net_local));
+	if (!dev) {
+		ret = -ENOMEM;
+		goto error_alloc;
+	}
+	priv = netdev_priv(dev);
+
+	priv->netdev = dev;	/* priv to netdev reference */
+	priv->spi = spi;	/* priv to spi reference */
+	priv->spi_transfer_buf = kmalloc(SPI_TRANSFER_BUF_LEN, GFP_KERNEL);

Why not declare the transfer buffer as an array in spi?

+	if (!priv->spi_transfer_buf) {
+		ret = -ENOMEM;
+		goto error_buf;
+	}
+	init_MUTEX(&priv->semlock);
+
+	INIT_WORK(&priv->tx_work, enc28j60_tx_work_handler);
+	INIT_WORK(&priv->irq_work, enc28j60_irq_work_handler);
+	dev_set_drvdata(&spi->dev, priv);	/* spi to priv reference */
+	SET_NETDEV_DEV(dev, &spi->dev);


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