lkml.org 
[lkml]   [2021]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets
Date
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.


\
 
 \ /
  Last update: 2021-01-31 08:09    [W:0.220 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site