Messages in this thread | | | Date | Thu, 9 Jun 2016 11:44:02 +0200 | From | LABBE Corentin <> | Subject | Re: [PATCH 1/5] ethernet: add sun8i-emac driver |
| |
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] > > > + > > +/* 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.
> > +/* 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)
> > +/* 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.
> > + > > + 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.
> > + 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.
> > + > > + 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.
> > + /* 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.
> > +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.
> > + > > +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.
> > + free_netdev(ndev); > > + > > + return 0; > > +} > -- > Florian
Thanks for your review
Regards
LABBE Corentin
| |