| Message ID | 20200828070752.54444-1-xie.he.0141@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] drivers/net/wan/hdlc_cisco: Add hard_header_len | expand |
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);
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
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.
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);
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(+)