| 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: Thu, 6 Jan 2011 15:56:47 +0100
From: Michał Mirosław <mirqus@...il.com>
To: Balaji Venkatachalam <balaji.v@...takaa.com>
Cc: netdev@...r.kernel.org, mohan@...takaa.com, blue.cube@...nam.cz,
lanconelli.claudio@...ar.com,
Sriram Subramanian <sriram@...takaa.com>
Subject: Re: [PATCH]netdev: add driver for enc424j600 ethernet chip on SPI bus
A quick look follows.
2011/1/6 Balaji Venkatachalam <balaji.v@...takaa.com>:
> From: Balaji Venkatachalam <balaji.v@...takaa.com>
>
> These patches add support for Microchip enc424j600 ethernet chip
> controlled via SPI.
>
> I tested it on my custom board with ARM9 (Freescale i.MX233) with
> Kernel 2.6.31.14.
> Any comments are welcome.
>
> Signed-off-by: Balaji Venkatachalam <balaji.v@...takaa.com>
> ---
>
> diff -uprN -X a/Documentation/dontdiff a/drivers/net/enc424j600.c
> b/drivers/net/enc424j600.c
> --- a/drivers/net/enc424j600.c 1970-01-01 05:30:00.000000000 +0530
> +++ b/drivers/net/enc424j600.c 2011-01-06 09:39:17.000000000 +0530
> @@ -0,0 +1,1694 @@
[...]
> +static int enc424j600_spi_trans(struct enc424j600_net *priv, int len)
> +{
> + /*modified to suit half duplexed spi */
> + struct spi_transfer tt = {
> + .tx_buf = priv->spi_tx_buf,
> + .len = SPI_OPLEN,
> + };
> + struct spi_transfer tr = {
> + .rx_buf = priv->spi_rx_buf,
> + .len = len,
> + };
> + struct spi_message m;
> + int ret;
> +
> + spi_message_init(&m);
> +
> + spi_message_add_tail(&tt, &m);
> + spi_message_add_tail(&tr, &m);
> +
> + ret = spi_sync(priv->spi, &m);
> +
> + if (ret == 0)
> + memcpy(priv->spi_rx_buf, tr.rx_buf, len);
> +
> + if (ret)
> + dev_err(&priv->spi->dev,
> + "spi transfer failed: ret = %d\n", ret);
> + return ret;
> +}
> +
> +/*
> + * Read data from chip SRAM.
> + * window = 0 for Receive Buffer
> + * = 1 for User Defined area
> + * = 2 for General Purpose area
> + */
This could use an enum. And the comment does not match the code, BTW.
> +static int enc424j600_read_sram(struct enc424j600_net *priv,
> + u8 *dst, int len, u16 srcaddr, int window)
> +{
> + int ret;
> +
> + if (len > SPI_TRANSFER_BUF_LEN - 1 || len <= 0)
> + return -EINVAL;
> +
> + /* First set the write pointer as per selected window */
> + if (window == 1)
> + priv->spi_tx_buf[0] = WRXRDPT;
> + else if (window == 2)
> + priv->spi_tx_buf[0] = WUDARDPT;
> + else if (window == 3)
> + priv->spi_tx_buf[0] = WGPRDPT;
> +
> + priv->spi_tx_buf[1] = srcaddr & 0xFF;
> + priv->spi_tx_buf[2] = srcaddr >> 8;
> + ret = spi_write(priv->spi, priv->spi_tx_buf, 3);
> +
> + /* Transfer the data */
> + if (window == 1)
> + priv->spi_tx_buf[0] = RRXDATA;
> + else if (window == 2)
> + priv->spi_tx_buf[0] = RUDADATA;
> + else if (window == 3)
> + priv->spi_tx_buf[0] = RGPDATA;
> +
> + ret = enc424j600_spi_trans(priv, len + 1); /*READ*/
> +
> + /* Copy the data to the tx buffer */
> + memcpy(dst, &priv->spi_rx_buf[0], len);
You could avoid the copy if enc424j600_spi_trans() would get the
buffer as its arguments.
> +
> + return ret;
> +}
> +
> +/*
> + * Write data to chip SRAM.
> + * window = 1 for RX
> + * window = 2 for User Data
> + * window = 3 for GP
> + */
Enum.
> +static int enc424j600_write_sram(struct enc424j600_net *priv,
> + const u8 *src, int len, u16 dstaddr,
> + int window)
> +{
> + int ret;
> +
> + if (len > SPI_TRANSFER_BUF_LEN - 1 || len <= 0)
> + return -EINVAL;
> +
> + /* First set the general purpose write pointer */
> + if (window == 1)
> + priv->spi_tx_buf[0] = WRXWRPT;
> + else if (window == 2)
> + priv->spi_tx_buf[0] = WUDAWRPT;
> + else if (window == 3)
> + priv->spi_tx_buf[0] = WGPWRPT;
> +
> + priv->spi_tx_buf[1] = dstaddr & 0xFF;
> + priv->spi_tx_buf[2] = dstaddr >> 8;
> + ret = spi_write(priv->spi, priv->spi_tx_buf, 3);
> +
> + /* Copy the data to the tx buffer */
> + memcpy(&priv->spi_tx_buf[1], src, len);
> +
> + /* Transfer the data */
> + if (window == 1)
> + priv->spi_tx_buf[0] = WRXDATA;
> + else if (window == 2)
> + priv->spi_tx_buf[0] = WUDADATA;
> + else if (window == 3)
> + priv->spi_tx_buf[0] = WGPDATA;
> +
> + ret = spi_write(priv->spi, priv->spi_tx_buf, len + 1);
> +
> + return ret;
> +}
> +
[...]
> +/*
> + * Write a 16bit special function register.
> + * The @sfr parameters takes address of the low byte of the register.
> + * Takes care of the endiannes & buffers.
> + * Uses banked write instruction.
> + */
> +
> +static int enc424j600_write_16b_sfr(struct enc424j600_net *priv,
> + u8 sfr, u16 data)
> +{
> + int ret;
> + enc424j600_set_bank(priv, sfr);
> +
> + priv->spi_tx_buf[0] = WCR(sfr & ADDR_MASK);
> + priv->spi_tx_buf[1] = data & 0xFF;
> + priv->spi_tx_buf[2] = data >> 8;
> + ret = spi_write(priv->spi, priv->spi_tx_buf, 3);
> + if (ret && netif_msg_drv(priv))
> + printk(KERN_DEBUG DRV_NAME ": %s() failed: ret = %d\n",
> + __func__, ret);
netif_dbg()
It's in 2.6.35 or later, though.
> +
> + return ret;
> +}
> +
> +/*
> + * Read a 16bit special function register.
> + * The @sfr parameters takes address of the low byte of the register.
> + * Takes care of the endiannes & buffers.
> + * Uses banked read instruction.
> + */
> +static int enc424j600_read_16b_sfr(struct enc424j600_net *priv,
> + u8 sfr, u16 *data)
> +{
> + int ret;
> + enc424j600_set_bank(priv, sfr);
> +
> + priv->spi_tx_buf[0] = RCR(sfr & ADDR_MASK);
> + priv->spi_tx_buf[1] = 0;
> + priv->spi_tx_buf[2] = 0;
> + priv->spi_tx_buf[3] = 0;
> + ret = enc424j600_spi_trans(priv, 3); /*READ*/
> +
> + *data = priv->spi_rx_buf[0] | priv->spi_rx_buf[1] << (u16) 8;
> +
> + return ret;
> +}
> +
> +/*
> + * Reset the enc424j600.
> + * TODO: What if we get stuck on non-working spi with the initial
> + * test access to EUDAST ?
> + */
> +static void enc424j600_soft_reset(struct enc424j600_net *priv)
> +{
> + u8 estath;
> + u16 eudast;
> + if (netif_msg_hw(priv))
> + printk(KERN_DEBUG DRV_NAME ": %s() enter\n", __func__);
> +
> + do {
> + enc424j600_write_16b_sfr(priv, EUDASTL, EUDAST_TEST_VAL);
> + enc424j600_read_16b_sfr(priv, EUDASTL, &eudast);
> + } while (eudast != EUDAST_TEST_VAL);
> +
> + do {
> + enc424j600_read_8b_sfr(priv, ESTATH, &estath);
> + } while (!estath & CLKRDY);
Those loops need a timeout or you risk lockup with broken hardware.
> +
> + priv->spi_tx_buf[0] = SETETHRST;
> + enc424j600_spi_trans(priv, 1);
> +
> + udelay(50);
> +
> + enc424j600_read_16b_sfr(priv, EUDASTL, &eudast);
> + if (netif_msg_hw(priv) && eudast != 0)
> + printk(KERN_DEBUG DRV_NAME
> + ": %s() EUDASTL is not zero!\n", __func__);
> +
> + udelay(500);
Long enough for usleep() if there are no locks held.
> +}
> +
> +static unsigned long msec20_to_jiffies;
== HZ/50. It's constant AFAIR.
> +
> +/*
> + * Wait for bits in register to become equal to @readyMask, but at most 20ms.
> + */
> +static int poll_ready(struct enc424j600_net *priv,
> + u8 reg, u8 mask, u8 readyMask)
> +{
> + unsigned long timeout = jiffies + msec20_to_jiffies;
> + u8 value;
> + /* 20 msec timeout read */
> + enc424j600_read_8b_sfr(priv, reg, &value);
> + while ((value & mask) != readyMask) {
> + if (time_after(jiffies, timeout)) {
> + if (netif_msg_drv(priv))
> + dev_dbg(&priv->spi->dev,
> + "reg %02x ready timeout!\n", reg);
> + return -ETIMEDOUT;
> + }
> + cpu_relax();
> + enc424j600_read_8b_sfr(priv, reg, &value);
> + }
> +
> + return 0;
> +}
> +
[...]
> +/* Waits for autonegotiation to complete and sets FULDPX bit in macon2. */
> +static void enc424j600_wait_for_autoneg(struct enc424j600_net *priv)
> +{
> + u16 phstat1;
> + do {
> + enc424j600_phy_read(priv, PHSTAT1, &phstat1);
> + } while (!(phstat1 & ANDONE));
Timeout?
[...]
> +static int
> +enc424j600_setlink(struct net_device *ndev, u8 autoneg, u16 speed, u8 duplex)
> +{
> + struct enc424j600_net *priv = netdev_priv(ndev);
> + int ret = 0;
> + if (!priv->hw_enable) {
> + /* link is in low power mode now; duplex setting
> + * will take effect on next enc424j600_hw_init().
> + */
> + if (speed == SPEED_10 || speed == SPEED_100) {
> + priv->autoneg = (autoneg == AUTONEG_ENABLE);
> + priv->full_duplex = (duplex == DUPLEX_FULL);
> + priv->speed100 = (speed == SPEED_100);
> + } else {
> + if (netif_msg_link(priv))
> + dev_warn(&ndev->dev,
> + "unsupported link setting\n");
> + ret = -EOPNOTSUPP;
EINVAL
> + }
> + } else {
> + if (netif_msg_link(priv))
> + dev_warn(&ndev->dev, "Warning: hw must be disabled "
> + "to set link mode\n");
> + ret = -EBUSY;
> + }
> + return ret;
> +}
[...]
> +
> +/*
> + * Hardware receive function.
> + * Read the buffer memory, update the FIFO pointer to free the buffer,
> + * check the status vector and decrement the packet counter.
> + */
> +static void enc424j600_hw_rx(struct net_device *ndev)
> +{
> + struct enc424j600_net *priv = netdev_priv(ndev);
> + struct sk_buff *skb = NULL;
> + u16 erxrdpt, next_packet, rxstat;
> + u8 pkcnt;
> + u16 head, tail;
> + u8 rsv[RSV_SIZE];
> + u16 newrxtail;
> + int len;
> +
> + if (netif_msg_rx_status(priv))
> + printk(KERN_DEBUG DRV_NAME ": RX pk_addr:0x%04x\n",
> + priv->next_pk_ptr);
> + if (unlikely(priv->next_pk_ptr > RXEND_INIT)) {
> + if (netif_msg_rx_err(priv))
> + dev_err(&ndev->dev,
> + "%s() Invalid packet address!! 0x%04x\n",
> + __func__, priv->next_pk_ptr);
> + mutex_lock(&priv->lock);
> + enc424j600_clear_bits(priv, ECON1L, RXEN);
> + enc424j600_set_bits(priv, ECON2L, RXRST);
> + enc424j600_clear_bits(priv, ECON2L, RXRST);
> + nolock_rxfifo_init(priv, RXSTART, RXEND_INIT);
> + enc424j600_clear_bits(priv, EIRL, RXABTIF);
> + enc424j600_set_bits(priv, ECON1L, RXEN);
> + mutex_unlock(&priv->lock);
> + ndev->stats.rx_errors++;
> + return;
> + }
> +
> + /* Read next packet pointer and rx status vector */
> + enc424j600_read_sram(priv, rsv, sizeof(rsv), priv->next_pk_ptr, 1);
> +
> + next_packet = rsv[1];
> + next_packet <<= 8;
> + next_packet |= rsv[0];
> +
> + len = rsv[3];
> + len <<= 8;
> + len |= rsv[2];
> +
> + rxstat = rsv[5];
> + rxstat <<= 8;
> + rxstat |= rsv[4];
> +
> + if (netif_msg_rx_status(priv))
> + enc424j600_dump_rsv(priv, __func__, next_packet, len, rxstat);
> +
> + if (!RSV_GETBIT(rxstat, RSV_RXOK) || len > MAX_FRAMELEN) {
> + if (netif_msg_rx_err(priv))
> + dev_err(&ndev->dev, "Rx Error (%04x)\n", rxstat);
> + ndev->stats.rx_errors++;
> + if (RSV_GETBIT(rxstat, RSV_CRCERROR))
> + ndev->stats.rx_crc_errors++;
> + if (RSV_GETBIT(rxstat, RSV_LENCHECKERR))
> + ndev->stats.rx_frame_errors++;
> + if (len > MAX_FRAMELEN)
> + ndev->stats.rx_over_errors++;
> + } else {
> + skb = dev_alloc_skb(len + NET_IP_ALIGN);
> + if (!skb) {
> + if (netif_msg_rx_err(priv))
> + dev_err(&ndev->dev,
> + "out of memory for Rx'd frame\n");
> + ndev->stats.rx_dropped++;
> + } else {
> + skb->dev = ndev;
> + skb_reserve(skb, NET_IP_ALIGN);
> +
> + /* copy the packet from the receive buffer */
> + enc424j600_read_sram(priv, skb_put(skb, len), len,
> + rx_packet_start(priv->next_pk_ptr),
> + 1);
> +
> + if (netif_msg_pktdata(priv))
> + dump_packet(__func__, skb->len, skb->data);
> + skb->protocol = eth_type_trans(skb, ndev);
> + /* update statistics */
> + ndev->stats.rx_packets++;
> + ndev->stats.rx_bytes += len;
> + netif_rx_ni(skb);
I assume that this is not an IRQ context because of (slow) SPI
accesses. So use netif_rx() or better netif_receive_skb().
> + }
> + }
> + newrxtail = next_packet - 2;
> + if (next_packet == RXSTART)
> + newrxtail = SRAMSIZE - 2;
> +
> + enc424j600_write_16b_sfr(priv, ERXTAILL, newrxtail);
> + /*
> + * Move the RX read pointer to the start of the next
> + * received packet.
> + * This frees the memory we just read out
> + */
> + erxrdpt = erxrdpt_workaround(next_packet, RXSTART, RXEND_INIT);
> + if (netif_msg_hw(priv))
> + printk(KERN_DEBUG DRV_NAME ": %s() ERXRDPT:0x%04x\n", __func__,
> + erxrdpt);
> +
> + mutex_lock(&priv->lock);
Why do you need the lock here, but not on previous accesses to the hardware?
> + enc424j600_write_16b_sfr(priv, ERXRDPTL, erxrdpt);
> +
> +#ifdef CONFIG_ENC28J60_WRITEVERIFY
> + if (netif_msg_drv(priv)) {
> + u16 reg;
> +
> + enc424j600_read_16b_sfr(priv, ERXRDPTL, ®);
> + if (reg != erxrdpt)
> + printk(KERN_DEBUG DRV_NAME ": %s() ERXRDPT verify "
> + "error (0x%04x - 0x%04x)\n", __func__, reg,
> + erxrdpt);
> + }
> +#endif
> +
> + priv->next_pk_ptr = next_packet;
> + enc424j600_read_8b_sfr(priv, ESTATL, &pkcnt);
> + enc424j600_read_16b_sfr(priv, ERXHEADL, &head);
> + enc424j600_read_16b_sfr(priv, ERXTAILL, &tail);
> + /* we are done with this packet, decrement the packet counter */
> + enc424j600_set_bits(priv, ECON1H, PKTDEC);
> +
> + mutex_unlock(&priv->lock);
> +}
[...]
> +static irqreturn_t enc424j600_irq(int irq, void *dev_id)
> +{
> + struct enc424j600_net *priv = dev_id;
> + /*
> + * Can't do anything in interrupt context because we need to
> + * block (spi_sync() is blocking) so fire of the interrupt
> + * handling workqueue.
> + * Remember that we access enc424j600 registers through SPI bus
> + * via spi_sync() call.
> + */
> + schedule_work(&priv->irq_work);
> +
> + return IRQ_HANDLED;
> +}
You can use threaded interrupts here and get rid of irq_work. And
definitely disable source interrupt before returning. And after you do
that, you have implemented most of what's necessary for NAPI rx.
[...]
> +static void enc424j600_setrx_work_handler(struct work_struct *work)
> +{
> + u16 macon1;
> + struct enc424j600_net *priv =
> + container_of(work, struct enc424j600_net, setrx_work);
> +
> + if (priv->rxfilter == RXFILTER_PROMISC) {
> + if (netif_msg_drv(priv))
> + printk(KERN_DEBUG DRV_NAME ": promiscuous mode\n");
> + enc424j600_read_16b_sfr(priv, MACON1L, &macon1);
> + macon1 = macon1 | PASSALL;
> + enc424j600_write_16b_sfr(priv, MACON1L, macon1);
> + locked_regb_write(priv, ERXFCONL, UCEN | MCEN | NOTMEEN);
> + } else if (priv->rxfilter == RXFILTER_MULTI) {
> + if (netif_msg_drv(priv))
> + printk(KERN_DEBUG DRV_NAME ": multicast mode\n");
> + locked_regb_write(priv, ERXFCONL, UCEN | CRCEN | BCEN | MCEN);
> +
> + } else {
> + if (netif_msg_drv(priv))
> + printk(KERN_DEBUG DRV_NAME ": normal mode\n");
> + locked_regb_write(priv, ERXFCONL, UCEN | CRCEN | BCEN);
> +
> + }
> +}
> +
> +static void enc424j600_restart_work_handler(struct work_struct *work)
> +{
> + struct enc424j600_net *priv =
> + container_of(work, struct enc424j600_net, restart_work);
> + struct net_device *ndev = priv->netdev;
> + int ret;
> +
> + rtnl_lock();
> + if (netif_running(ndev)) {
> + enc424j600_net_close(ndev);
> + ret = enc424j600_net_open(ndev);
> + if (unlikely(ret)) {
> + dev_info(&ndev->dev, " could not restart %d\n", ret);
> + dev_close(ndev);
> + }
> + }
> + rtnl_unlock();
> +}
Are these work handlers guaranteed to not be scheduled concurrently?
[...]
> + /* If requested, allocate DMA buffers */
> + if (enc424j600_enable_dma) {
> + spi->dev.coherent_dma_mask = ~0;
> +
> + /*
> + * Minimum coherent DMA allocation is PAGE_SIZE, so allocate
> + * that much and share it between Tx and Rx DMA buffers.
> + */
> +#if SPI_TRANSFER_BUF_LEN > PAGE_SIZE / 2
> +#error "A problem in DMA buffer allocation"
> +#endif
> + priv->spi_tx_buf = dma_alloc_coherent(&spi->dev,
> + PAGE_SIZE,
> + &priv->spi_tx_dma,
> + GFP_DMA);
> +
> + if (priv->spi_tx_buf) {
> + priv->spi_rx_buf = (u8 *) (priv->spi_tx_buf +
> + (PAGE_SIZE / 2));
> + priv->spi_rx_dma = (dma_addr_t) (priv->spi_tx_dma +
> + (PAGE_SIZE / 2));
> + } else {
> + /* Fall back to non-DMA */
> + enc424j600_enable_dma = 0;
> + }
> + }
You don't use DMA addresses anywhere. And don't do anything with this
flag except for freeing the buffers. You should use streaming DMA
calls for skb data to avoid copying anyway and let the arch code care
about bounce buffers if needed.
[...]
MAINTAINTERS entry would be nice also.
Best Regards,
Michał Mirosław
--
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