Messages in this thread | | | From | Xie He <> | Date | Fri, 4 Sep 2020 18:57:27 -0700 | Subject | Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for PVC devices |
| |
On Fri, Sep 4, 2020 at 6:28 PM Xie He <xie.he.0141@gmail.com> wrote: > > 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.
On Fri, Sep 4, 2020 at 6:28 PM Xie He <xie.he.0141@gmail.com> wrote: > > Thank you for your email, Jakub! > > On Fri, Sep 4, 2020 at 3:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > 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) > > > > 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.
| |