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

[Kurt Kanzenbach <kurt@linutronix.de>]

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

On Wed Jul 29 2020, Russell King - ARM Linux admin wrote:
> 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?

Yes. Avoiding relying on whether skb_{push,pull} has been used is
overall the simplest solution and it works for all drivers regardless if
DSA or something else is used. Looking at your code, it looks correct
to me.

I'll test it and send v3. But before, I've got another question that
somebody might have an answer to:

The ptp v1 code always does locate the message type at

 msgtype = data + offset + OFF_PTP_CONTROL

OFF_PTP_CONTROL is 32. However, looking at the ptp v1 header, the
message type is located at offset 20. What am I missing here?

Thanks,
Kurt

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