Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Christian Eggers <ceggers@arri.de>
To:     Florian Fainelli <f.fainelli@gmail.com>
CC:     Richard Cochran <richardcochran@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Krzysztof Halasa <khalasa@piap.pl>,
        Vishal Kulkarni <vishal@chelsio.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] net: ptp: get rid of IPV4_HLEN() and OFF_IHL macros
Date:   Fri, 16 Oct 2020 09:04:38 +0200
Message-ID: <2135183.8zsl8bQRyA@n95hx1g2>
Organization: Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
In-Reply-To: <939828b0-846c-9e10-df31-afcb77b1150a@gmail.com>
References: <20201014115805.23905-1-ceggers@arri.de> <20201015033648.GA24901@hoboy> <939828b0-846c-9e10-df31-afcb77b1150a@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thursday, 15 October 2020, 18:56:41 CEST, Florian Fainelli wrote:
> Having recently helped someone with a bug that involved using
> IPV4_HLEN() instead of ip_hdr() in a driver's transmit path (so with the
> transport header correctly set), it would probably help if we could make
> IPV4_HLEN()'s semantics clearer with converting it to a static inline
> function and put asserts in there about what the transport and MAC
> header positions should be.
IPV4_HLEN() is only used for PTP. Is there any way to use "normal" 
infrastructure as in the rest of the network stack? It looks like PTP code 
typically has to look into multiple network layers (mac, network, transport) 
at places these layers may not be processed yet (at least in RX direction).

> At the very least, creating a new function, like this maybe in
> include/linux/ip.h might help:
> 
> static inline u8 __ip_hdr_len(const struct sk_buff *skb)
> {
> 	const struct iphdr *ip_hdr = (const struct iphdr *)(skb->data);
> 
> 	return ip_hdr->ihl << 2;
> }
Is there any reason using skb->data instead of skb_network_header()? Debugging 
through my DSA driver showed that ...

- for TX (called by dsa_slave_xmit), skb->data is set to the MAC header
(skb->head+0x02), whilst skb->network_header is correctly set to 0x10 
(skb->mac_header+14).
- for TX, skb->transport_header is 0xffff, so udp_hdr() cannot be used

- for RX (called by dsa_switch_rcv), skb->data is set to skb->head+0x50, which 
is identical to skb->network_header
- for RX, skb->transport_header ist set equal to skb->network_header, so 
udp_hdr() can also not be used.

Best regards
Christian