On Wed, 27 Jan 2021 01:07:47 -0800 Xie He wrote:
> An HDLC hardware driver may call netif_stop_queue to temporarily stop
> the TX queue when the hardware is busy sending a frame, and after the
> hardware has finished sending the frame, call netif_wake_queue to
> resume the TX queue.
>
> However, the LAPB module doesn't know about this. Whether or not the
> hardware driver has stopped the TX queue, the LAPB module still feeds
> outgoing frames to the hardware driver for transmission. This can cause
> frames to be dropped by the hardware driver.
>
> It's not easy to fix this issue in the LAPB module. We can indeed let the
> LAPB module check whether the TX queue has been stopped before feeding
> each frame to the hardware driver, but when the hardware driver resumes
> the TX queue, it's not easy to immediately notify the LAPB module and ask
> it to resume transmission.
>
> Instead, we can fix this issue at the hdlc_x25 layer, by using qdisc TX
> queues to queue outgoing LAPB frames. The qdisc TX queue will then
> automatically be controlled by netif_stop_queue and netif_wake_queue.
Noob question - could you point at or provide a quick guide to layering
here? I take there is only one netdev, and something maintains an
internal queue which is not stopped when HW driver stops the qdisc?
> This way, when sending, we will use the qdisc queue to queue and send
> the data twice: once as the L3 packet and then (after processed by the
> LAPB module) as an LAPB (L2) frame. This does not make the logic of the
> code messy, because when receiving, data are already "received" on the
> device twice: once as an LAPB (L2) frame and then (after processed by
> the LAPB module) as the L3 packet.
>
> Some more details about the code change:
>
> 1. dev_queue_xmit_nit is removed because we already have it when we send
> the skb through the qdisc TX queue (in xmit_one).
>
> 2. hdlc_type_trans is replaced by assigning skb->dev and skb->protocol
> directly. skb_reset_mac_header in hdlc_type_trans is no longer necessary
> because it will be called in __dev_queue_xmit.
Sounds like we're optimizing to prevent drops, and this was not
reported from production, rather thru code inspection. Ergo I think
net-next will be more appropriate here, unless Martin disagrees.
> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
> index bb164805804e..b7f2823bf100 100644
> --- a/drivers/net/wan/hdlc_x25.c
> +++ b/drivers/net/wan/hdlc_x25.c
> @@ -89,15 +89,10 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
>
> static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
> {
> - hdlc_device *hdlc = dev_to_hdlc(dev);
> -
> + skb->dev = dev;
> + skb->protocol = htons(ETH_P_HDLC);
> skb_reset_network_header(skb);
> - skb->protocol = hdlc_type_trans(skb, dev);
> -
> - if (dev_nit_active(dev))
> - dev_queue_xmit_nit(skb, dev);
> -
> - hdlc->xmit(skb, dev); /* Ignore return value :-( */
> + dev_queue_xmit(skb);
> }
>
>
> @@ -106,6 +101,12 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> int result;
>
> + if (skb->protocol == htons(ETH_P_HDLC)) {
> + hdlc_device *hdlc = dev_to_hdlc(dev);
> +
> + return hdlc->xmit(skb, dev);
> + }
> +
> /* There should be a pseudo header of 1 byte added by upper layers.
> * Check to make sure it is there before reading it.
> */
