Messages in this thread | | | From | <> | Subject | RE: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets | Date | Sun, 31 Jan 2021 07:06:53 +0000 |
| |
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.
| |