diff mbox series

[net-next] net/packet: Fix a comment about mac_header

Message ID 20200916122308.11678-1-xie.he.0141@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [net-next] net/packet: Fix a comment about mac_header | expand

Commit Message

Xie He Sept. 16, 2020, 12:23 p.m. UTC
1. Change all "dev->hard_header" to "dev->header_ops"

2. On receiving incoming frames when header_ops == NULL:

The comment only says what is wrong, but doesn't say what is right.
This patch changes the comment to make it clear what is right.

3. On transmitting and receiving outgoing frames when header_ops == NULL:

The comment explains that the LL header will be later added by the driver.

However, I think it's better to simply say that the LL header is invisible
to us. This phrasing is better from a software engineering perspective,
because this makes it clear that what happens in the driver should be
hidden from us and we should not care about what happens internally in the
driver.

4. On resuming the LL header (for RAW frames) when header_ops == NULL:

The comment says we are "unlikely" to restore the LL header.

However, we should say that we are "unable" to restore it.
It's not possible (rather than not likely) to restore it, because:

1) There is no way for us to restore because the LL header internally
processed by the driver should be invisible to us.

2) In function packet_rcv and tpacket_rcv, the code only tries to restore
the LL header when header_ops != NULL.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 net/packet/af_packet.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Willem de Bruijn Sept. 17, 2020, 8:50 a.m. UTC | #1
On Wed, Sep 16, 2020 at 8:54 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1. Change all "dev->hard_header" to "dev->header_ops"
>
> 2. On receiving incoming frames when header_ops == NULL:
>
> The comment only says what is wrong, but doesn't say what is right.
> This patch changes the comment to make it clear what is right.
>
> 3. On transmitting and receiving outgoing frames when header_ops == NULL:
>
> The comment explains that the LL header will be later added by the driver.
>
> However, I think it's better to simply say that the LL header is invisible
> to us. This phrasing is better from a software engineering perspective,
> because this makes it clear that what happens in the driver should be
> hidden from us and we should not care about what happens internally in the
> driver.
>
> 4. On resuming the LL header (for RAW frames) when header_ops == NULL:
>
> The comment says we are "unlikely" to restore the LL header.
>
> However, we should say that we are "unable" to restore it.
> It's not possible (rather than not likely) to restore it, because:
>
> 1) There is no way for us to restore because the LL header internally
> processed by the driver should be invisible to us.
>
> 2) In function packet_rcv and tpacket_rcv, the code only tries to restore
> the LL header when header_ops != NULL.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Acked-by: Willem de Bruijn <willemb@google.com>
Xie He Sept. 17, 2020, 10:17 a.m. UTC | #2
On Thu, Sep 17, 2020 at 1:51 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Acked-by: Willem de Bruijn <willemb@google.com>

Thank you, Willem!
David Miller Sept. 17, 2020, 11:25 p.m. UTC | #3
From: Xie He <xie.he.0141@gmail.com>
Date: Wed, 16 Sep 2020 05:23:08 -0700

> 1. Change all "dev->hard_header" to "dev->header_ops"
> 
> 2. On receiving incoming frames when header_ops == NULL:
> 
> The comment only says what is wrong, but doesn't say what is right.
> This patch changes the comment to make it clear what is right.
> 
> 3. On transmitting and receiving outgoing frames when header_ops == NULL:
> 
> The comment explains that the LL header will be later added by the driver.
> 
> However, I think it's better to simply say that the LL header is invisible
> to us. This phrasing is better from a software engineering perspective,
> because this makes it clear that what happens in the driver should be
> hidden from us and we should not care about what happens internally in the
> driver.
> 
> 4. On resuming the LL header (for RAW frames) when header_ops == NULL:
> 
> The comment says we are "unlikely" to restore the LL header.
> 
> However, we should say that we are "unable" to restore it.
> It's not possible (rather than not likely) to restore it, because:
> 
> 1) There is no way for us to restore because the LL header internally
> processed by the driver should be invisible to us.
> 
> 2) In function packet_rcv and tpacket_rcv, the code only tries to restore
> the LL header when header_ops != NULL.
> 
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Applied, thank you.
diff mbox series

Patch

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2d5d5fbb435c..f59fa26d4826 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -105,42 +105,43 @@ 
    - packet socket receives packets with pulled ll header,
      so that SOCK_RAW should push it back.
 
 On receive:
 -----------
 
-Incoming, dev->hard_header!=NULL
+Incoming, dev->header_ops != NULL
    mac_header -> ll header
    data       -> data
 
-Outgoing, dev->hard_header!=NULL
+Outgoing, dev->header_ops != NULL
    mac_header -> ll header
    data       -> ll header
 
-Incoming, dev->hard_header==NULL
-   mac_header -> UNKNOWN position. It is very likely, that it points to ll
-		 header.  PPP makes it, that is wrong, because introduce
-		 assymetry between rx and tx paths.
+Incoming, dev->header_ops == NULL
+   mac_header -> data
+     However drivers often make it point to the ll header.
+     This is incorrect because the ll header should be invisible to us.
    data       -> data
 
-Outgoing, dev->hard_header==NULL
-   mac_header -> data. ll header is still not built!
+Outgoing, dev->header_ops == NULL
+   mac_header -> data. ll header is invisible to us.
    data       -> data
 
 Resume
-  If dev->hard_header==NULL we are unlikely to restore sensible ll header.
+  If dev->header_ops == NULL we are unable to restore the ll header,
+    because it is invisible to us.
 
 
 On transmit:
 ------------
 
-dev->hard_header != NULL
+dev->header_ops != NULL
    mac_header -> ll header
    data       -> ll header
 
-dev->hard_header == NULL (ll header is added by device, we cannot control it)
+dev->header_ops == NULL (ll header is invisible to us)
    mac_header -> data
    data       -> data
 
    We should set nh.raw on output to correct posistion,
    packet classifier depends on it.
  */