Re: [PATCH net-next 5/6] drivers: net: xgene-v2: Add transmit and receive

[Iyappan Subramanian <isubramanian@apm.com>]

On Wed, Feb 1, 2017 at 3:09 AM, David Laight <David.Laight@aculab.com> wrote:
> From Florian Fainelli
>> Sent: 31 January 2017 20:33
>> On 01/31/2017 11:03 AM, Iyappan Subramanian wrote:
>> > This patch adds,
>> >     - Transmit
>> >     - Transmit completion poll
>> >     - Receive poll
>> >     - NAPI handler
>> >
>> > and enables the driver.
>> >
>> > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
>> > Signed-off-by: Keyur Chudgar <kchudgar@apm.com>
>> > ---
>>
>> > +
>> > +   tx_ring = pdata->tx_ring;
>> > +   tail = tx_ring->tail;
>> > +   len = skb_headlen(skb);
>> > +   raw_desc = &tx_ring->raw_desc[tail];
>> > +
>> > +   /* Tx descriptor not available */
>> > +   if (!GET_BITS(E, le64_to_cpu(raw_desc->m0)) ||
>> > +       GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0)))
>> > +           return NETDEV_TX_BUSY;
>
> Aren't you supposed to detect 'ring full' and stop the code
> giving you packets to transmit.

I'll add stop queue and wake queue.

>
>> > +
>> > +   /* Packet buffers should be 64B aligned */
>
> Is that really a requirement of the hardware?
> Almost all ethernet frames are 4n+2 aligned.

Yes, it's a hardware requirement.

>
>> > +   pkt_buf = dma_alloc_coherent(dev, XGENE_ENET_STD_MTU, &dma_addr,
>> > +                                GFP_ATOMIC);
>> > +   if (unlikely(!pkt_buf))
>> > +           goto out;
>>
>> Can't you obtain a DMA-API mapping for skb->data and pass it down to the
>> hardware? This copy here is inefficient.
>>
>> > +
>> > +   memcpy(pkt_buf, skb->data, len);
>
> You really need to verify that the len <= XGENE_ENET_STD_MTU.

This version of the driver, doesn't support jumbo frame.  So, the
check is not required.

>
> Isn't this code only transmitting the 'head' of the packet?
> What about the fragments??

This driver doesn't enable SG yet.

> ...
>         David
>