Re: [PATCH net-next 3/8] cadence: macb: Add page pool support handle multi-descriptor frame rx

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Thu Jan 15, 2026 at 11:25 PM CET, Paolo Valerio wrote:
> @@ -1382,58 +1381,117 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  #if defined(DEBUG) && defined(VERBOSE_DEBUG)
> -		netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
> -			    skb->len, skb->csum);
> -		print_hex_dump(KERN_DEBUG, " mac: ", DUMP_PREFIX_ADDRESS, 16, 1,
> -			       skb_mac_header(skb), 16, true);
> -		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_ADDRESS, 16, 1,
> -			       skb->data, 32, true);
> +			netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
> +				    queue->skb->len, queue->skb->csum);
> +			print_hex_dump_debug(" mac: ", DUMP_PREFIX_ADDRESS, 16, 1,
> +					     skb_mac_header(queue->skb), 16, true);
> +			print_hex_dump_debug("buff_head: ", DUMP_PREFIX_ADDRESS, 16, 1,
> +					     buff_head, 32, true);
>  #endif

I think the print_hex_*() calls need some updating. The first call
prints the MAC header (which is sensible) but the second one prints the
first 32 bytes of buff_head, which points to the headroom.

I have been using those calls today to debug something on a new
platform, and I replaced it by:

#if defined(DEBUG) && defined(VERBOSE_DEBUG)
         BUG_ON(!first_frame);
         netdev_vdbg(bp->dev, "received skb of length %u (len %d), csum: %08x\n",
                queue->skb->len, data_len, queue->skb->csum);
         print_hex_dump_debug("buffer: ", DUMP_PREFIX_ADDRESS, 16, 1,
                    buff_head + bp->rx_headroom, data_len, true);
#endif

So I replaced it by a single call that prints the full buffer.
 - It prints packets in their entirety, in a single call. Being a single
   call is important because then with scripting it can be imported
   into wireshark. Yes this is ugly but tcpdump is too late for my
   debugging needs.
 - This does not work if an SKB spans >1 frame, which is why I put the
   BUG_ON() call. It is fine for my usecase but upstream might need
   something more resistant.

Here is one proposal for upstream:
 - We move the printk from last_frame to first_frame.
   That way the debugging code avoids dealing with fragmentation.
 - We print `buff_head + bp->rx_headroom` with a size of
   `min(±64, data_len)`, to avoid printing full packets.

I can deal with it if you want. If I do, you should probably drop the
print_hex_dump_debug() calls in your series because they are broken
as-is. Or you do it, as you want!

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com