From: Xie He <xie.he.0141@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Halasa <khc@pm.waw.pl>,
"David S. Miller" <davem@davemloft.net>,
Linux Kernel Network Developers <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Martin Schiller <ms@dev.tdt.de>,
Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Subject: Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for PVC devices
Date: Fri, 4 Sep 2020 18:57:27 -0700 [thread overview]
Message-ID: <CAJht_ENKyfMm7wAxcVSThEG63oVe72FvMs-5VaLWemKvveY+dQ@mail.gmail.com> (raw)
In-Reply-To: <CAJht_EN+=WTuduvm43_Lq=XWL78AcF5q6Zoyg8S5fao_udL=+Q@mail.gmail.com>
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.
next prev parent reply other threads:[~2020-09-05 1:57 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-03 0:06 [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for PVC devices Xie He
2020-09-04 22:14 ` Jakub Kicinski
2020-09-05 1:28 ` Xie He
2020-09-05 1:57 ` Xie He [this message]
2020-09-05 4:36 ` Jakub Kicinski
2020-09-05 9:20 ` Xie He
2020-09-14 5:26 ` Krzysztof Hałasa
2020-09-14 6:18 ` Xie He
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJht_ENKyfMm7wAxcVSThEG63oVe72FvMs-5VaLWemKvveY+dQ@mail.gmail.com \
--to=xie.he.0141@gmail.com \
--cc=davem@davemloft.net \
--cc=khc@pm.waw.pl \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ms@dev.tdt.de \
--cc=netdev@vger.kernel.org \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).