Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 18 Feb 2021 10:57:43 +0200
From:   Leon Romanovsky <leon@kernel.org>
To:     Xie He <xie.he.0141@gmail.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-x25@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Martin Schiller <ms@dev.tdt.de>,
        Krzysztof Halasa <khc@pm.waw.pl>,
        Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org
Subject: Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
Message-ID: <YC4sB9OCl5mm3JAw@unreal>
References: <20210216201813.60394-1-xie.he.0141@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210216201813.60394-1-xie.he.0141@gmail.com>
Precedence: bulk

On Tue, Feb 16, 2021 at 12:18:13PM -0800, Xie He wrote:
> When sending packets, we will first hand over the (L3) packets to the
> LAPB module. The LAPB module will then hand over the corresponding LAPB
> (L2) frames back to us for us to transmit.
>
> The LAPB module can also emit LAPB (L2) frames at any time, even without
> an (L3) packet currently being sent on the device. This happens when the
> LAPB module tries to send (L3) packets queued up in its internal queue,
> or when the LAPB module decides to send some (L2) control frame.
>
> This means we need to have a queue for these outgoing LAPB (L2) frames,
> otherwise frames can be dropped if sent when the hardware driver is
> already busy in transmitting. The queue needs to be controlled by
> the hardware driver's netif_stop_queue and netif_wake_queue calls.
> Therefore, we need to use the device's qdisc TX queue for this purpose.
> However, currently outgoing LAPB (L2) frames are not queued.
>
> On the other hand, outgoing (L3) packets (before they are handed over
> to the LAPB module) don't need to be queued, because the LAPB module
> already has an internal queue for them, and is able to queue new outgoing
> (L3) packets at any time. However, currently outgoing (L3) packets are
> being queued in the device's qdisc TX queue, which is controlled by
> the hardware driver's netif_stop_queue and netif_wake_queue calls.
> This is unnecessary and meaningless.
>
> To fix these issues, we can split the HDLC device into two devices -
> a virtual X.25 device and the actual HDLC device, use the virtual X.25
> device to send (L3) packets and then use the actual HDLC device to
> queue LAPB (L2) frames. The outgoing (L2) LAPB queue will be controlled
> by the hardware driver's netif_stop_queue and netif_wake_queue calls,
> while outgoing (L3) packets will not be affected by these calls.
>
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---

<...>

> +static void x25_setup_virtual_dev(struct net_device *dev)
> +{
> +	dev->netdev_ops	     = &hdlc_x25_netdev_ops;
> +	dev->type            = ARPHRD_X25;
> +	dev->addr_len        = 0;
> +	dev->hard_header_len = 0;
> +	dev->mtu             = HDLC_MAX_MTU;
> +
> +	/* When transmitting data:
> +	 * first we'll remove a pseudo header of 1 byte,
> +	 * then the LAPB module will prepend an LAPB header of at most 3 bytes.
> +	 */
> +	dev->needed_headroom = 3 - 1;

It is nice that you are resending your patch without the resolution.
However it will be awesome if you don't ignore review comments and fix this "3 - 1"
by writing solid comment above.

Thanks and good luck.