Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next v2 2/3] net: ethernet: rmnet: Support for
 downlink MAPv5 checksum offload
To:     Sharath Chandra Vurukala <sharathv@codeaurora.org>,
        davem@davemloft.net, kuba@kernel.org, elder@kernel.org,
        cpratapa@codeaurora.org, subashab@codeaurora.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1614012946-23506-1-git-send-email-sharathv@codeaurora.org>
 <1614012946-23506-3-git-send-email-sharathv@codeaurora.org>
From:   Alex Elder <elder@ieee.org>
Message-ID: <f968761d-2567-4538-c4cd-e1cb66d47bcd@ieee.org>
Date:   Mon, 22 Feb 2021 11:55:09 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <1614012946-23506-3-git-send-email-sharathv@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/22/21 10:55 AM, Sharath Chandra Vurukala wrote:
> Adding support for processing of Mapv5 downlink packets.
> It involves parsing the Mapv5 packet and checking the csum header
> to know whether the hardware has validated the checksum and is
> valid or not.
> 
> Based on the checksum valid bit the corresponding stats are
> incremented and skb->ip_summed is marked either CHECKSUM_UNNECESSARY
> or left as CHEKSUM_NONE to let network stack revalidated the checksum
> and update the respective snmp stats.
> 
> Current MapV1 header has been modified, the reserved field in the
> Mapv1 header is now used for next header indication.
> 
> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org>
> ---

. . .


> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
> index 9661416..a6de521 100644
> --- a/include/linux/if_rmnet.h
> +++ b/include/linux/if_rmnet.h
> @@ -1,5 +1,5 @@
>   /* SPDX-License-Identifier: GPL-2.0-only
> - * Copyright (c) 2013-2019, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
>    */
>   
>   #ifndef _LINUX_IF_RMNET_H_
> @@ -8,11 +8,11 @@
>   struct rmnet_map_header {
>   #if defined(__LITTLE_ENDIAN_BITFIELD)
>   	u8  pad_len:6;
> -	u8  reserved_bit:1;
> +	u8  next_hdr:1;
>   	u8  cd_bit:1;
>   #elif defined (__BIG_ENDIAN_BITFIELD)
>   	u8  cd_bit:1;
> -	u8  reserved_bit:1;
> +	u8  next_hdr:1;
>   	u8  pad_len:6;
>   #else
>   #error	"Please fix <asm/byteorder.h>"

. . .

I know that KS said he is "not convinced that it is
helping improve anything" and that it "just adds a
big overhead of testing everything again without
any improvement of performance or readability of
code."  But I will ask again that these structures
be redefined to use host byte-order masks and
structure fields with clearly defined endianness.

I strongly disagree with the statement from KS.
Specifically I feel the whole notion of "bit field
endianness" is not obvious, and makes it harder than
necessary to understand how the bits are laid out
in memory.  It also obscures in code that bit fields
have certain properties that are different from other
"normal" struct field types (such as alignment, size,
or atomicity of the field).  And I say this despite
knowing this pattern is used elsewhere in the
networking code.

In the first version of the series, Jakub asked that
the conversion be done.  I offered to implement the
change to the existing code, and that offer stands.
I can do so fairly quickly if you would like to have
it soon to build upon.

Either way, I would like a chance to review the
rest of this series, but I'd like to get this issue
resolved (either decide it must be done or not)
before I spend more time on that.

Thanks.

					-Alex