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

2021-02-03 Thread Bryan.Whitehead
> On Wed, Feb 3, 2021 at 3:14 PM  wrote:
> >
> > We can test on x86 PC. We will just need about a week after you release
> your next version.
> >
> 
> That's great. If you have any suggestions on how I can improve testing on my
> end, feel free to reach out.

If you are able, in addition to basic rx and tx iperf tests, I would recommend 
PTP tests.
PTP relies on the time stamps extracted from the extension descriptors, which 
is directly in the RX path you are modifying.

If you are not able, we will at least cover that for x86 PC.

Thanks,
Bryan



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

2021-02-03 Thread Sven Van Asbroeck
On Wed, Feb 3, 2021 at 3:14 PM  wrote:
>
> We can test on x86 PC. We will just need about a week after you release your 
> next version.
>

That's great. If you have any suggestions on how I can improve testing
on my end, feel free to reach out.


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

2021-02-03 Thread Bryan.Whitehead
Hi Sven,

We can test on x86 PC. We will just need about a week after you release your 
next version.

Thanks,
Bryan

> -Original Message-
> From: Sven Van Asbroeck 
> Sent: Wednesday, February 3, 2021 1:53 PM
> To: Bryan Whitehead - C21958 
> Cc: UNGLinuxDriver ; David Miller
> ; Jakub Kicinski ; Andrew Lunn
> ; Alexey Denisov ; Sergej Bauer
> ; Tim Harvey ; Anders
> Rønningen ; netdev
> ; Linux Kernel Mailing List  ker...@vger.kernel.org>
> Subject: Re: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer
> packets
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Thank you Bryan. I will prepare a v2 early next week.
> 
> Would Microchip be able to donate some time to test v2? My own tests are
> certainly not perfect. Various stress tests across architectures
> (intel/arm) would help create confidence in the multi-buffer frame
> implementation. Perhaps Microchip has various test rigs already set up?
> 
> After all, absence of bugs is more important than speed improvements.


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

2021-02-03 Thread Sven Van Asbroeck
Thank you Bryan. I will prepare a v2 early next week.

Would Microchip be able to donate some time to test v2? My own tests
are certainly not perfect. Various stress tests across architectures
(intel/arm) would help create confidence in the multi-buffer frame
implementation. Perhaps Microchip has various test rigs already set
up?

After all, absence of bugs is more important than speed improvements.


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

2021-02-01 Thread Bryan.Whitehead
Hi Sven, see below

> >
> > 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.
> 
> Good catch. I think you're right, the skb allocation always has to come before
> the unmap. Because if we unmap, and then the skb allocation fails, there is
> no guarantee that we can remap the old skb we've just unmapped (it could
> fail).
> And then we'd end up with a broken driver.
> 
> BUT I actually joined skb alloc and init_ring_element, because of a very
> subtle synchronization bug I was seeing: if someone changes the mtu
> _in_between_ skb alloc and init_ring_element, things will go haywire,
> because the skb and mapping lengths would be different !
> 
> We could fix that by using a spinlock I guess, but synchronization primitives 
> in
> "hot paths" like these are far from ideal... Would be nice if we could avoid
> that.
> 
> Here's an idea: what if we fold "unmap from dma" into init_ring_element()?
> That way, we get the best of both worlds: length cannot change in the
> middle, and the function can always "back out" without touching the ring
> element in case an allocation or mapping fails.
> 
> Pseudo-code:
> 
> init_ring_element() {
> /* single "sampling" of mtu, so no synchronization required */
> length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;
> 
> skb = alloc(length);
> if (!skb) return FAIL;
> dma_ptr = dma_map(skb, length);
> if (!dma_ptr) {
> free(skb);
> return FAIL;
> }
> if (buffer_info->dma_ptr)
> dma_unmap(buffer_info->dma_ptr, buffer_info->buffer_length);
> buffer_info->skb = skb;
> buffer_info->dma_ptr = dma_ptr;
> buffer_info->buffer_length = length;
> 
> return SUCCESS;
> }
> 
> What do you think?

Yes, I believe this will work.

> 
> >
> > 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?
> >
> 
> Yes if an skb alloc failure in the middle of a multi-buffer frame, will 
> corrupt
> the packet inside the frame. A chunk will be missing. I had assumed that this
> would be caught by an upper network layer, some checksum would be
> incorrect?
> 
> Are there current networking devices that would send a corrupted packet to
> Linux if there is a corruption on the physical link? Especially if they don't
> support checksumming?
> 
> Maybe my assumption is naive.
> I'll fix this up if you believe that it could be an issue.

Yes the upper layers will catch it and drop it.
But if we already know the packet is corrupted, I think it would be better if we
dropped it here, to avoid unnecessary processing upstream.

...
> 
> RX_PROCESS_RESULT_XXX can now only take two values (RECEIVED and
> NOTHING_TO_DO), so in theory it could be replaced by a bool. But perhaps
> we should keep the current names, because they are clearer to the reader?
> 
I'm ok if you want to change it too a bool. 



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

2021-01-31 Thread Sven Van Asbroeck
On Sun, Jan 31, 2021 at 2:06 AM  wrote:
>
> >  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.

Agreed, will do.

>
> 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.

Good catch. I think you're right, the skb allocation always has to come before
the unmap. Because if we unmap, and then the skb allocation fails, there is no
guarantee that we can remap the old skb we've just unmapped (it could fail).
And then we'd end up with a broken driver.

BUT I actually joined skb alloc and init_ring_element, because of a very subtle
synchronization bug I was seeing: if someone changes the mtu _in_between_
skb alloc and init_ring_element, things will go haywire, because the skb and
mapping lengths would be different !

We could fix that by using a spinlock I guess, but synchronization primitives
in "hot paths" like these are far from ideal... Would be nice if we could
avoid that.

Here's an idea: what if we fold "unmap from dma" into init_ring_element()?
That way, we get the best of both worlds: length cannot change in the middle,
and the function can always "back out" without touching the ring element
in case an allocation or mapping fails.

Pseudo-code:

init_ring_element() {
/* single "sampling" of mtu, so no synchronization required */
length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;

skb = alloc(length);
if (!skb) return FAIL;
dma_ptr = dma_map(skb, length);
if (!dma_ptr) {
free(skb);
return FAIL;
}
if (buffer_info->dma_ptr)
dma_unmap(buffer_info->dma_ptr, buffer_info->buffer_length);
buffer_info->skb = skb;
buffer_info->dma_ptr = dma_ptr;
buffer_info->buffer_length = length;

return SUCCESS;
}

What do you think?

>
> 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?
>

Yes if an skb alloc failure in the middle of a multi-buffer frame, will corrupt
the packet inside the frame. A chunk will be missing. I had assumed that this
would be caught by an upper network layer, some checksum would be incorrect?

Are there current networking devices that would send a corrupted packet to
Linux if there is a corruption on the physical link? Especially if they don't
support checksumming?

Maybe my assumption is naive.
I'll fix this up if you believe that it could be an issue.

> ...
> > -   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.

Agreed, will do.

>
> ...
> >  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.

Agreed, will do.

RX_PROCESS_RESULT_XXX can now only take two values (RECEIVED and NOTHING_TO_DO),
so in theory it could be replaced by a bool. But perhaps we should keep the
current names, because they are clearer to the reader?

>
>


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

2021-01-30 Thread Bryan.Whitehead
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(>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 = >ring_cpu_ptr[extension_index];
> -   buffer_info = >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.




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

2021-01-29 Thread Sven Van Asbroeck
On Fri, Jan 29, 2021 at 6:08 PM Willem de Bruijn
 wrote:
>
> Okay. I found it a bit hard to parse how much true code change was
> mixed in with just reindenting existing code. If a lot, then no need
> to split of the code refactor.

Thank you. The code is quite hard to review in patch format.
Probably easier to apply the patch first, then read the replaced
function.


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

2021-01-29 Thread Willem de Bruijn
On Fri, Jan 29, 2021 at 6:03 PM Sven Van Asbroeck  wrote:
>
> Hoi Willem, thanks a lot for reviewing this patch, much appreciated !!
>
> On Fri, Jan 29, 2021 at 5:11 PM Willem de Bruijn
>  wrote:
> >
> > > +static struct sk_buff *
> > > +lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
> > > +{
> > > +   if (skb_linearize(skb)) {
> >
> > Is this needed? That will be quite expensive
>
> The skb will only be non-linear when it's created from a multi-buffer frame.
> Multi-buffer frames are only generated right after a mtu change - fewer than
> 32 frames will be non-linear after an mtu increase. So as long as people don't
> change the mtu in a tight loop, skb_linearize is just a single comparison,
> 99.99+% of the time.

Ah. I had missed the temporary state of this until the buffers are
reinitialized. Yes, then there is no reason to worry. Same for the
frag_list vs frags comment I made.

> >
> > Is it possible to avoid the large indentation change, or else do that
> > in a separate patch? It makes it harder to follow the functional
> > change.
>
> It's not immediately obvious, but I have replaced the whole function
> with slightly different logic, and the replacement content has a much
> flatter indentation structure, and should be easier to follow.
>
> Or perhaps I am misinterpreting your question?

Okay. I found it a bit hard to parse how much true code change was
mixed in with just reindenting existing code. If a lot, then no need
to split of the code refactor.

>
> > > +
> > > +   /* add buffers to skb via skb->frag_list */
> > > +   if (is_first) {
> > > +   skb_reserve(skb, RX_HEAD_PADDING);
> > > +   skb_put(skb, buffer_length - RX_HEAD_PADDING);
> > > +   if (rx->skb_head)
> > > +   dev_kfree_skb_irq(rx->skb_head);
> > > +   rx->skb_head = skb;
> > > +   } else if (rx->skb_head) {
> > > +   skb_put(skb, buffer_length);
> > > +   if (skb_shinfo(rx->skb_head)->frag_list)
> > > +   rx->skb_tail->next = skb;
> > > +   else
> > > +   skb_shinfo(rx->skb_head)->frag_list = skb;
> >
> > Instead of chaining skbs into frag_list, you could perhaps delay skb
> > alloc until after reception, allocate buffers stand-alone, and link
> > them into the skb as skb_frags? That might avoid a few skb alloc +
> > frees. Though a bit change, not sure how feasible.
>
> The problem here is this (copypasta from somewhere else in this patch):
>
> /* Only the last buffer in a multi-buffer frame contains the total frame
> * length. All other buffers have a zero frame length. The chip
> * occasionally sends more buffers than strictly required to reach the
> * total frame length.
> * Handle this by adding all buffers to the skb in their entirety.
> * Once the real frame length is known, trim the skb.
> */
>
> In other words, the chip sometimes sends more buffers than strictly needed to
> fit the frame. linearize + trim deals with this thorny issue perfectly.
>
> If the skb weren't linearized, we would run into trouble when trying to trim
> (remove from the end) a chunk bigger than the last skb fragment.
>
> > > +process_extension:
> > > +   if (extension_index >= 0) {
> > > +   u32 ts_sec;
> > > +   u32 ts_nsec;
> > > +
> > > +   ts_sec = le32_to_cpu(desc_ext->data1);
> > > +   ts_nsec = (le32_to_cpu(desc_ext->data2) &
> > > + RX_DESC_DATA2_TS_NS_MASK_);
> > > +   if (rx->skb_head) {
> > > +   hwtstamps = skb_hwtstamps(rx->skb_head);
> > > +   if (hwtstamps)
> >
> > This is always true.
> >
> > You can just call skb_hwtstamps(skb)->hwtstamp = ktime_set(ts_sec, ts_nsec);
>
> Thank you, will do !
>
> >
> > Though I see that this is existing code just moved due to
> > aforementioned indentation change.
>
> True, but I can make the change anyway.


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

2021-01-29 Thread Sven Van Asbroeck
Hoi Willem, thanks a lot for reviewing this patch, much appreciated !!

On Fri, Jan 29, 2021 at 5:11 PM Willem de Bruijn
 wrote:
>
> > +static struct sk_buff *
> > +lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
> > +{
> > +   if (skb_linearize(skb)) {
>
> Is this needed? That will be quite expensive

The skb will only be non-linear when it's created from a multi-buffer frame.
Multi-buffer frames are only generated right after a mtu change - fewer than
32 frames will be non-linear after an mtu increase. So as long as people don't
change the mtu in a tight loop, skb_linearize is just a single comparison,
99.99+% of the time.

>
> Is it possible to avoid the large indentation change, or else do that
> in a separate patch? It makes it harder to follow the functional
> change.

It's not immediately obvious, but I have replaced the whole function
with slightly different logic, and the replacement content has a much
flatter indentation structure, and should be easier to follow.

Or perhaps I am misinterpreting your question?

> > +
> > +   /* add buffers to skb via skb->frag_list */
> > +   if (is_first) {
> > +   skb_reserve(skb, RX_HEAD_PADDING);
> > +   skb_put(skb, buffer_length - RX_HEAD_PADDING);
> > +   if (rx->skb_head)
> > +   dev_kfree_skb_irq(rx->skb_head);
> > +   rx->skb_head = skb;
> > +   } else if (rx->skb_head) {
> > +   skb_put(skb, buffer_length);
> > +   if (skb_shinfo(rx->skb_head)->frag_list)
> > +   rx->skb_tail->next = skb;
> > +   else
> > +   skb_shinfo(rx->skb_head)->frag_list = skb;
>
> Instead of chaining skbs into frag_list, you could perhaps delay skb
> alloc until after reception, allocate buffers stand-alone, and link
> them into the skb as skb_frags? That might avoid a few skb alloc +
> frees. Though a bit change, not sure how feasible.

The problem here is this (copypasta from somewhere else in this patch):

/* Only the last buffer in a multi-buffer frame contains the total frame
* length. All other buffers have a zero frame length. The chip
* occasionally sends more buffers than strictly required to reach the
* total frame length.
* Handle this by adding all buffers to the skb in their entirety.
* Once the real frame length is known, trim the skb.
*/

In other words, the chip sometimes sends more buffers than strictly needed to
fit the frame. linearize + trim deals with this thorny issue perfectly.

If the skb weren't linearized, we would run into trouble when trying to trim
(remove from the end) a chunk bigger than the last skb fragment.

> > +process_extension:
> > +   if (extension_index >= 0) {
> > +   u32 ts_sec;
> > +   u32 ts_nsec;
> > +
> > +   ts_sec = le32_to_cpu(desc_ext->data1);
> > +   ts_nsec = (le32_to_cpu(desc_ext->data2) &
> > + RX_DESC_DATA2_TS_NS_MASK_);
> > +   if (rx->skb_head) {
> > +   hwtstamps = skb_hwtstamps(rx->skb_head);
> > +   if (hwtstamps)
>
> This is always true.
>
> You can just call skb_hwtstamps(skb)->hwtstamp = ktime_set(ts_sec, ts_nsec);

Thank you, will do !

>
> Though I see that this is existing code just moved due to
> aforementioned indentation change.

True, but I can make the change anyway.


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

2021-01-29 Thread Willem de Bruijn
On Fri, Jan 29, 2021 at 2:56 PM Sven Van Asbroeck  wrote:
>
> From: Sven Van Asbroeck 
>
> Multi-buffer packets enable us to use rx ring buffers smaller than
> the mtu. This will allow us to change the mtu on-the-fly, without
> having to stop the network interface in order to re-size the rx
> ring buffers.
>
> This is a big change touching a key driver function (process_packet),
> so care has been taken to test this extensively:
>
> Tests with debug logging enabled (add #define DEBUG).
>
> 1. Limit rx buffer size to 500, so mtu (1500) takes 3 buffers.
>Ping to chip, verify correct packet size is sent to OS.
>Ping large packets to chip (ping -s 1400), verify correct
>  packet size is sent to OS.
>Ping using packets around the buffer size, verify number of
>  buffers is changing, verify correct packet size is sent
>  to OS:
>  $ ping -s 472
>  $ ping -s 473
>  $ ping -s 992
>  $ ping -s 993
>Verify that each packet is followed by extension processing.
>
> 2. Limit rx buffer size to 500, so mtu (1500) takes 3 buffers.
>Run iperf3 -s on chip, verify that packets come in 3 buffers
>  at a time.
>Verify that packet size is equal to mtu.
>Verify that each packet is followed by extension processing.
>
> 3. Set chip and host mtu to 2000.
>Limit rx buffer size to 500, so mtu (2000) takes 4 buffers.
>Run iperf3 -s on chip, verify that packets come in 4 buffers
>  at a time.
>Verify that packet size is equal to mtu.
>Verify that each packet is followed by extension processing.
>
> Tests with debug logging DISabled (remove #define DEBUG).
>
> 4. Limit rx buffer size to 500, so mtu (1500) takes 3 buffers.
>Run iperf3 -s on chip, note sustained rx speed.
>Set chip and host mtu to 2000, so mtu takes 4 buffers.
>Run iperf3 -s on chip, note sustained rx speed.
>Verify no packets are dropped in both cases.
>
> Tests with DEBUG_KMEMLEAK on:
>  $ mount -t debugfs nodev /sys/kernel/debug/
>  $ echo scan > /sys/kernel/debug/kmemleak
>
> 5. Limit rx buffer size to 500, so mtu (1500) takes 3 buffers.
>Run the following tests concurrently for at least one hour:
>- iperf3 -s on chip
>- ping -> chip
>Monitor reported memory leaks.
>
> 6. Set chip and host mtu to 2000.
>Limit rx buffer size to 500, so mtu (2000) takes 4 buffers.
>Run the following tests concurrently for at least one hour:
>- iperf3 -s on chip
>- ping -> chip
>Monitor reported memory leaks.
>
> 7. Simulate low-memory in lan743x_rx_allocate_skb(): fail every
>  100 allocations.
>Repeat (5) and (6).
>Monitor reported memory leaks.
>
> 8. Simulate  low-memory in lan743x_rx_allocate_skb(): fail 10
>  allocations in a row in every 100.
>Repeat (5) and (6).
>Monitor reported memory leaks.
>
> 9. Simulate  low-memory in lan743x_rx_trim_skb(): fail 1 allocation
>  in every 100.
>Repeat (5) and (6).
>Monitor reported memory leaks.
>
> Signed-off-by: Sven Van Asbroeck 
> ---
>
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git # 
> 46eb3c108fe1
>
> To: Bryan Whitehead 
> To: unglinuxdri...@microchip.com
> To: "David S. Miller" 
> To: Jakub Kicinski 
> Cc: Andrew Lunn 
> Cc: Alexey Denisov 
> Cc: Sergej Bauer 
> Cc: Tim Harvey 
> Cc: Anders Rønningen 
> Cc: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org (open list)
>
>  drivers/net/ethernet/microchip/lan743x_main.c | 321 --
>  drivers/net/ethernet/microchip/lan743x_main.h |   2 +
>  2 files changed, 143 insertions(+), 180 deletions(-)


> +static struct sk_buff *
> +lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
> +{
> +   if (skb_linearize(skb)) {

Is this needed? That will be quite expensive

> +   dev_kfree_skb_irq(skb);
> +   return NULL;
> +   }
> +   frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2);
> +   if (skb->len > frame_length) {
> +   skb->tail -= skb->len - frame_length;
> +   skb->len = frame_length;
> +   }
> +   return skb;
> +}
> +
>  static int lan743x_rx_process_packet(struct lan743x_rx *rx)
>  {
> -   struct skb_shared_hwtstamps *hwtstamps = NULL;
> +   struct lan743x_rx_descriptor *descriptor, *desc_ext;
> int result = RX_PROCESS_RESULT_NOTHING_TO_DO;
> int current_head_index = le32_to_cpu(*rx->head_cpu_ptr);
> struct lan743x_rx_buffer_info *buffer_info;
> -   struct lan743x_rx_descriptor *descriptor;
> +   struct skb_shared_hwtstamps *hwtstamps;
> +   int frame_length, buffer_length;
> +   struct sk_buff *skb;
> int extension_index = -1;
> -   int first_index = -1;
> -   int last_index = -1;
> +   bool is_last, is_first;
>
> if (current_head_index < 0 || current_head_index >= rx->ring_size)
> goto done;
> @@ -2068,170 +2075,126 @@ static int lan743x_rx_process_packet(struct 
> lan743x_rx *rx)
> if