Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Petr Machata <petrm@mellanox.com>
Cc: Kurt Kanzenbach <kurt@linutronix.de>,
	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>,
	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: Wed, 29 Jul 2020 11:02:57 +0100
Message-ID: <20200729100257.GX1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <87pn8fgxj3.fsf@mellanox.com>

On Tue, Jul 28, 2020 at 11:06:08PM +0200, Petr Machata wrote:
> 
> Kurt Kanzenbach <kurt@linutronix.de> writes:
> 
> > 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?
> 
> Correct, mlxsw pushes the header very soon. Given that both
> ptp_classify_raw() and eth_type_trans() that are invoked later assume
> the header, it is reasonable. I have shuffled the pushes around and have
> a patch that both works and I think is correct.

Would it make more sense to do:

	u8 *data = skb_mac_header(skb);
	u8 *ptr = data;

	if (type & PTP_CLASS_VLAN)
		ptr += VLAN_HLEN;

	switch (type & PTP_CLASS_PMASK) {
	case PTP_CLASS_IPV4:
		ptr += IPV4_HLEN(ptr) + UDP_HLEN;
		break;

	case PTP_CLASS_IPV6:
		ptr += IP6_HLEN + UDP_HLEN;
		break;

	case PTP_CLASS_L2:
		break;

	default:
		return NULL;
	}

	ptr += ETH_HLEN;

	if (ptr + 34 > skb->data + skb->len)
		return NULL;

	return ptr;

in other words, compare pointers, so that whether skb_push() etc has
been used on the skb is irrelevant?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-27  9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
2020-07-27  9:05 ` [PATCH v2 1/9] ptp: Add generic ptp v2 " Kurt Kanzenbach
2020-07-27  9:05 ` [PATCH v2 2/9] ptp: Add generic ptp message type function Kurt Kanzenbach
2020-07-27  9:05 ` [PATCH v2 3/9] net: dsa: mv88e6xxx: Use generic helper function Kurt Kanzenbach
2020-07-27  9:05 ` [PATCH v2 4/9] mlxsw: spectrum_ptp: " Kurt Kanzenbach
2020-07-27 13:00   ` Petr Machata
2020-07-28 13:29     ` Kurt Kanzenbach
2020-07-28 13:41       ` Kurt Kanzenbach
2020-07-28 21:06       ` Petr Machata
2020-07-29 10:02         ` Russell King - ARM Linux admin [this message]
2020-07-29 10:29           ` Kurt Kanzenbach
2020-07-29 13:49             ` Richard Cochran
2020-07-29 14:26               ` Kurt Kanzenbach
2020-07-29 14:07             ` Petr Machata
2020-07-29 13:22           ` Richard Cochran
2020-07-29 12:23         ` Grygorii Strashko
2020-07-27  9:05 ` [PATCH v2 5/9] ethernet: ti: am65-cpts: " Kurt Kanzenbach
2020-07-27  9:05 ` [PATCH v2 6/9] ethernet: ti: cpts: " Kurt Kanzenbach
2020-07-27  9:05 ` [PATCH v2 7/9] net: phy: dp83640: " Kurt Kanzenbach
2020-07-27  9:06 ` [PATCH v2 8/9] ptp: ptp_ines: " Kurt Kanzenbach
2020-07-27  9:06 ` [PATCH v2 9/9] ptp: Remove unused macro Kurt Kanzenbach
2020-07-29  0:47 ` [PATCH v2 0/9] ptp: Add generic header parsing function David Miller
2020-07-29  9:31   ` Kurt Kanzenbach

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200729100257.GX1551@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=grygorii.strashko@ti.com \
    --cc=hkallweit1@gmail.com \
    --cc=idosch@mellanox.com \
    --cc=ivan.khoronzhuk@linaro.org \
    --cc=jiri@mellanox.com \
    --cc=kuba@kernel.org \
    --cc=kurt@linutronix.de \
    --cc=netdev@vger.kernel.org \
    --cc=petrm@mellanox.com \
    --cc=richardcochran@gmail.com \
    --cc=vivien.didelot@gmail.com \
    --cc=zou_wei@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lkml.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lkml.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git