| Message ID | 20190512012508.10608-3-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: introduce Qualcomm IPA driver | expand |
On Sat, 2019-05-11 at 20:24 -0500, Alex Elder wrote:
> include/soc/qcom/rmnet.h
Should this file be added to the MAINTAINERS file
update in patch 16/18 ?
On 5/11/19 9:34 PM, Joe Perches wrote: > On Sat, 2019-05-11 at 20:24 -0500, Alex Elder wrote: >> include/soc/qcom/rmnet.h > > Should this file be added to the MAINTAINERS file > update in patch 16/18 ? Sure, that's a good point. I'll add it when I submit a v2. Thank you. -Alex
On Sun, May 12, 2019 at 3:25 AM Alex Elder <elder@linaro.org> wrote: > diff --git a/include/soc/qcom/rmnet.h b/include/soc/qcom/rmnet.h > new file mode 100644 > index 000000000000..80dcd6e68c3d > --- /dev/null > +++ b/include/soc/qcom/rmnet.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved. > + * Copyright (C) 2018-2019 Linaro Ltd. > + */ > +#ifndef _SOC_QCOM_RMNET_H_ > +#define _SOC_QCOM_RMNET_H_ > + > +#include <linux/types.h> > + > +/* Header structure that precedes packets in ETH_P_MAP protocol */ > +struct rmnet_map_header { > + u8 pad_len : 6; > + u8 reserved_bit : 1; > + u8 cd_bit : 1; > + u8 mux_id; > + __be16 pkt_len; > +} __aligned(1); If we move this into include/soc/, I want the structure to be portable, and avoid the bit fields. Please use mask/shift operations or the include/linux/bits.h macros instead to make this work with big-endian kernels. Arnd
On 5/15/19 1:59 AM, Arnd Bergmann wrote: > On Sun, May 12, 2019 at 3:25 AM Alex Elder <elder@linaro.org> wrote: > >> diff --git a/include/soc/qcom/rmnet.h b/include/soc/qcom/rmnet.h >> new file mode 100644 >> index 000000000000..80dcd6e68c3d >> --- /dev/null >> +++ b/include/soc/qcom/rmnet.h >> @@ -0,0 +1,38 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved. >> + * Copyright (C) 2018-2019 Linaro Ltd. >> + */ >> +#ifndef _SOC_QCOM_RMNET_H_ >> +#define _SOC_QCOM_RMNET_H_ >> + >> +#include <linux/types.h> >> + >> +/* Header structure that precedes packets in ETH_P_MAP protocol */ >> +struct rmnet_map_header { >> + u8 pad_len : 6; >> + u8 reserved_bit : 1; >> + u8 cd_bit : 1; >> + u8 mux_id; >> + __be16 pkt_len; >> +} __aligned(1); > > If we move this into include/soc/, I want the structure to be portable, > and avoid the bit fields. Please use mask/shift operations or the > include/linux/bits.h macros instead to make this work with big-endian > kernels. Sure, I'll do that. I did that everywhere else in the driver, but here I just tried to preserve the original code as I moved it. I will update at least these structures, and all existing code (plus the IPA code) to use fields masks. -Alex > > Arnd >
>>> +#ifndef _SOC_QCOM_RMNET_H_ >>> +#define _SOC_QCOM_RMNET_H_ >>> + >>> +#include <linux/types.h> >>> + >>> +/* Header structure that precedes packets in ETH_P_MAP protocol */ >>> +struct rmnet_map_header { >>> + u8 pad_len : 6; >>> + u8 reserved_bit : 1; >>> + u8 cd_bit : 1; >>> + u8 mux_id; >>> + __be16 pkt_len; >>> +} __aligned(1); >> >> If we move this into include/soc/, I want the structure to be >> portable, >> and avoid the bit fields. Please use mask/shift operations or the >> include/linux/bits.h macros instead to make this work with big-endian >> kernels. > > Sure, I'll do that. I did that everywhere else in the driver, > but here I just tried to preserve the original code as I moved > it. I will update at least these structures, and all existing > code (plus the IPA code) to use fields masks. > > -Alex >> >> Arnd >> Hi Alex Could we instead have the rmnet header definition in include/linux/if_rmnet.h -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 5/15/19 8:09 PM, Subash Abhinov Kasiviswanathan wrote: . . . > Hi Alex > > Could we instead have the rmnet header definition in > include/linux/if_rmnet.h I have no objection to that, but I don't actually know what the criteria are for putting a file in that directory. Glancing at other "if_*" files there it seems sensible, but because I don't know, I'd like to have a little better justification. Can you provide a good explanation about why these definitions belong in "include/linux/if_rmnet.h" instead of "include/soc/qcom/rmnet.h"? Thanks. -Alex
On 2019-05-17 11:27, Alex Elder wrote: > On 5/15/19 8:09 PM, Subash Abhinov Kasiviswanathan wrote: > . . . >> Hi Alex >> >> Could we instead have the rmnet header definition in >> include/linux/if_rmnet.h > > I have no objection to that, but I don't actually know what > the criteria are for putting a file in that directory. > > Glancing at other "if_*" files there it seems sensible, but > because I don't know, I'd like to have a little better > justification. > > Can you provide a good explanation about why these > definitions belong in "include/linux/if_rmnet.h" instead > of "include/soc/qcom/rmnet.h"? > > Thanks. > > -Alex rmnet was designed similar to vlan / macvlan / ipvlan / bridge. These drivers support creation of virtual netdevices, define custom rtnl_link_ops, expose netlink attributes to uapi via if_link.h and register rx_handlers. They expose some common structs and helpers via if_vlan.h / if_macvlan.h / if_bridge.h. I would prefer rmnet to use if_rmnet.h similar to them. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 5/17/19 1:08 PM, Subash Abhinov Kasiviswanathan wrote: > On 2019-05-17 11:27, Alex Elder wrote: . . . >> Can you provide a good explanation about why these >> definitions belong in "include/linux/if_rmnet.h" instead >> of "include/soc/qcom/rmnet.h"? >> >> Thanks. >> >> -Alex > > rmnet was designed similar to vlan / macvlan / ipvlan / bridge. > These drivers support creation of virtual netdevices, > define custom rtnl_link_ops, expose netlink attributes to > uapi via if_link.h and register rx_handlers. > > They expose some common structs and helpers via if_vlan.h / > if_macvlan.h / if_bridge.h. I would prefer rmnet to use if_rmnet.h > similar to them. OK, I will name the file "include/linux/if_rmnet.h" as you suggest. It will still only define the three structures that I need in the IPA driver; I won't expose anything else from the rmnet_data driver. I will mention now that, to facilitate addressing Arnd's concerns about the portability of using C bit-fields in these structures, I made a set of other changes (including a bug fix in one of the structure definitions). As a preview, here are the subject lines for that series: net: qualcomm: rmnet: fix struct rmnet_map_header net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros net: qualcomm: rmnet: use field masks instead of C bit-fields net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer soc: qcom: ipa: get rid of a variable in rmnet_map_ipv4_ul_csum_header() net: create "include/linux/if_rmnet.h" I will be posting that as a separate series now and will have the IPA driver series mention a dependence on that. -Alex
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c index 11167abe5934..3aa79b7ed539 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c @@ -17,6 +17,7 @@ #include <linux/netdev_features.h> #include <linux/if_arp.h> #include <net/sock.h> +#include <soc/qcom/rmnet.h> #include "rmnet_private.h" #include "rmnet_config.h" #include "rmnet_vnd.h" diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h index 884f1f52dcc2..39d0be99a771 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h @@ -39,30 +39,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/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c index f6cf59aee212..54b86a8be570 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c @@ -11,6 +11,7 @@ */ #include <linux/netdevice.h> +#include <soc/qcom/rmnet.h> #include "rmnet_config.h" #include "rmnet_map.h" #include "rmnet_private.h" diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c index 57a9c314a665..e3fb4035820c 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c @@ -17,6 +17,7 @@ #include <linux/ip.h> #include <linux/ipv6.h> #include <net/ip6_checksum.h> +#include <soc/qcom/rmnet.h> #include "rmnet_config.h" #include "rmnet_map.h" #include "rmnet_private.h" diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c index d11c16aeb19a..b8df36e827d4 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c @@ -17,6 +17,7 @@ #include <linux/etherdevice.h> #include <linux/if_arp.h> #include <net/pkt_sched.h> +#include <soc/qcom/rmnet.h> #include "rmnet_config.h" #include "rmnet_handlers.h" #include "rmnet_private.h" diff --git a/include/soc/qcom/rmnet.h b/include/soc/qcom/rmnet.h new file mode 100644 index 000000000000..80dcd6e68c3d --- /dev/null +++ b/include/soc/qcom/rmnet.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved. + * Copyright (C) 2018-2019 Linaro Ltd. + */ +#ifndef _SOC_QCOM_RMNET_H_ +#define _SOC_QCOM_RMNET_H_ + +#include <linux/types.h> + +/* Header structure that precedes packets in ETH_P_MAP protocol */ +struct rmnet_map_header { + u8 pad_len : 6; + u8 reserved_bit : 1; + u8 cd_bit : 1; + u8 mux_id; + __be16 pkt_len; +} __aligned(1); + +/* Checksum offload metadata header for outbound packets*/ +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); + +/* Checksum offload metadata trailer for inbound packets */ +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); + +#endif /* _SOC_QCOM_RMNET_H_ */
The IPA driver requires some (but not all) symbols defined in "drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h". Create a new public header file "include/soc/qcom/rmnet.h" and move the needed definitions there. Signed-off-by: Alex Elder <elder@linaro.org> --- .../ethernet/qualcomm/rmnet/rmnet_handlers.c | 1 + .../net/ethernet/qualcomm/rmnet/rmnet_map.h | 24 ------------ .../qualcomm/rmnet/rmnet_map_command.c | 1 + .../ethernet/qualcomm/rmnet/rmnet_map_data.c | 1 + .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c | 1 + include/soc/qcom/rmnet.h | 38 +++++++++++++++++++ 6 files changed, 42 insertions(+), 24 deletions(-) create mode 100644 include/soc/qcom/rmnet.h -- 2.20.1