Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Xie He <xie.he.0141@gmail.com>
To: "Krzysztof Hałasa" <khalasa@piap.pl>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Martin Schiller <ms@dev.tdt.de>
Subject: Re: [PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len
Date: Fri, 28 Aug 2020 14:05:32 -0700	[thread overview]
Message-ID: <CAJht_EOk2_L-77KDDEJTcfqhw48X0ZMA2PKdLG4+LXHAAtNtsw@mail.gmail.com> (raw)
In-Reply-To: <m3pn7b6opa.fsf@t19.piap.pl>

On Fri, Aug 28, 2020 at 3:37 AM Krzysztof Hałasa <khalasa@piap.pl> wrote:
>
> OTOH hdlc_setup_dev() initializes hard_header_len to 16,
> but in this case I guess 4 bytes are better.
>
> Acked-by: Krzysztof Halasa <khc@pm.waw.pl>

Thank you, Krzysztof!

Actually I'm thinking about changing the default value of 16 in hdlc.c to 0.

I think a driver should always keep its hard_header_len consistent
with its header_ops functions. If a driver doesn't have header_ops,
its hard_header_len should be set to 0. This makes the driver able to
be correctly used with AF_PACKET sockets.

In net/packet/af_packet.c, in the function packet_snd, for
AF_PACKET/DGRAM sockets, it would reserve a headroom of
hard_header_len for the skb, and call dev_hard_header (which calls the
header_ops->create function) to fill in the headroom, but for
AF_PACKET/RAW sockets, it would not reserve the headroom of
hard_header_len, and will check (in function dev_validate_header)
whether the user has provided the header of length hard_header_len. So
I think hard_header_len should be kept consistent with header_ops to
make the driver able to work correctly with af_packet.c.

If the driver really needs to use additional header space outside of
the header_ops->create function, it should request that header space
in dev->needed_headroom instead of hard_header_len. This avoids the
complex header processing in af_packet.c but still gets the header
space reserved.

Currently for the 6 HDLC protocol drivers, hdlc_ppp sets
hard_header_len and the value is consistent with its header_ops,
hdlc_raw_eth sets both hard_header_len and header_ops correctly with
the ether_setup function, hdlc_x25 has been previously changed by me
to set hard_header_len to 0 because it doesn't have header_ops, and
this patch would make hdlc_cisco set its hard_header_len to the value
consistent with its header_ops. This leaves us hdlc_raw and hdlc_fr. I
see that both of these 2 drivers don't set hard_header_len when
attaching the protocol, so they will use the default value of 16. But
because both of these drivers don't have header_ops, I think their
hard_header_len should be set to 0. So I think maybe it's better to
change the default value in hdlc.c to 0 and let them take the default
value of 0.

What do you think?

Thanks!

Xie He

  reply	other threads:[~2020-08-28 21:05 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-28  7:07 Xie He
2020-08-28 10:37 ` Krzysztof Hałasa
2020-08-28 21:05   ` Xie He [this message]
2020-08-31 19:23 ` David Miller

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_EOk2_L-77KDDEJTcfqhw48X0ZMA2PKdLG4+LXHAAtNtsw@mail.gmail.com \
    --to=xie.he.0141@gmail.com \
    --cc=davem@davemloft.net \
    --cc=khalasa@piap.pl \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ms@dev.tdt.de \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len' \
    /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

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).