Thread (30 messages) 30 messages, 6 authors, 2021-02-05

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

From: <Bryan.Whitehead@microchip.com>
Date: 2021-01-31 07:09:06
Also in: lkml

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.

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help