On Fri, Aug 28, 2020 at 3:37 AM Krzysztof Hałasa <khal...@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 <k...@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
