| Message ID | 20200726110524.151957-1-xie.he.0141@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len | expand |
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@gmail.com/ 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. What do you think? Thanks!
Hello, On Mon, Jul 27, 2020 at 12:41 PM Xie He <xie.he.0141@gmail.com> 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@gmail.com/ > > 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@chromium.org> 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.
Hi Brian Norris, 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 submitted a similar patch in the past. So I think you might be able to help review my patch. Can you help review my patch? Thanks! The patch is at: http://patchwork.ozlabs.org/project/netdev/patch/20200726110524.151957-1-xie.he.0141@gmail.com/ As you have noted in the commit message of your patch, hard_header_len has special meaning in AF_PACKET so we need to use needed_headroom instead when we just need to reserve some header space. I believe the value of hard_header_len should be the same as the length of the header created by dev_hard_header.
(Reviewing as requested; I'm not familiar with this driver either, or really any WAN driver. It also seems that hard_header_len vs. needed_headroom aren't very well documented, and even I can't guarantee I understand them completely. So take my thoughts with a grain of salt.) Hi, On Sun, Jul 26, 2020 at 04:05:24AM -0700, Xie He wrote: > In net/packet/af_packet.c, the function packet_snd first reserves a > headroom of length (dev->hard_header_len + dev->needed_headroom). > Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header, > which calls dev->header_ops->create, to create the link layer header. > If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of > length (dev->hard_header_len), and assumes the user to provide the > appropriate link layer header. > > So according to the logic of af_packet.c, dev->hard_header_len should > be the length of the header that would be created by > dev->header_ops->create. I believe I'm with you up to here, but: > However, this driver doesn't provide dev->header_ops, so logically > dev->hard_header_len should be 0. I'm not clear on this part. What's to say you shouldn't be implementing header_ops instead? Note that with WiFi drivers, they're exposing themselves as ARPHRD_ETHER, and only the Ethernet headers are part of the upper "protocol" headers. So my patch deferred to the eth headers. What is the intention with this X25 protocol? I guess the headers added in lapbeth_data_transmit() are supposed to be "invisible", as with this note in af_packet.c? - if device has no dev->hard_header routine, it adds and removes ll header inside itself. In this case ll header is invisible outside of device, but higher levels still should reserve dev->hard_header_len. If that's the case, then yes, I believe this patch should be correct. Brian > So we should use dev->needed_headroom instead of dev->hard_header_len > to request necessary headroom to be allocated. > > Signed-off-by: Xie He <xie.he.0141@gmail.com>
Thank you for your detailed review, Brian! I guess we have the same understanding on the "hard_header_len vs needed_headroom" part. I agree it is not well documented and is also confusing to driver developers. I didn't understand it either until I looked at af_packet.c. On Tue, Jul 28, 2020 at 12:52 PM -0700 Brian Norris <briannorris@chromium.org> wrote: > > What's to say you shouldn't be implementing header_ops instead? Note > that with WiFi drivers, they're exposing themselves as ARPHRD_ETHER, and > only the Ethernet headers are part of the upper "protocol" headers. So > my patch deferred to the eth headers. > > What is the intention with this X25 protocol? I guess the headers added > in lapbeth_data_transmit() are supposed to be "invisible", as with this > note in af_packet.c? > > - if device has no dev->hard_header routine, it adds and removes ll header > inside itself. In this case ll header is invisible outside of device, > but higher levels still should reserve dev->hard_header_len. > > If that's the case, then yes, I believe this patch should be correct. This driver is not intended to be used with IPv4 or IPv6 protocols, but is intended to be used with a special "X.25" protocol. That's the reason the device type is ARPHRD_X25. I used "grep" in the X.25 network layer code (net/x25) and I found there's nowhere "dev_hard_header" is called. I also used "grep" in all the X.25 drivers in the kernel (lapbether.c, x25_asy.c, hdlc_x25.c under drivers/net/wan) and I found no driver implemented "header_ops". So I think the X.25 networking code doesn't expect any header visible outside of the device driver, and X.25 drivers should make their headers invisible outside of them. So I think hard_header_len should be 0 for all X.25 drivers, so that they can be used correctly with af_packet.c. I don't know if this sounds plausible to you. If it does, could you please let me have your name in a "Reviewed_by" tag. It would be of great help to have your support. Thanks!
On Tue, Jul 28, 2020 at 6:42 PM Xie He <xie.he.0141@gmail.com> wrote: > On Tue, Jul 28, 2020 at 12:52 PM -0700 > Brian Norris <briannorris@chromium.org> wrote: > > What is the intention with this X25 protocol? I guess the headers added > > in lapbeth_data_transmit() are supposed to be "invisible", as with this > > note in af_packet.c? ... > This driver is not intended to be used with IPv4 or IPv6 protocols, > but is intended to be used with a special "X.25" protocol. That's the > reason the device type is ARPHRD_X25. I used "grep" in the X.25 > network layer code (net/x25) and I found there's nowhere > "dev_hard_header" is called. I also used "grep" in all the X.25 That well could just be a bug in net/x25/. But from context, it does appear that X.25 does not intend to expose its headers to higher layers. > drivers in the kernel (lapbether.c, x25_asy.c, hdlc_x25.c under > drivers/net/wan) and I found no driver implemented "header_ops". So I > think the X.25 networking code doesn't expect any header visible > outside of the device driver, and X.25 drivers should make their > headers invisible outside of them. > > So I think hard_header_len should be 0 for all X.25 drivers, so that > they can be used correctly with af_packet.c. > > I don't know if this sounds plausible to you. If it does, could you > please let me have your name in a "Reviewed_by" tag. It would be of > great help to have your support. Thanks! Sure, I can do that: Reviewed-by: Brian Norris <briannorris@chromium.org> I guess x25 is basically an abandoned project, if you're coming to me for this?
On Thu, Jul 30, 2020 at 5:24 PM Brian Norris <briannorris@chromium.org> wrote: > > Sure, I can do that: > > Reviewed-by: Brian Norris <briannorris@chromium.org> Thank you so much for your review, Brian! > I guess x25 is basically an abandoned project, if you're coming to me for this? Yes, it does seem to me that X.25 is unmaintained. I'm submitting patches for it because I'm personally interested in X.25 and I want to fix things that I find to be having issues. But it's very hard for me to do so because it's hard to find reviewers for X.25 code. So I really appreciate that you review this patch. Thanks! I don't know if it is the right thing to continue submitting patches for X.25, or we should just keep the code as is. Maybe the kernel community can have a discussion some time to decide what to do with it.
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c index b2868433718f..34cf6db89912 100644 --- a/drivers/net/wan/lapbether.c +++ b/drivers/net/wan/lapbether.c @@ -305,6 +305,7 @@ static void lapbeth_setup(struct net_device *dev) dev->netdev_ops = &lapbeth_netdev_ops; dev->needs_free_netdev = true; dev->type = ARPHRD_X25; + dev->hard_header_len = 0; dev->mtu = 1000; dev->addr_len = 0; } @@ -331,7 +332,8 @@ static int lapbeth_new_device(struct net_device *dev) * then this driver prepends a length field of 2 bytes, * then the underlying Ethernet device prepends its own header. */ - ndev->hard_header_len = -1 + 3 + 2 + dev->hard_header_len; + ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len + + dev->needed_headroom; lapbeth = netdev_priv(ndev); lapbeth->axdev = ndev;
In net/packet/af_packet.c, the function packet_snd first reserves a headroom of length (dev->hard_header_len + dev->needed_headroom). Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header, which calls dev->header_ops->create, to create the link layer header. If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of length (dev->hard_header_len), and assumes the user to provide the appropriate link layer header. So according to the logic of af_packet.c, dev->hard_header_len should be the length of the header that would be created by dev->header_ops->create. However, this driver doesn't provide dev->header_ops, so logically dev->hard_header_len should be 0. So we should use dev->needed_headroom instead of dev->hard_header_len to request necessary headroom to be allocated. Signed-off-by: Xie He <xie.he.0141@gmail.com> --- drivers/net/wan/lapbether.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)