[PATCH]NET/KS8695: add support NAPI for Rx

[PATCH]NET/KS8695: add support NAPI for Rx

[Figo.zhang]

Add support NAPI Rx API for KS8695NET driver.

Signed-off-by: Figo.zhang <figo1802@gmail.com>
--- 
drivers/net/arm/ks8695net.c |  165 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 165 insertions(+), 0 deletions(-)

diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
index 2a7b774..7f9d4bd 100644
--- a/drivers/net/arm/ks8695net.c
+++ b/drivers/net/arm/ks8695net.c
@@ -35,12 +35,16 @@
 
 #include <mach/regs-switch.h>
 #include <mach/regs-misc.h>
+#include <asm/mach/irq.h>
+#include <mach/regs-irq.h>
 
 #include "ks8695net.h"
 
 #define MODULENAME	"ks8695_ether"
 #define MODULEVERSION	"1.01"
 
+#define KS8695NET_NAPI  1
+
 /*
  * Transmit and device reset timeout, default 5 seconds.
  */
@@ -152,6 +156,10 @@ struct ks8695_priv {
 	enum ks8695_dtype dtype;
 	void __iomem *io_regs;
 
+	#ifdef KS8695NET_NAPI
+	struct napi_struct	napi;
+	#endif
+
 	const char *rx_irq_name, *tx_irq_name, *link_irq_name;
 	int rx_irq, tx_irq, link_irq;
 
@@ -172,6 +180,7 @@ struct ks8695_priv {
 	dma_addr_t rx_ring_dma;
 	struct ks8695_skbuff rx_buffers[MAX_RX_DESC];
 	int next_rx_desc_read;
+	spinlock_t rx_lock;
 
 	int msg_enable;
 };
@@ -391,6 +400,155 @@ ks8695_tx_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+#ifdef KS8695NET_NAPI
+static irqreturn_t
+ks8695_rx_irq(int irq, void *dev_id)
+{
+	struct net_device *ndev = (struct net_device *)dev_id;
+	struct ks8695_priv *ksp = netdev_priv(ndev);
+	unsigned long status;
+
+	unsigned long mask_bit = 1 << ksp->rx_irq;
+
+	spin_lock(&ksp->rx_lock);
+
+	status = __raw_readl(KS8695_IRQ_VA + KS8695_INTST);
+
+	/*clean rx status bit*/
+	__raw_writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
+
+	if (status & mask_bit) {
+		if (napi_schedule_prep(&ksp->napi)) {
+			/*disable rx interrupt*/
+			status &= ~mask_bit;
+			__raw_writel(status , KS8695_IRQ_VA + KS8695_INTEN);
+			__napi_schedule(&ksp->napi);
+		}
+	}
+
+	spin_unlock(&ksp->rx_lock);
+	return IRQ_HANDLED;
+}
+
+static int ks8695_rx(struct net_device *ndev, int budget)
+{
+	struct ks8695_priv *ksp = netdev_priv(ndev);
+	struct sk_buff *skb;
+	int buff_n;
+	u32 flags;
+	int pktlen;
+	int last_rx_processed = -1;
+	int received = 0;
+
+	buff_n = ksp->next_rx_desc_read;
+	while (netif_running(ndev) && received < budget
+			&& ksp->rx_buffers[buff_n].skb
+			&& (!(ksp->rx_ring[buff_n].status &
+					cpu_to_le32(RDES_OWN)))) {
+			rmb();
+			flags = le32_to_cpu(ksp->rx_ring[buff_n].status);
+			/* Found an SKB which we own, this means we
+			 * received a packet
+			 */
+			if ((flags & (RDES_FS | RDES_LS)) !=
+			    (RDES_FS | RDES_LS)) {
+				/* This packet is not the first and
+				 * the last segment.  Therefore it is
+				 * a "spanning" packet and we can't
+				 * handle it
+				 */
+				goto rx_failure;
+			}
+
+			if (flags & (RDES_ES | RDES_RE)) {
+				/* It's an error packet */
+				ndev->stats.rx_errors++;
+				if (flags & RDES_TL)
+					ndev->stats.rx_length_errors++;
+				if (flags & RDES_RF)
+					ndev->stats.rx_length_errors++;
+				if (flags & RDES_CE)
+					ndev->stats.rx_crc_errors++;
+				if (flags & RDES_RE)
+					ndev->stats.rx_missed_errors++;
+
+				goto rx_failure;
+			}
+
+			pktlen = flags & RDES_FLEN;
+			pktlen -= 4; /* Drop the CRC */
+
+			/* Retrieve the sk_buff */
+			skb = ksp->rx_buffers[buff_n].skb;
+
+			/* Clear it from the ring */
+			ksp->rx_buffers[buff_n].skb = NULL;
+			ksp->rx_ring[buff_n].data_ptr = 0;
+
+			/* Unmap the SKB */
+			dma_unmap_single(ksp->dev,
+					 ksp->rx_buffers[buff_n].dma_ptr,
+					 ksp->rx_buffers[buff_n].length,
+					 DMA_FROM_DEVICE);
+
+			/* Relinquish the SKB to the network layer */
+			skb_put(skb, pktlen);
+			skb->protocol = eth_type_trans(skb, ndev);
+			netif_receive_skb(skb);
+
+			/* Record stats */
+			ndev->stats.rx_packets++;
+			ndev->stats.rx_bytes += pktlen;
+			goto rx_finished;
+
+rx_failure:
+			/* This ring entry is an error, but we can
+			 * re-use the skb
+			 */
+			/* Give the ring entry back to the hardware */
+			ksp->rx_ring[buff_n].status = cpu_to_le32(RDES_OWN);
+rx_finished:
+			received++;
+			/* And note this as processed so we can start
+			 * from here next time
+			 */
+			last_rx_processed = buff_n;
+			buff_n = (buff_n + 1) & MAX_RX_DESC_MASK;
+			/*And note which RX descriptor we last did */
+			if (likely(last_rx_processed != -1))
+				ksp->next_rx_desc_read =
+					(last_rx_processed + 1) &
+					MAX_RX_DESC_MASK;
+
+			/* And refill the buffers */
+			ks8695_refill_rxbuffers(ksp);
+	}
+	return received;
+}
+
+static int ks8695_poll(struct napi_struct *napi, int budget)
+{
+	struct ks8695_priv *ksp = container_of(napi, struct ks8695_priv, napi);
+	struct net_device *dev = ksp->ndev;
+	unsigned long mask_bit = 1 << ksp->rx_irq;
+	unsigned long isr = __raw_readl(KS8695_IRQ_VA + KS8695_INTEN);
+
+	unsigned long  work_done = 0;
+
+	work_done = ks8695_rx(dev, budget);
+
+	if (work_done < budget) {
+		unsigned long flags;
+		spin_lock_irqsave(&ksp->rx_lock, flags);
+		/*enable rx interrupt*/
+		__raw_writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);
+		__napi_complete(napi);
+		spin_unlock_irqrestore(&ksp->rx_lock, flags);
+	}
+	return work_done;
+}
+
+#else
 /**
  *	ks8695_rx_irq - Receive IRQ handler
  *	@irq: The IRQ which went off (ignored)
@@ -503,6 +661,8 @@ rx_finished:
 	return IRQ_HANDLED;
 }
 
+#endif
+
 /**
  *	ks8695_link_irq - Link change IRQ handler
  *	@irq: The IRQ which went off (ignored)
@@ -1472,6 +1632,10 @@ ks8695_probe(struct platform_device *pdev)
 	SET_ETHTOOL_OPS(ndev, &ks8695_ethtool_ops);
 	ndev->watchdog_timeo	 = msecs_to_jiffies(watchdog);
 
+#ifdef KS8695NET_NAPI
+	netif_napi_add(ndev, &ksp->napi, ks8695_poll, 64);
+#endif
+
 	/* Retrieve the default MAC addr from the chip. */
 	/* The bootloader should have left it in there for us. */
 
@@ -1505,6 +1669,7 @@ ks8695_probe(struct platform_device *pdev)
 
 	/* And initialise the queue's lock */
 	spin_lock_init(&ksp->txq_lock);
+	spin_lock_init(&ksp->rx_lock);
 
 	/* Specify the RX DMA ring buffer */
 	ksp->rx_ring = ksp->ring_base + TX_RING_DMA_SIZE;

Re: [PATCH]NET/KS8695: add support NAPI for Rx

[Daniel Silverstone]

On Tue, Oct 27, 2009 at 12:00:28AM +0800, Figo.zhang wrote:
> +#ifdef KS8695NET_NAPI
> +static irqreturn_t
> +ks8695_rx_irq(int irq, void *dev_id)

This routine lacks its documentation comment.  This driver is fully documented
in order to serve as a good example for others.  Indeed this lack of
documentation comments continues through your patch, I won't bring up each
instance, instead trusting you to go back over your patch and sort them out.

> +	status = __raw_readl(KS8695_IRQ_VA + KS8695_INTST);
[snip]
> +	__raw_writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
[snip]
> +			__raw_writel(status , KS8695_IRQ_VA + KS8695_INTEN);
[snip]
> +	unsigned long isr = __raw_readl(KS8695_IRQ_VA + KS8695_INTEN);
[snip]
> +		__raw_writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);

Please don't use __raw_readl or __raw_writel.  This driver was nice and clean,
don't ruin it.

Also, as an aside, you seem to add a spinlock (rx_lock) which afaict is only
used by NAPI related routines, and yet you include it regardless of NAPI being
enabled or not.  Did I misread your patch, or is this an oversight?

Regards,

Daniel.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/

Re: [PATCH]NET/KS8695: add support NAPI for Rx

[Ben Hutchings]

On Tue, 2009-10-27 at 00:00 +0800, Figo.zhang wrote:
> Add support NAPI Rx API for KS8695NET driver.
> 
> Signed-off-by: Figo.zhang <figo1802@gmail.com>
> --- 
> drivers/net/arm/ks8695net.c |  165 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 165 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
> index 2a7b774..7f9d4bd 100644
> --- a/drivers/net/arm/ks8695net.c
> +++ b/drivers/net/arm/ks8695net.c
> @@ -35,12 +35,16 @@
>  
>  #include <mach/regs-switch.h>
>  #include <mach/regs-misc.h>
> +#include <asm/mach/irq.h>
> +#include <mach/regs-irq.h>
> 
>  #include "ks8695net.h"
>  
>  #define MODULENAME	"ks8695_ether"
>  #define MODULEVERSION	"1.01"

I think this merits a version bump.
 
> +#define KS8695NET_NAPI  1
> +
>  /*
>   * Transmit and device reset timeout, default 5 seconds.
>   */
> @@ -152,6 +156,10 @@ struct ks8695_priv {
>  	enum ks8695_dtype dtype;
>  	void __iomem *io_regs;
>  
> +	#ifdef KS8695NET_NAPI
> +	struct napi_struct	napi;
> +	#endif
> +

NAPI is well-established and there should be no need to make it
optional.  So far as I'm aware, all other drivers that had it as an
option now use it unconditionally.

>  	const char *rx_irq_name, *tx_irq_name, *link_irq_name;
>  	int rx_irq, tx_irq, link_irq;
>  
> @@ -172,6 +180,7 @@ struct ks8695_priv {
>  	dma_addr_t rx_ring_dma;
>  	struct ks8695_skbuff rx_buffers[MAX_RX_DESC];
>  	int next_rx_desc_read;
> +	spinlock_t rx_lock;
>  
>  	int msg_enable;
>  };
> @@ -391,6 +400,155 @@ ks8695_tx_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +#ifdef KS8695NET_NAPI
> +static irqreturn_t
> +ks8695_rx_irq(int irq, void *dev_id)
> +{
> +	struct net_device *ndev = (struct net_device *)dev_id;
> +	struct ks8695_priv *ksp = netdev_priv(ndev);
> +	unsigned long status;
> +
> +	unsigned long mask_bit = 1 << ksp->rx_irq;
> +
> +	spin_lock(&ksp->rx_lock);
> +
> +	status = __raw_readl(KS8695_IRQ_VA + KS8695_INTST);
> +
> +	/*clean rx status bit*/
> +	__raw_writel(status | mask_bit , KS8695_IRQ_VA + KS8695_INTST);
> +
> +	if (status & mask_bit) {
> +		if (napi_schedule_prep(&ksp->napi)) {
> +			/*disable rx interrupt*/
> +			status &= ~mask_bit;
> +			__raw_writel(status , KS8695_IRQ_VA + KS8695_INTEN);
> +			__napi_schedule(&ksp->napi);
> +		}
> +	}
> +
> +	spin_unlock(&ksp->rx_lock);
> +	return IRQ_HANDLED;
> +}

The interrupt register manipulation here looks wrong, but I don't have
specific knowledge of this platform.

Since the interrupt control registers appear to be shared with other
devices, this needs to be serialised with manipulation by other drivers.

> +static int ks8695_rx(struct net_device *ndev, int budget)
> +{
> +	struct ks8695_priv *ksp = netdev_priv(ndev);
> +	struct sk_buff *skb;
> +	int buff_n;
> +	u32 flags;
> +	int pktlen;
> +	int last_rx_processed = -1;
> +	int received = 0;
> +
> +	buff_n = ksp->next_rx_desc_read;
> +	while (netif_running(ndev) && received < budget

netif_running() is quite redundant here.

> +			&& ksp->rx_buffers[buff_n].skb
> +			&& (!(ksp->rx_ring[buff_n].status &
> +					cpu_to_le32(RDES_OWN)))) {
> +			rmb();
> +			flags = le32_to_cpu(ksp->rx_ring[buff_n].status);
> +			/* Found an SKB which we own, this means we
> +			 * received a packet
> +			 */
> +			if ((flags & (RDES_FS | RDES_LS)) !=
> +			    (RDES_FS | RDES_LS)) {
> +				/* This packet is not the first and
> +				 * the last segment.  Therefore it is
> +				 * a "spanning" packet and we can't
> +				 * handle it
> +				 */
> +				goto rx_failure;
> +			}
> +
> +			if (flags & (RDES_ES | RDES_RE)) {
> +				/* It's an error packet */
> +				ndev->stats.rx_errors++;
> +				if (flags & RDES_TL)
> +					ndev->stats.rx_length_errors++;
> +				if (flags & RDES_RF)
> +					ndev->stats.rx_length_errors++;
> +				if (flags & RDES_CE)
> +					ndev->stats.rx_crc_errors++;
> +				if (flags & RDES_RE)
> +					ndev->stats.rx_missed_errors++;
> +
> +				goto rx_failure;
> +			}
> +
> +			pktlen = flags & RDES_FLEN;
> +			pktlen -= 4; /* Drop the CRC */
> +
> +			/* Retrieve the sk_buff */
> +			skb = ksp->rx_buffers[buff_n].skb;
> +
> +			/* Clear it from the ring */
> +			ksp->rx_buffers[buff_n].skb = NULL;
> +			ksp->rx_ring[buff_n].data_ptr = 0;
> +
> +			/* Unmap the SKB */
> +			dma_unmap_single(ksp->dev,
> +					 ksp->rx_buffers[buff_n].dma_ptr,
> +					 ksp->rx_buffers[buff_n].length,
> +					 DMA_FROM_DEVICE);
> +
> +			/* Relinquish the SKB to the network layer */
> +			skb_put(skb, pktlen);
> +			skb->protocol = eth_type_trans(skb, ndev);
> +			netif_receive_skb(skb);
> +
> +			/* Record stats */
> +			ndev->stats.rx_packets++;
> +			ndev->stats.rx_bytes += pktlen;
> +			goto rx_finished;
> +
> +rx_failure:
> +			/* This ring entry is an error, but we can
> +			 * re-use the skb
> +			 */
> +			/* Give the ring entry back to the hardware */
> +			ksp->rx_ring[buff_n].status = cpu_to_le32(RDES_OWN);
> +rx_finished:
> +			received++;
> +			/* And note this as processed so we can start
> +			 * from here next time
> +			 */
> +			last_rx_processed = buff_n;
> +			buff_n = (buff_n + 1) & MAX_RX_DESC_MASK;
> +			/*And note which RX descriptor we last did */
> +			if (likely(last_rx_processed != -1))
> +				ksp->next_rx_desc_read =
> +					(last_rx_processed + 1) &
> +					MAX_RX_DESC_MASK;
> +
> +			/* And refill the buffers */
> +			ks8695_refill_rxbuffers(ksp);
> +	}
> +	return received;
> +}
> +
> +static int ks8695_poll(struct napi_struct *napi, int budget)
> +{
> +	struct ks8695_priv *ksp = container_of(napi, struct ks8695_priv, napi);
> +	struct net_device *dev = ksp->ndev;
> +	unsigned long mask_bit = 1 << ksp->rx_irq;
> +	unsigned long isr = __raw_readl(KS8695_IRQ_VA + KS8695_INTEN);

This is surely the wrong place to be reading this register.

> +	unsigned long  work_done = 0;

Pointless initialisation.

> +
> +	work_done = ks8695_rx(dev, budget);
> +
> +	if (work_done < budget) {
> +		unsigned long flags;
> +		spin_lock_irqsave(&ksp->rx_lock, flags);
> +		/*enable rx interrupt*/
> +		__raw_writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);
> +		__napi_complete(napi);
> +		spin_unlock_irqrestore(&ksp->rx_lock, flags);
> +	}
> +	return work_done;
> +}
> +
> +#else
>  /**
>   *	ks8695_rx_irq - Receive IRQ handler
>   *	@irq: The IRQ which went off (ignored)
> @@ -503,6 +661,8 @@ rx_finished:
>  	return IRQ_HANDLED;
>  }
>  
> +#endif
> +
>  /**
>   *	ks8695_link_irq - Link change IRQ handler
>   *	@irq: The IRQ which went off (ignored)
> @@ -1472,6 +1632,10 @@ ks8695_probe(struct platform_device *pdev)
>  	SET_ETHTOOL_OPS(ndev, &ks8695_ethtool_ops);
>  	ndev->watchdog_timeo	 = msecs_to_jiffies(watchdog);
>  
> +#ifdef KS8695NET_NAPI
> +	netif_napi_add(ndev, &ksp->napi, ks8695_poll, 64);
> +#endif
> +
>  	/* Retrieve the default MAC addr from the chip. */
>  	/* The bootloader should have left it in there for us. */
>  
> @@ -1505,6 +1669,7 @@ ks8695_probe(struct platform_device *pdev)
>  
>  	/* And initialise the queue's lock */
>  	spin_lock_init(&ksp->txq_lock);
> +	spin_lock_init(&ksp->rx_lock);
>  
>  	/* Specify the RX DMA ring buffer */
>  	ksp->rx_ring = ksp->ring_base + TX_RING_DMA_SIZE;

You're missing a netif_napi_del() on removal.

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.

Re: [PATCH]NET/KS8695: add support NAPI for Rx

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 26 Oct 2009 16:49:06 +0000

>> @@ -152,6 +156,10 @@ struct ks8695_priv {
>>  	enum ks8695_dtype dtype;
>>  	void __iomem *io_regs;
>>  
>> +	#ifdef KS8695NET_NAPI
>> +	struct napi_struct	napi;
>> +	#endif
>> +
> 
> NAPI is well-established and there should be no need to make it
> optional.  So far as I'm aware, all other drivers that had it as an
> option now use it unconditionally.

I absolutely refuse to apply this patch with the CPP conditional
present, if you convert to NAPI make it unconditional.