Re: [PATCH] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len

[Cong Wang]

Hello,

On Mon, Jul 27, 2020 at 12:41 PM Xie He <xie.he.0141@xxxxxxxxx> wrote:
>
> Hi Cong Wang,
>
> I'm wishing to change a driver from using "hard_header_len" to using
> "needed_headroom" to declare its needed headroom. I submitted a patch
> and it is decided it needs to be reviewed. I see you participated in
> "hard_header_len vs needed_headroom" discussions in the past. Can you
> help me review this patch? Thanks!
>
> The patch is at:
> http://patchwork.ozlabs.org/project/netdev/patch/20200726110524.151957-1-xie.he.0141@xxxxxxxxx/
>
> In my understanding, hard_header_len should be the length of the header
> created by dev_hard_header. Any additional headroom needed should be
> declared in needed_headroom instead of hard_header_len. I came to this
> conclusion by examining the logic of net/packet/af_packet.c:packet_snd.

I am not familiar with this WAN driver, but I suggest you to look at
the following commit, which provides a lot of useful information:

commit 9454f7a895b822dd8fb4588fc55fda7c96728869
Author: Brian Norris <briannorris@xxxxxxxxxxxx>
Date:   Wed Feb 26 16:05:11 2020 -0800

    mwifiex: set needed_headroom, not hard_header_len

    hard_header_len provides limitations for things like AF_PACKET, such
    that we don't allow transmitting packets smaller than this.

    needed_headroom provides a suggested minimum headroom for SKBs, so that
    we can trivally add our headers to the front.

    The latter is the correct field to use in this case, while the former
    mostly just prevents sending small AF_PACKET frames.

    In any case, mwifiex already does its own bounce buffering [1] if we
    don't have enough headroom, so hints (not hard limits) are all that are
    needed.

    This is the essentially the same bug (and fix) that brcmfmac had, fixed
    in commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
    additional header space").

    [1] mwifiex_hard_start_xmit():
            if (skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN) {
            [...]
                    /* Insufficient skb headroom - allocate a new skb */

Hope this helps.

Thanks.