On Mon 20 May 06:53 PDT 2019, Alex Elder wrote: > The following macros, defined in "rmnet_map.h", assume a socket > buffer is provided as an argument without any real indication this > is the case. > RMNET_MAP_GET_MUX_ID() > RMNET_MAP_GET_CD_BIT() > RMNET_MAP_GET_PAD() > RMNET_MAP_GET_CMD_START() > RMNET_MAP_GET_LENGTH() > What they hide is pretty trivial accessing of fields in a structure, > and it's much clearer to see this if we do these accesses directly. > > So rather than using these accessor macros, assign a local > variable of the map header pointer type to the socket buffer data > pointer, and derereference that pointer variable. > > Use the network byte order macros (e.g., ntohs()), not the Linux > byte order functions (e.g. be_to_cpu16()) to convert the big-endian > packet length field, to match the convention used elswhere in the > driver. > > There's no need to byte swap 0; it's all zeros irrespective of > endianness. >
Reviewed-by: Bjorn Andersson <[email protected]> > Signed-off-by: Alex Elder <[email protected]> > --- > drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 9 +++++---- > drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 12 ------------ > .../net/ethernet/qualcomm/rmnet/rmnet_map_command.c | 11 ++++++++--- > drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 4 ++-- > 4 files changed, 15 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c > b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c > index 11167abe5934..4c1b62b72504 100644 > --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c > +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c > @@ -65,20 +65,21 @@ static void > __rmnet_map_ingress_handler(struct sk_buff *skb, > struct rmnet_port *port) > { > + struct rmnet_map_header *map_header = (void *)skb->data; > struct rmnet_endpoint *ep; > u16 len, pad; > u8 mux_id; > > - if (RMNET_MAP_GET_CD_BIT(skb)) { > + if (map_header->cd_bit) { > if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS) > return rmnet_map_command(skb, port); > > goto free_skb; > } > > - mux_id = RMNET_MAP_GET_MUX_ID(skb); > - pad = RMNET_MAP_GET_PAD(skb); > - len = RMNET_MAP_GET_LENGTH(skb) - pad; > + mux_id = map_header->mux_id; > + pad = map_header->pad_len; > + len = ntohs(map_header->pkt_len) - pad; > > if (mux_id >= RMNET_MAX_LOGICAL_EP) > goto free_skb; > diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h > b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h > index b1ae9499c0b2..a30a7b405a11 100644 > --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h > +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h > @@ -63,18 +63,6 @@ struct rmnet_map_ul_csum_header { > 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 *) \ > - (Y)->data)->cd_bit) > -#define RMNET_MAP_GET_PAD(Y) (((struct rmnet_map_header *) \ > - (Y)->data)->pad_len) > -#define RMNET_MAP_GET_CMD_START(Y) ((struct rmnet_map_control_command *) \ > - ((Y)->data + \ > - sizeof(struct rmnet_map_header))) > -#define RMNET_MAP_GET_LENGTH(Y) (ntohs(((struct rmnet_map_header *) \ > - (Y)->data)->pkt_len)) > - > #define RMNET_MAP_COMMAND_REQUEST 0 > #define RMNET_MAP_COMMAND_ACK 1 > #define RMNET_MAP_COMMAND_UNSUPPORTED 2 > diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c > b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c > index f6cf59aee212..f675f47c3495 100644 > --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c > +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c > @@ -20,12 +20,13 @@ static u8 rmnet_map_do_flow_control(struct sk_buff *skb, > struct rmnet_port *port, > int enable) > { > + struct rmnet_map_header *map_header = (void *)skb->data; > struct rmnet_endpoint *ep; > struct net_device *vnd; > u8 mux_id; > int r; > > - mux_id = RMNET_MAP_GET_MUX_ID(skb); > + mux_id = map_header->mux_id; > > if (mux_id >= RMNET_MAX_LOGICAL_EP) { > kfree_skb(skb); > @@ -57,6 +58,7 @@ static void rmnet_map_send_ack(struct sk_buff *skb, > unsigned char type, > struct rmnet_port *port) > { > + struct rmnet_map_header *map_header = (void *)skb->data; > struct rmnet_map_control_command *cmd; > struct net_device *dev = skb->dev; > > @@ -66,7 +68,8 @@ static void rmnet_map_send_ack(struct sk_buff *skb, > > skb->protocol = htons(ETH_P_MAP); > > - cmd = RMNET_MAP_GET_CMD_START(skb); > + /* Command data immediately follows the header */ > + cmd = (struct rmnet_map_control_command *)(map_header + 1); > cmd->cmd_type = type & 0x03; > > netif_tx_lock(dev); > @@ -79,11 +82,13 @@ static void rmnet_map_send_ack(struct sk_buff *skb, > */ > void rmnet_map_command(struct sk_buff *skb, struct rmnet_port *port) > { > + struct rmnet_map_header *map_header = (void *)skb->data; > struct rmnet_map_control_command *cmd; > unsigned char command_name; > unsigned char rc = 0; > > - cmd = RMNET_MAP_GET_CMD_START(skb); > + /* Command data immediately follows the header */ > + cmd = (struct rmnet_map_control_command *)(map_header + 1); > command_name = cmd->command_name; > > switch (command_name) { > diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > index 57a9c314a665..498f20ba1826 100644 > --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c > @@ -323,7 +323,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, > return NULL; > > maph = (struct rmnet_map_header *)skb->data; > - packet_len = ntohs(maph->pkt_len) + sizeof(struct rmnet_map_header); > + packet_len = ntohs(maph->pkt_len) + sizeof(*maph); > > if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) > packet_len += sizeof(struct rmnet_map_dl_csum_trailer); > @@ -332,7 +332,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, > return NULL; > > /* Some hardware can send us empty frames. Catch them */ > - if (ntohs(maph->pkt_len) == 0) > + if (!maph->pkt_len) > return NULL; > > skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC); > -- > 2.20.1 >
