RE: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets
flat view
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. ...
quoted hunk ↗ jump to hunk
+ /* 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. ...
quoted hunk ↗ jump to hunk
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.