RE: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets

[Bryan.Whitehead]

Hi Sven, 

Looks good.
see comments below.

>  static int lan743x_rx_process_packet(struct lan743x_rx *rx)  {
It looks like this function no longer processes a packet, but rather only processes a single buffer.
So perhaps it should be renamed to lan743x_rx_process_buffer, so it is not misleading.

...
> +       /* unmap from dma */
> +       if (buffer_info->dma_ptr) {
> +               dma_unmap_single(&rx->adapter->pdev->dev,
> +                                buffer_info->dma_ptr,
> +                                buffer_info->buffer_length,
> +                                DMA_FROM_DEVICE);
> +               buffer_info->dma_ptr = 0;
> +               buffer_info->buffer_length = 0;
> +       }
> +       skb = buffer_info->skb;
> 
> -process_extension:
> -               if (extension_index >= 0) {
> -                       descriptor = &rx->ring_cpu_ptr[extension_index];
> -                       buffer_info = &rx->buffer_info[extension_index];
> -
> -                       ts_sec = le32_to_cpu(descriptor->data1);
> -                       ts_nsec = (le32_to_cpu(descriptor->data2) &
> -                                 RX_DESC_DATA2_TS_NS_MASK_);
> -                       lan743x_rx_reuse_ring_element(rx, extension_index);
> -                       real_last_index = extension_index;
> -               }
> +       /* allocate new skb and map to dma */
> +       if (lan743x_rx_init_ring_element(rx, rx->last_head)) {

If lan743x_rx_init_ring_element fails to allocate an skb,
Then lan743x_rx_reuse_ring_element will be called.
But that function expects the skb is already allocated and dma mapped.
But the dma was unmapped above.

Also if lan743x_rx_init_ring_element fails to allocate an skb.
Then control will jump to process_extension and therefor
the currently received skb will not be added to the skb list.
I assume that would corrupt the packet? Or am I missing something?

...
> -               if (!skb) {
> -                       result = RX_PROCESS_RESULT_PACKET_DROPPED;
It looks like this return value is no longer used.
If there is no longer a case where a packet will be dropped 
then maybe this return value should be deleted from the header file.

... 
>  move_forward:
> -               /* push tail and head forward */
> -               rx->last_tail = real_last_index;
> -               rx->last_head = lan743x_rx_next_index(rx, real_last_index);
> -       }
> +       /* push tail and head forward */
> +       rx->last_tail = rx->last_head;
> +       rx->last_head = lan743x_rx_next_index(rx, rx->last_head);
> +       result = RX_PROCESS_RESULT_PACKET_RECEIVED;

Since this function handles one buffer at a time,
  The return value RX_PROCESS_RESULT_PACKET_RECEIVED is now misleading.
  Can you change it to RX_PROCESS_RESULT_BUFFER_RECEIVED.