Re: net: xilinx: axienet: Query about checksum partial implementation

[Simon Horman <horms@kernel.org>]

On Tue, Jul 30, 2024 at 07:15:13PM +0000, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Friday, July 26, 2024 5:37 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>; Ariane Keller
> > <ariane.keller@tik.ee.ethz.ch>; Simek, Michal <michal.simek@amd.com>;
> > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: net: xilinx: axienet: Query about checksum partial implementation
> > 
> > Hi Radhey, all,
> > 
> > I am wondering if you could shed some light on the following checksum
> > partial handling in the axienet_rx_poll():
> > 
> >                         /* if we're doing Rx csum offload, set it up */
> >                         if (lp->features & XAE_FEATURE_FULL_RX_CSUM) {
> > 				...
> >                         } else if ((lp->features & XAE_FEATURE_PARTIAL_RX_CSUM) != 0
> > &&
> >                                    skb->protocol == htons(ETH_P_IP) &&
> >                                    skb->len > 64) {
> >                                 skb->csum = be32_to_cpu(cur_p->app3 & 0xFFFF);
> >                                 ...
> >                         }
> > 
> > In particluar the "skb->csum =" line.
> > 
> > The type of cur_p->app3 is u32, and 0xFFFF is also host byte order.
> > So far so good. But after the bitwise operation it is treated as a big-endian
> > value by passing it to be32_to_cpu.
> > 
> > Perhaps I am missing something obvious, but my question is how does that
> > work?
> > 
> > * Was it only tested on big endian sysgtems where be32_to_cpu() is a no-op
> > 
> > * Was it only tested on little endian systems where be32_to_cpu()
> >   is a byteswap and somehow that works (how?).
> > 
> > * Is the code unecessised because the XAE_FEATURE_FULL_RX_CSUM branch
> > is
> >   always taken?
> > 
> >   A grep of dts files shows up arch/microblaze/boot/dts/system.dts which
> >   sets sets xlnx,rxcsum to 0, which corresponds to XAE_NO_CSUM_OFFLOAD.
> 
> + Harini
> 
> Yes, IIRC default AXI Ethernet IP RX checksum is set to "No checksum offload"
> so, it is default case and being set in most designs. Have added Harini to this
> thread to confirm on partial checksum verification results.
> 
> Assuming partial implementation is functional then likely DMA IP updates
> application field in big endian format and that is the reason we have this
> be32 to CPU conversion in place. will dig a bit more and get back on it.

Thanks, much appreciated.

FWIIW, I do agree that the scenario you describe would mostly explain
things, although the mask with 0xFFFF still seems off.