On Mon, Feb 26, 2024 at 07:06:46PM +0000, Russell King (Oracle) wrote: > On Mon, Feb 26, 2024 at 10:35:18AM -0800, Charlie Jenkins wrote: > > On Mon, Feb 26, 2024 at 05:50:57PM +0000, Russell King (Oracle) wrote: > > > On Mon, Feb 26, 2024 at 08:44:29AM -0800, Guenter Roeck wrote: > > > > On 2/26/24 03:34, Christophe Leroy wrote: > > > > > > > > > > > > > > > Le 23/02/2024 à 23:11, Charlie Jenkins a écrit : > > > > > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly > > > > > > aligning the IP header, which were causing failures on architectures > > > > > > that do not support misaligned accesses like some ARM platforms. To > > > > > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the > > > > > > standard alignment of an IP header and must be supported by the > > > > > > architecture. > > > > > > > > > > I'm still wondering what we are really trying to fix here. > > > > > > > > > > All other tests are explicitely testing that it works with any alignment. > > > > > > > > > > Shouldn't ip_fast_csum() and csum_ipv6_magic() work for any alignment as > > > > > well ? I would expect it, I see no comment in arm code which explicits > > > > > that assumption around those functions. > > > > > > > > > > Isn't the problem only the following line, because csum_offset is > > > > > unaligned ? > > > > > > > > > > csum = *(__wsum *)(random_buf + i + csum_offset); > > > > > > > > > > Otherwise, if there really is an alignment issue for the IPv6 source or > > > > > destination address, isn't it enough to perform a 32 bits alignment ? > > > > > > > > > > > > > It isn't just arm. > > > > > > > > Question should be what alignments the functions are supposed to be able > > > > to handle, not what they are optimized for. If byte and/or half word alignments > > > > are expected to be supported, there is still architecture code which would > > > > have to be fixed. Unaligned accesses are known to fail on hppa64/parisc64 > > > > and on sh4, for example. If unaligned accesses are expected to be handled, > > > > it would probably make sense to add a separate test case, though, to clarify > > > > that the test fails due to alignment issues, not due to input parameters. > > > > > > It's network driver dependent. Most network drivers receive packets > > > to the offset defined by NET_IP_ALIGN (which is normally 2) which > > > has the effect of "mis-aligning" the ethernet header, but aligning > > > the IP header. > > > > > > Whether drivers do that is up to drivers (and their capabilities). > > > Some network drivers can not do this kind of alignment, so there are > > > cases where the received packets aren't offset by two bytes, leading > > > to the IP header being aligned to an odd 16-bit word rather than an > > > even 16-bit word (and thus 32-bit aligned.) > > > > > > Then you have the possibility of other headers between the ethernet > > > and IP header - not only things like VLANs, but also possibly DSA > > > headers (for switches) and how big those are. > > > > Those additional combinations can be supported by future test cases, > > but the goal of this patch was simply to have basic testing for these > > functions. The NET_IP_ALIGN offset is what the kernel defines to be > > supported, so that is the test case I went for. > > I think you misunderstand. "NET_IP_ALIGN offset is what the kernel > defines to be supported" is a gross misinterpretation. It is not > "defined to be supported" at all. It is the _preferred_ alignment > nothing more, nothing less. What alignment can be relied on by a test case? - Charlie > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
