Hi Florian,
On Sat, 9 Sep 2017 09:30:58 -0700 <f.fainelli@xxxxxxxxx> wrote:
>
>
> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> > The UniPhier platform from Socionext provides the AVE ethernet
> > controller that includes MAC and MDIO bus supporting RGMII/RMII
> > modes. The controller is named AVE.
> >
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
> > Signed-off-by: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
> > ---
>
> [snip]
>
> > +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > + struct ave_private *priv = netdev_priv(ndev);
> > + u32 proc_idx, done_idx, ndesc, cmdsts;
> > + int freepkt;
> > + unsigned char *buffptr = NULL; /* buffptr for descriptor */
> > + unsigned int len;
> > + dma_addr_t paddr;
> > +
> > + proc_idx = priv->tx.proc_idx;
> > + done_idx = priv->tx.done_idx;
> > + ndesc = priv->tx.ndesc;
> > + freepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;
> > +
> > + /* not enough entry, then we stop queue */
> > + if (unlikely(freepkt < 2)) {
> > + netif_stop_queue(ndev);
> > + if (unlikely(freepkt < 1))
> > + return NETDEV_TX_BUSY;
>
> This looks wrong, why are you checking first for less than 2
> descriptors, and if there is none, NETDEV_TX_BUSY? If you need 2 slots
> to complete a transmision, stop the transmit queue and return
> NETDEV_TX_BUSY.
This code is misleading and I have to fix this.
The device needs a slot to complete a transmission.
> > + }
> > +
> > + priv->tx.desc[proc_idx].skbs = skb;
> > +
> > + /* add padding for short packet */
> > + if (skb_padto(skb, ETH_ZLEN)) {
> > + dev_kfree_skb_any(skb);
> > + return NETDEV_TX_OK;
> > + }
>
> skb_padto() frees the SKB in case of error, that would lead to a double
> free here.
Ah, it occurs double free. I'll fix it.
> > +
> > + buffptr = skb->data - NET_IP_ALIGN;
> > + len = max_t(unsigned int, ETH_ZLEN, skb->len);
>
> If you use skb_put_padto() if padding was necessary skb->len will be at
> least ETH_ZLEN, so you can remove this.
I see. It's reasonable.
> > +
> > + paddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,
> > + len + NET_IP_ALIGN, DMA_TO_DEVICE);
>
> As mentioned before you can't assume this will never fail.
Okay, I'll rewrite it in consideration of error case.
> > + paddr += NET_IP_ALIGN;
> > +
> > + /* set buffer address to descriptor */
> > + ave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);
>
> Also mentioned in the other email, make this 4 a constant so we know
> it's an offset and not a length.
I see.
> > +
> > + /* set flag and length to send */
> > + cmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST
> > + | (len & AVE_STS_PKTLEN_TX);
>
> AVE_STS_PKTLEN_TX would be better named with a _MASK suffix.
Yes.
> > +
> > + /* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */
> > + if (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))
> > + cmdsts |= AVE_STS_INTR;
> > +
> > + /* disable checksum calculation when skb doesn't calurate checksum */
> > + if (skb->ip_summed == CHECKSUM_NONE ||
> > + skb->ip_summed == CHECKSUM_UNNECESSARY)
> > + cmdsts |= AVE_STS_NOCSUM;
> > +
> > + /* set cmdsts */
> > + ave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);
> > +
> > + priv->tx.proc_idx = (proc_idx + 1) % ndesc;
>
> You should also check the ring space after transmission and assert flow
> control on the transmit queue if needed.
Okay, I'll add this.
> > +
> > + return NETDEV_TX_OK;
> > +}
>
> [snip]
>
> > +static struct net_device_stats *ave_stats(struct net_device *ndev)
> > +{
> > + struct ave_private *priv = netdev_priv(ndev);
> > + u32 drop_num = 0;
> > +
> > + priv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);
> > +
> > + drop_num += ave_r32(ndev, AVE_RX0OVFFC);
> > + drop_num += ave_r32(ndev, AVE_SN5FC);
> > + drop_num += ave_r32(ndev, AVE_SN6FC);
> > + drop_num += ave_r32(ndev, AVE_SN7FC);
> > + priv->stats.rx_dropped = drop_num;
> > +
>
> You should consider switching to 64-bit statistics, this requires a
> little bit more work for 32-bit hosts (see
> include/linux/u64_stats_sync.h) but this allows you to keep statistics
> around above 4GB.
I see.
I'll refer to this header and its examples, and rewrite it to be suitable
for 32-bit and 64-bit hosts.
>
> > + return &priv->stats;
> > +}
> > +--
> Florian
---
Best Regards,
Kunihiko Hayashi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
