Re: [PATCH net-next 1/6] net: qualcomm: rmnet: Remove the rmnet_map_results enum

[David Miller <davem@davemloft.net>]

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.