Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for PVC devices

On Fri, 4 Sep 2020 18:57:27 -0700 Xie He wrote:

On Fri, Sep 4, 2020 at 6:28 PM Xie He [off-list ref] wrote:

quoted

The HDLC device is not actually prepending any header when it is used
with this driver. When the PVC device has prepended its header and
handed over the skb to the HDLC device, the HDLC device just hands it
over to the hardware driver for transmission without prepending any
header.

If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we
can see there is no "header_ops" implemented in these two files and
all "skb_push" happen in the PVC device in hdlc_fr.c.  

I want to provide a little more information about the flow after an
HDLC device's ndo_start_xmit is called.

An HDLC hardware driver's ndo_start_xmit is required to point to
hdlc_start_xmit in hdlc.c. When a HDLC device receives a call to its
ndo_start_xmit, hdlc_start_xmit will check if the protocol driver has
provided a xmit function. If it has provided this function,
hdlc_start_xmit will call it to start transmission. If it has not,
hdlc_start_xmit will directly call the hardware driver's function to
start transmission. This driver (hdlc_fr) has not provided a xmit
function in its hdlc_proto struct, so hdlc_start_xmit will directly
call the hardware driver's function to transmit.

So no header will be prepended after ndo_start_xmit is called.

There would not be any header prepended before ndo_start_xmit is
called, either, because there is no header_ops implemented in either
hdlc.c or hdlc_fr.c.

Thank you for the detailed explanation.

On Fri, Sep 4, 2020 at 6:28 PM Xie He [off-list ref] wrote:

quoted

Thank you for your email, Jakub!

On Fri, Sep 4, 2020 at 3:14 PM Jakub Kicinski [off-list ref] wrote:  

quoted

Since this is a tunnel protocol on top of HDLC interfaces, and
hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually
set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if
hdlc devices actually prepend 16 bytes of header, though.  

The HDLC device is not actually prepending any header when it is used
with this driver. When the PVC device has prepended its header and
handed over the skb to the HDLC device, the HDLC device just hands it
over to the hardware driver for transmission without prepending any
header.

If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we
can see there is no "header_ops" implemented in these two files and
all "skb_push" happen in the PVC device in hdlc_fr.c.

For this reason, I have previously submitted a patch to change the
value of hard_header_len of the HDLC device from 16 to 0, because it
is not actually used.

See:
2b7bcd967a0f (drivers/net/wan/hdlc: Change the default of hard_header_len to 0)

Ah, sorry.. the tree I was looking at did not have this commit.

quoted

quoted

quoted

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 9acad651ea1f..12b35404cd8e 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c

@@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev)
 {
      dev->type = ARPHRD_DLCI;
      dev->flags = IFF_POINTOPOINT;
-     dev->hard_header_len = 10;
+     dev->hard_header_len = 0;  

Is there a need to set this to 0? Will it not be zero after allocation?  

Oh. I understand your point. Theoretically we don't need to set it to
0 because it already has the default value of 0. I'm setting it to 0
only because I want to tell future developers that this value is
intentionally set to 0, and it is not carelessly missed out.  

Sounds fair.

Applied to net, thank you!