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.