Hello

I agree to all your comments, but for some I have additionnal questions

On Mon, Jun 06, 2016 at 11:25:15AM -0700, Florian Fainelli wrote:

On 06/03/2016 02:56 AM, LABBE Corentin wrote:

[snip]

quoted

+
+/* The datasheet said that each descriptor can transfers up to 4096bytes
+ * But latter, a register documentation reduce that value to 2048
+ * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
+ */
+#define DESC_BUF_MAX 2044
+#if (DESC_BUF_MAX < (ETH_FRAME_LEN + 4))
+#error "DESC_BUF_MAX must be set at minimum to ETH_FRAME_LEN + 4"
+#endif

You can probably drop that, it would not make much sense to enable
fragments and a buffer size smaller than ETH_FRAME_LEN + ETH_FCS_LEN anyway.

I has added this test for preventing someone who want to "optimize" DESC_BUF_MAX to doing mistake.
But I agree that it is of low use.

quoted

+/* Return the number of contiguous free descriptors
+ * starting from tx_slot
+ */
+static int rb_tx_numfreedesc(struct net_device *ndev)
+{
+	struct sun8i_emac_priv *priv = netdev_priv(ndev);
+
+	if (priv->tx_slot < priv->tx_dirty)
+		return priv->tx_dirty - priv->tx_slot;

Does this work with if tx_dirty wraps around?

The tx_dirty cannot wrap since I always keep an empty slot. (tx_slot cannot go equal or after tx_dirty)

quoted

+/* Grab a frame into a skb from descriptor number i */
+static int sun8i_emac_rx_from_ddesc(struct net_device *ndev, int i)
+{
+	struct sk_buff *skb;
+	struct sun8i_emac_priv *priv = netdev_priv(ndev);
+	struct dma_desc *ddesc = priv->dd_rx + i;
+	int frame_len;
+	int crc_checked = 0;
+
+	if (ndev->features & NETIF_F_RXCSUM)
+		crc_checked = 1;

Assuming CRC here refers to the Ethernet frame's FCS, then this is
absolutely not how NETIF_F_RXCSUM works. NETIF_F_RXCSUM is about your
Ethernet adapter supporting L3/L4 checksum offloads, while the Ethernet
FCS is pretty much mandatory for the frame to be properly received in
the first place. Can you clarify which way it is?

No CRC here is RXCSUM. I understand the misnaming.
I will rename the variable to rxcsum_done.

quoted

+
+	priv->ndev->stats.rx_packets++;
+	priv->ndev->stats.rx_bytes += frame_len;
+	priv->rx_sk[i] = NULL;
+
+	/* this frame is not the last */
+	if ((ddesc->status & BIT(8)) == 0) {
+		dev_warn(priv->dev, "Multi frame not implemented currlen=%d\n",
+			 frame_len);
+	}
+
+	sun8i_emac_rx_sk(ndev, i);
+
+	netif_rx(skb);

netif_receive_skb() at the very least, or if you implement NAPI, like
you shoud napi_gro_receive().

netif_receive_skb documentation say
"This function may only be called from softirq context and interrupts should be enabled."
but the calling functions is in hardirq context.

quoted

+	return 0;
+}
+
+/* Cycle over RX DMA descriptors for finding frame to receive
+ */
+static int sun8i_emac_receive_all(struct net_device *ndev)
+{
+	struct sun8i_emac_priv *priv = netdev_priv(ndev);
+	struct dma_desc *ddesc;
+
+	ddesc = priv->dd_rx + priv->rx_dirty;
+	while (!(ddesc->status & BIT(31))) {
+		sun8i_emac_rx_from_ddesc(ndev, priv->rx_dirty);
+		rb_inc(&priv->rx_dirty, nbdesc_rx);
+		ddesc = priv->dd_rx + priv->rx_dirty;
+	};

So, what if we ping flood your device here, is not there a remote chance
that we keep the RX interrupt so busy we can't break out of this loop,
and we are executing from hard IRQ context, that's bad.

I have added a start variable for preventing to do more than a full loop.

quoted

+
+	return 0;
+}
+
+/* iterate over dma_desc for finding completed xmit.
+ * Called from interrupt context, so no need to spinlock tx

Humm, well it depends if what you are doing here may race with
ndo_start_xmit(), and usually it does.

I believe that how it is designed it cannot race each over (access the same descriptor slot) since I keep a free slot between each other.

Also, you should consider completing TX packets in NAPI context (soft
IRQ) instead of hard IRQs like here.

I wanted to finish this driver the "old" way (with hard IRQ) and implementing NAPI after as a Kconfig choice.
Does NAPI is mandatory now ? (or really recommended)
For resuming my understanding, NAPI is good when expecting high traffic. (so my Kconfig idea)
If you say that NAPI is really preferable, I will do it.

quoted

+	/* last descriptor point back to first one */
+	ddesc--;
+	ddesc->next = (u32)priv->dd_rx_phy;

So is there a limitation of this hardware that can only do DMA within
the first 4GB of the system?

Yes, I have added all DMA stuff for handling that after apritzel review.

quoted

+static int sun8i_emac_check_if_running(struct net_device *ndev)
+{
+	if (!netif_running(ndev))
+		return -EBUSY;

This does not seem like the intended usage of a

I have changed the return code after reading other drivers.
But could you end your sentence for be sure that the problem is that.

quoted

+
+static int sun8i_emac_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+
+	unregister_netdev(ndev);
+	platform_set_drvdata(pdev, NULL);

Missing unregister_netdevice() here.

Does I need to replace unregister_netdev by it ?
They seems to to the same job.

quoted

+	free_netdev(ndev);
+
+	return 0;
+}

-- 
Florian

Thanks for your review

Regards

LABBE Corentin