Message ID | 1558467302-17072-1-git-send-email-subashab@codeaurora.org |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | [RFC] net: qualcomm: rmnet: Move common struct definitions to include | expand |
On Tue, May 21, 2019 at 9:35 PM Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> wrote: > > Create if_rmnet.h and move the rmnet MAP packet structs to this > common include file. To account for portability, add little and > big endian bitfield definitions similar to the ip & tcp headers. > > The definitions in the headers can now be re-used by the > upcoming ipa driver series as well as qmi_wwan. > > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> > --- > This patch is an alternate implementation of the series posted by Elder. > This eliminates the changes needed in the rmnet packet parsing > while maintaining portability. > --- I think I'd just duplicate the structure definitions then, to avoid having the bitfield definitions in a common header and using them in the new driver. Arnd
On Tue 21 May 13:45 PDT 2019, Arnd Bergmann wrote: > On Tue, May 21, 2019 at 9:35 PM Subash Abhinov Kasiviswanathan > <subashab@codeaurora.org> wrote: > > > > Create if_rmnet.h and move the rmnet MAP packet structs to this > > common include file. To account for portability, add little and > > big endian bitfield definitions similar to the ip & tcp headers. > > > > The definitions in the headers can now be re-used by the > > upcoming ipa driver series as well as qmi_wwan. > > > > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> > > --- > > This patch is an alternate implementation of the series posted by Elder. > > This eliminates the changes needed in the rmnet packet parsing > > while maintaining portability. > > --- > > I think I'd just duplicate the structure definitions then, to avoid having > the bitfield definitions in a common header and using them in the new > driver. > Doing would allow each driver to represent the bits as suitable, at the cost of some duplication and confusion. Confusion, because it doesn't resolve the question of what the right bit order actually is. Subash stated yesterday that bit 0 is "CD", which in the current struct is represented as the 8th bit, while Alex's patch changes the definition so that this bit is the lsb. I.e. I read Subash answer as confirming that patch 1/8 from Alex is correct. Subash, as we're not addressing individual bits in this machine, so given a pointer map_hdr to a struct rmnet_map_header, which of the following ways would give you the correct value of pad_len: u8 p = *(char*)map_hdr; pad_len = p & 0x3f; or: u8 p = *(char*)map_hdr; pad_len = p >> 2; PS. I do prefer the two drivers share the definition of these structures... Regards, Bjorn
On 5/21/19 4:08 PM, Bjorn Andersson wrote: > On Tue 21 May 13:45 PDT 2019, Arnd Bergmann wrote: > >> On Tue, May 21, 2019 at 9:35 PM Subash Abhinov Kasiviswanathan >> <subashab@codeaurora.org> wrote: >>> >>> Create if_rmnet.h and move the rmnet MAP packet structs to this >>> common include file. To account for portability, add little and >>> big endian bitfield definitions similar to the ip & tcp headers. >>> >>> The definitions in the headers can now be re-used by the >>> upcoming ipa driver series as well as qmi_wwan. >>> >>> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> >>> --- >>> This patch is an alternate implementation of the series posted by Elder. >>> This eliminates the changes needed in the rmnet packet parsing >>> while maintaining portability. >>> --- >> >> I think I'd just duplicate the structure definitions then, to avoid having >> the bitfield definitions in a common header and using them in the new >> driver. >> > > Doing would allow each driver to represent the bits as suitable, at the > cost of some duplication and confusion. Confusion, because it doesn't > resolve the question of what the right bit order actually is. I have exchanged a few private messages with Subash. He has said that it is the high-order bit that indicates whether a QMAP packet contains a command (1) or data (0). That bit might be extracted this way: u8 byte = *(u8 *)skb->data; bool command = !!(byte & 0x80); Subash, if you don't mind, please acknowledge that again here so everyone knows. What this means is that the first patch in my series is wrong, because I misinterpreted the documentation, which indicated bit 0 was the command/data bit. (I presume this is the first bit that travels over the wire, and is not referring to the conventionally-understood lowest bit position in the first byte.) My plan is, as I said in a previous message: - Remove the first patch (that switches the bit-fields) - Adjust the subsequent patches to use correct field masks - Re-send the series as v2, with Bjorn's Reviewed-by. > Subash stated yesterday that bit 0 is "CD", which in the current struct > is represented as the 8th bit, while Alex's patch changes the definition > so that this bit is the lsb. I.e. I read Subash answer as confirming > that patch 1/8 from Alex is correct. I'm not sure about that but I don't want to confuse things further. > Subash, as we're not addressing individual bits in this machine, so > given a pointer map_hdr to a struct rmnet_map_header, which of the > following ways would give you the correct value of pad_len: > > u8 p = *(char*)map_hdr; > pad_len = p & 0x3f; > > or: > > u8 p = *(char*)map_hdr; > pad_len = p >> 2; My new understanding is it's the latter. > PS. I do prefer the two drivers share the definition of these > structures... I agree with you completely. I don't think it makes sense to have two definitions of the same structure. Subash wants to reduce the impact my changes have on the "rmnet" driver, but duplicating things makes things worse, not better. The IPA driver *assumes* it is talking to the rmnet driver; their interface definition should be common... -Alex > Regards, > Bjorn >
>>> I think I'd just duplicate the structure definitions then, to avoid >>> having >>> the bitfield definitions in a common header and using them in the new >>> driver. >>> >> >> Doing would allow each driver to represent the bits as suitable, at >> the >> cost of some duplication and confusion. Confusion, because it doesn't >> resolve the question of what the right bit order actually is. > If we duplicate this definition, then it will be the third instance of map header in kernel (1 in rmnet, 1 in qmi_wwan, 1 in ipa). > Subash, if you don't mind, please acknowledge that again here > so everyone knows. > >> Subash stated yesterday that bit 0 is "CD", which in the current >> struct >> is represented as the 8th bit, while Alex's patch changes the >> definition >> so that this bit is the lsb. I.e. I read Subash answer as confirming >> that patch 1/8 from Alex is correct. > > I'm not sure about that but I don't want to confuse things further. > Data over the wire is always big endian a.k.a network byte order. When looking at the bits in the order in which they are seen in the network, the command_data bit is the 0th bit. This is what is present in the rmnet documentation which I had shared. The struct definitions which are in the rmnet_map.h headers are in little endian because that is what my ARM64 devices are using. In little endian, the bits in the byte are read in the opposite order i.e. most significant bit (7th) is command_data. The addition of definitions for big endian in if_rmnet.h should now cover any host supporting big endian bit schemes in addition to little endian systems like ARM64. > I have exchanged a few private messages with Subash. He has said > that it is the high-order bit that indicates whether a QMAP packet > contains a command (1) or data (0). That bit might be extracted > this way: > > u8 byte = *(u8 *)skb->data; > bool command = !!(byte & 0x80); > !!(byte & 0x80) gives the correct value of command_data bit. >> Subash, as we're not addressing individual bits in this machine, so >> given a pointer map_hdr to a struct rmnet_map_header, which of the >> following ways would give you the correct value of pad_len: >> >> u8 p = *(char*)map_hdr; >> pad_len = p & 0x3f; >> p & 0x3f gives the correct value of pad_len. >> or: >> >> u8 p = *(char*)map_hdr; >> pad_len = p >> 2; > > My new understanding is it's the latter. > >> PS. I do prefer the two drivers share the definition of these >> structures... > > I agree with you completely. I don't think it makes sense to > have two definitions of the same structure. Subash wants to > reduce the impact my changes have on the "rmnet" driver, but > duplicating things makes things worse, not better. The IPA > driver *assumes* it is talking to the rmnet driver; their > interface definition should be common... > That is right. If David has no objections to this patch, then Alex can just rebase the ipa changes over this instead of his rmnet series.
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h index 884f1f5..991d7e2 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h @@ -12,6 +12,7 @@ #ifndef _RMNET_MAP_H_ #define _RMNET_MAP_H_ +#include <linux/if_rmnet.h> struct rmnet_map_control_command { u8 command_name; @@ -39,30 +40,6 @@ enum rmnet_map_commands { RMNET_MAP_COMMAND_ENUM_LENGTH }; -struct rmnet_map_header { - u8 pad_len:6; - u8 reserved_bit:1; - u8 cd_bit:1; - u8 mux_id; - __be16 pkt_len; -} __aligned(1); - -struct rmnet_map_dl_csum_trailer { - u8 reserved1; - u8 valid:1; - u8 reserved2:7; - u16 csum_start_offset; - u16 csum_length; - __be16 csum_value; -} __aligned(1); - -struct rmnet_map_ul_csum_header { - __be16 csum_start_offset; - u16 csum_insert_offset:14; - u16 udp_ip4_ind:1; - u16 csum_enabled:1; -} __aligned(1); - #define RMNET_MAP_GET_MUX_ID(Y) (((struct rmnet_map_header *) \ (Y)->data)->mux_id) #define RMNET_MAP_GET_CD_BIT(Y) (((struct rmnet_map_header *) \ diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h new file mode 100644 index 0000000..852a1f68 --- /dev/null +++ b/include/linux/if_rmnet.h @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: GPL-2.0-only + * Copyright (c) 2013-2019, The Linux Foundation. All rights reserved. + */ + +#ifndef _LINUX_IF_RMNET_H_ +#define _LINUX_IF_RMNET_H_ + +struct rmnet_map_header { +#if defined(__LITTLE_ENDIAN_BITFIELD) + u8 pad_len:6; + u8 reserved_bit:1; + u8 cd_bit:1; +#elif defined (__BIG_ENDIAN_BITFIELD) + u8 cd_bit:1; + u8 reserved_bit:1; + u8 pad_len:6; +#else +#error "Please fix <asm/byteorder.h>" +#endif + u8 mux_id; + __be16 pkt_len; +} __aligned(1); + +struct rmnet_map_dl_csum_trailer { + u8 reserved1; +#if defined(__LITTLE_ENDIAN_BITFIELD) + u8 valid:1; + u8 reserved2:7; +#elif defined (__BIG_ENDIAN_BITFIELD) + u8 reserved2:7; + u8 valid:1; +#else +#error "Please fix <asm/byteorder.h>" +#endif + u16 csum_start_offset; + u16 csum_length; + __be16 csum_value; +} __aligned(1); + +struct rmnet_map_ul_csum_header { + __be16 csum_start_offset; +#if defined(__LITTLE_ENDIAN_BITFIELD) + u16 csum_insert_offset:14; + u16 udp_ip4_ind:1; + u16 csum_enabled:1; +#elif defined (__BIG_ENDIAN_BITFIELD) + u16 csum_enabled:1; + u16 udp_ip4_ind:1; + u16 csum_insert_offset:14; +#else +#error "Please fix <asm/byteorder.h>" +#endif +} __aligned(1); + +#endif /* !(_LINUX_IF_RMNET_H_) */ \ No newline at end of file
Create if_rmnet.h and move the rmnet MAP packet structs to this common include file. To account for portability, add little and big endian bitfield definitions similar to the ip & tcp headers. The definitions in the headers can now be re-used by the upcoming ipa driver series as well as qmi_wwan. Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> --- This patch is an alternate implementation of the series posted by Elder. This eliminates the changes needed in the rmnet packet parsing while maintaining portability. --- drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 25 +---------- include/linux/if_rmnet.h | 55 +++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 24 deletions(-) create mode 100644 include/linux/if_rmnet.h