Re: [PATCH v2] drivers/net/wan/x25_asy: Fix to make it work

[Martin Schiller <ms@dev.tdt.de>]

On 2020-07-17 01:44, Xie He wrote:
> This driver is not working because of problems of its receiving code.
> This patch fixes it to make it work.
> 
> When the driver receives an LAPB frame, it should first pass the frame
> to the LAPB module to process. After processing, the LAPB module passes
> the data (the packet) back to the driver, the driver should then add a
> one-byte pseudo header and pass the data to upper layers.
> 
> The changes to the "x25_asy_bump" function and the
> "x25_asy_data_indication" function are to correctly implement this
> procedure.
> 
> Also, the "x25_asy_unesc" function ignores any frame that is shorter
> than 3 bytes. However the shortest frames are 2-byte long. So we need
> to change it to allow 2-byte frames to pass.
> 
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
> 
> Change from v1:
> Added skb_cow before skb_push to ensure skb_push will succeed
> according the suggestion of Eric Dumazet.
> 
> Hi Eric Dumazet and Martin Schiller,
> Can you review this patch again and see if it is OK for me to include
> your names in a "Signed-off-by", "Reviewed-by" or "Acked-by" tag?
> Thank you!
> 
> Hi All,
> I'm happy to answer any questions you might have and make improvements
> according to your suggestions. Thanks!
> 
> ---
>  drivers/net/wan/x25_asy.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
> index 69773d228ec1..84640a0c13f3 100644
> --- a/drivers/net/wan/x25_asy.c
> +++ b/drivers/net/wan/x25_asy.c
> @@ -183,7 +183,7 @@ static inline void x25_asy_unlock(struct x25_asy 
> *sl)
>  	netif_wake_queue(sl->dev);
>  }
> 
> -/* Send one completely decapsulated IP datagram to the IP layer. */
> +/* Send an LAPB frame to the LAPB module to process. */
> 
>  static void x25_asy_bump(struct x25_asy *sl)
>  {
> @@ -195,13 +195,12 @@ static void x25_asy_bump(struct x25_asy *sl)
>  	count = sl->rcount;
>  	dev->stats.rx_bytes += count;
> 
> -	skb = dev_alloc_skb(count+1);
> +	skb = dev_alloc_skb(count);
>  	if (skb == NULL) {
>  		netdev_warn(sl->dev, "memory squeeze, dropping packet\n");
>  		dev->stats.rx_dropped++;
>  		return;
>  	}
> -	skb_push(skb, 1);	/* LAPB internal control */
>  	skb_put_data(skb, sl->rbuff, count);
>  	skb->protocol = x25_type_trans(skb, sl->dev);
>  	err = lapb_data_received(skb->dev, skb);
> @@ -209,7 +208,6 @@ static void x25_asy_bump(struct x25_asy *sl)
>  		kfree_skb(skb);
>  		printk(KERN_DEBUG "x25_asy: data received err - %d\n", err);
>  	} else {
> -		netif_rx(skb);
>  		dev->stats.rx_packets++;
>  	}
>  }
> @@ -356,12 +354,21 @@ static netdev_tx_t x25_asy_xmit(struct sk_buff 
> *skb,
>   */
> 
>  /*
> - *	Called when I frame data arrives. We did the work above - throw it
> - *	at the net layer.
> + *	Called when I frame data arrive. We add a pseudo header for upper
> + *	layers and pass it to upper layers.
>   */
> 
>  static int x25_asy_data_indication(struct net_device *dev, struct 
> sk_buff *skb)
>  {
> +	if (skb_cow(skb, 1)) {
> +		kfree_skb(skb);
> +		return NET_RX_DROP;
> +	}
> +	skb_push(skb, 1);
> +	skb->data[0] = X25_IFACE_DATA;
> +
> +	skb->protocol = x25_type_trans(skb, dev);
> +
>  	return netif_rx(skb);
>  }
> 
> @@ -657,7 +664,7 @@ static void x25_asy_unesc(struct x25_asy *sl,
> unsigned char s)
>  	switch (s) {
>  	case X25_END:
>  		if (!test_and_clear_bit(SLF_ERROR, &sl->flags) &&
> -		    sl->rcount > 2)
> +		    sl->rcount >= 2)
>  			x25_asy_bump(sl);
>  		clear_bit(SLF_ESCAPE, &sl->flags);
>  		sl->rcount = 0;

LGTM.

I have never used the driver, but the adjustments look conclusive. The
functionality is now comparable to the one in the drivers lapbether or
hdlc_x25.

Reviewed-by: Martin Schiller <ms@dev.tdt.de>