| Message ID | 1512369428-20455-2-git-send-email-subashab@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: qualcomm: rmnet: Configuration options | expand |
From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> Date: Sun, 3 Dec 2017 23:37:03 -0700 > Only the success and consumed entries were actually in use. > Use standard error codes instead. > > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> That is definitely not the only thing this change is doing: > @@ -126,12 +126,12 @@ static int rmnet_map_egress_handler(struct sk_buff *skb, > > if (skb_headroom(skb) < required_headroom) { > if (pskb_expand_head(skb, required_headroom, 0, GFP_KERNEL)) > - return RMNET_MAP_CONSUMED; > + goto fail; > } > > map_header = rmnet_map_add_map_header(skb, additional_header_len, 0); > if (!map_header) > - return RMNET_MAP_CONSUMED; > + goto fail; > Previously these paths would return "RMNET_MAP_CONSUMED": > @@ -209,17 +213,8 @@ void rmnet_egress_handler(struct sk_buff *skb) > } > > if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) { > - switch (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) { > - case RMNET_MAP_CONSUMED: > - return; > - > - case RMNET_MAP_SUCCESS: > - break; > - > - default: > - kfree_skb(skb); > + if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) > return; > - } > } Causing this code to return. Now, instead the code jumps to fail: > @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct sk_buff *skb, > > skb->protocol = htons(ETH_P_MAP); > > - return RMNET_MAP_SUCCESS; > + return 0; > + > +fail: > + kfree_skb(skb); > + return -ENOMEM; > } > Which frees the SKB. This is a behavioral change, perhaps a bug fix. Well, nobody knows because you do not explain this at all in your commit message. Do not mix functional changes with cleanups. If you want to change how freeing the SKB is done, do it in a separate change from the patch that removes this enumeration. Thank you.
> This will crash, you didn't test the new code path that does the > kfree_skb(). > > You should use an SKB helper function which will realloc the headroom > if ETH_HLEN is not available, instead of failing. ... > That is definitely not the only thing this change is doing: > Previously these paths would return "RMNET_MAP_CONSUMED": > Causing this code to return. > Now, instead the code jumps to fail: > >> @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct >> sk_buff *skb, >> >> skb->protocol = htons(ETH_P_MAP); >> >> - return RMNET_MAP_SUCCESS; >> + return 0; >> + >> +fail: >> + kfree_skb(skb); >> + return -ENOMEM; >> } >> > > Which frees the SKB. > > This is a behavioral change, perhaps a bug fix. > > Well, nobody knows because you do not explain this at all in your > commit message. > > Do not mix functional changes with cleanups. If you want to change how > freeing the SKB is done, do it in a separate change from the patch that > removes this enumeration. > > Thank you. Hi David I will send the change in freeing behavior as a bug fix for net and then post updates on this series after it.
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c index 29842cc..1e1ea10 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c @@ -126,12 +126,12 @@ static int rmnet_map_egress_handler(struct sk_buff *skb, if (skb_headroom(skb) < required_headroom) { if (pskb_expand_head(skb, required_headroom, 0, GFP_KERNEL)) - return RMNET_MAP_CONSUMED; + goto fail; } map_header = rmnet_map_add_map_header(skb, additional_header_len, 0); if (!map_header) - return RMNET_MAP_CONSUMED; + goto fail; if (port->egress_data_format & RMNET_EGRESS_FORMAT_MUXING) { if (mux_id == 0xff) @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct sk_buff *skb, skb->protocol = htons(ETH_P_MAP); - return RMNET_MAP_SUCCESS; + return 0; + +fail: + kfree_skb(skb); + return -ENOMEM; } static void @@ -209,17 +213,8 @@ void rmnet_egress_handler(struct sk_buff *skb) } if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) { - switch (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) { - case RMNET_MAP_CONSUMED: - return; - - case RMNET_MAP_SUCCESS: - break; - - default: - kfree_skb(skb); + if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) return; - } } rmnet_vnd_tx_fixup(skb, orig_dev); diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h index 3af3fe7..4df359d 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h @@ -30,15 +30,6 @@ struct rmnet_map_control_command { }; } __aligned(1); -enum rmnet_map_results { - RMNET_MAP_SUCCESS, - RMNET_MAP_CONSUMED, - RMNET_MAP_GENERAL_FAILURE, - RMNET_MAP_NOT_ENABLED, - RMNET_MAP_FAILED_AGGREGATION, - RMNET_MAP_FAILED_MUX -}; - enum rmnet_map_commands { RMNET_MAP_COMMAND_NONE, RMNET_MAP_COMMAND_FLOW_DISABLE,
Only the success and consumed entries were actually in use. Use standard error codes instead. Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> --- .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 21 ++++++++------------- drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 9 --------- 2 files changed, 8 insertions(+), 22 deletions(-)