Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function

From: Kurt Kanzenbach <kurt@linutronix.de>
To: Petr Machata <petrm@mellanox.com>
Cc: Richard Cochran <richardcochran@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@mellanox.com>,
	Ido Schimmel <idosch@mellanox.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>,
	Samuel Zou <zou_wei@huawei.com>,
	netdev@vger.kernel.org
Subject: Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
Date: Tue, 28 Jul 2020 15:29:44 +0200	[thread overview]
Message-ID: <875za7sr7b.fsf@kurt> (raw)
In-Reply-To: <87a6zli04l.fsf@mellanox.com>

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

On Mon Jul 27 2020, Petr Machata wrote:
> So this looks good, and works, but I'm wondering about one thing.

Thanks for testing.

>
> Your code (and evidently most drivers as well) use a different check
> than mlxsw, namely skb->len + ETH_HLEN < X. When I print_hex_dump()
> skb_mac_header(skb), skb->len in mlxsw with some test packet, I get e.g.
> this:
>
>     00000000259a4db7: 01 00 5e 00 01 81 00 02 c9 a4 e4 e1 08 00 45 00  ..^...........E.
>     000000005f29f0eb: 00 48 0d c9 40 00 01 11 c8 59 c0 00 02 01 e0 00  .H..@....Y......
>     00000000f3663e9e: 01 81 01 3f 01 3f 00 34 9f d3 00 02 00 2c 00 00  ...?.?.4.....,..
>                             ^sp^^ ^dp^^ ^len^ ^cks^       ^len^
>     00000000b3914606: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02  ................
>     000000002e7828ea: c9 ff fe a4 e4 e1 00 01 09 fa 00 00 00 00 00 00  ................
>     000000000b98156e: 00 00 00 00 00 00                                ......
>
> Both UDP and PTP length fields indicate that the payload ends exactly at
> the end of the dump. So apparently skb->len contains all the payload
> bytes, including the Ethernet header.
>
> Is that the case for other drivers as well? Maybe mlxsw is just missing
> some SKB magic in the driver.

So I run some tests (on other hardware/drivers) and it seems like that
the skb->len usually doesn't include the ETH_HLEN. Therefore, it is
added to the check.

Looking at the driver code:

|static void mlxsw_sp_rx_sample_listener(struct sk_buff *skb, u8 local_port,
|					void *trap_ctx)
|{
|	[...]
|	/* The sample handler expects skb->data to point to the start of the
|	 * Ethernet header.
|	 */
|	skb_push(skb, ETH_HLEN);
|	mlxsw_sp_sample_receive(mlxsw_sp, skb, local_port);
|}

Maybe that's the issue here?

I was also wondering about something else in that driver driver: The
parsing code allows for ptp v1, but the message type was always fetched
from offset 0 in the header. Is that indented?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]