[PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len

[PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len

[Xie He]

This driver didn't set hard_header_len. This patch sets hard_header_len
for it according to its header_ops->create function.

This driver's header_ops->create function (cisco_hard_header) creates
a header of (struct hdlc_header), so hard_header_len should be set to
sizeof(struct hdlc_header).

Cc: Martin Schiller <ms@dev.tdt.de>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_cisco.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wan/hdlc_cisco.c b/drivers/net/wan/hdlc_cisco.c
index d8cba3625c18..444130655d8e 100644
--- a/drivers/net/wan/hdlc_cisco.c
+++ b/drivers/net/wan/hdlc_cisco.c
@@ -370,6 +370,7 @@ static int cisco_ioctl(struct net_device *dev, struct ifreq *ifr)
 		memcpy(&state(hdlc)->settings, &new_settings, size);
 		spin_lock_init(&state(hdlc)->lock);
 		dev->header_ops = &cisco_header_ops;
+		dev->hard_header_len = sizeof(struct hdlc_header);
 		dev->type = ARPHRD_CISCO;
 		call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
 		netif_dormant_on(dev);
-- 
2.25.1

Re: [PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len

[Krzysztof Hałasa]

Hello Xie,

Xie He <xie.he.0141@gmail.com> writes:

> This driver didn't set hard_header_len. This patch sets hard_header_len
> for it according to its header_ops->create function.

BTW it's 4 bytes long:

struct hdlc_header {
        u8 address;
        u8 control;
        __be16 protocol;
}__packed;

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>

> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---

> --- a/drivers/net/wan/hdlc_cisco.c
> +++ b/drivers/net/wan/hdlc_cisco.c
> @@ -370,6 +370,7 @@ static int cisco_ioctl(struct net_device *dev, struct ifreq *ifr)
>  		memcpy(&state(hdlc)->settings, &new_settings, size);
>  		spin_lock_init(&state(hdlc)->lock);
>  		dev->header_ops = &cisco_header_ops;
> +		dev->hard_header_len = sizeof(struct hdlc_header);
>  		dev->type = ARPHRD_CISCO;
>  		call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
>  		netif_dormant_on(dev);

-- 
Krzysztof Halasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len

[Xie He]

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

Re: [PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len

[David Miller]

From: Xie He <xie.he.0141@gmail.com>
Date: Fri, 28 Aug 2020 00:07:52 -0700

> This driver didn't set hard_header_len. This patch sets hard_header_len
> for it according to its header_ops->create function.
> 
> This driver's header_ops->create function (cisco_hard_header) creates
> a header of (struct hdlc_header), so hard_header_len should be set to
> sizeof(struct hdlc_header).
> 
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Applied, thanks.