[PATCH v3 0/9] ptp: Add generic helper functions

[PATCH v3 0/9] ptp: Add generic helper functions

[Kurt Kanzenbach]

Hi!

in order to reduce code duplication (and cut'n'paste errors) in the ptp code of
DSA, Ethernet and Phy drivers, create helper functions and move them to
ptp_classify. This way all drivers can share the same implementation.

This is version three and contains bugfixes. Implemented as discussed [1] [2]
[3] [4].

Previous versions can be found here:

 * https://lkml.kernel.org/netdev/20200723074946.14253-1-kurt@linutronix.de/
 * https://lkml.kernel.org/netdev/20200727090601.6500-1-kurt@linutronix.de/

Thanks,
Kurt

Changes since v2:

 * Make ptp_parse_header() work in all scenarios (Russell King)
 * Fix msgtype offset for ptp v1 packets

Changes since v1:

 * Fix Kconfig (Richard Cochran)
 * Include more drivers (Richard Cochran)

[1] - https://lkml.kernel.org/netdev/20200713140112.GB27934@hoboy/
[2] - https://lkml.kernel.org/netdev/20200720142146.GB16001@hoboy/
[3] - https://lkml.kernel.org/netdev/20200723074946.14253-1-kurt@linutronix.de/
[4] - https://lkml.kernel.org/netdev/20200729100257.GX1551@shell.armlinux.org.uk/

Kurt Kanzenbach (9):
  ptp: Add generic ptp v2 header parsing function
  ptp: Add generic ptp message type function
  net: dsa: mv88e6xxx: Use generic helper function
  mlxsw: spectrum_ptp: Use generic helper function
  ethernet: ti: am65-cpts: Use generic helper function
  ethernet: ti: cpts: Use generic helper function
  net: phy: dp83640: Use generic helper function
  ptp: ptp_ines: Use generic helper function
  ptp: Remove unused macro

 drivers/net/dsa/mv88e6xxx/hwtstamp.c          | 59 +++----------
 .../ethernet/mellanox/mlxsw/spectrum_ptp.c    | 32 ++-----
 drivers/net/ethernet/ti/am65-cpts.c           | 37 ++------
 drivers/net/ethernet/ti/cpts.c                | 37 ++------
 drivers/net/phy/dp83640.c                     | 69 ++++-----------
 drivers/ptp/ptp_ines.c                        | 88 ++++++-------------
 include/linux/ptp_classify.h                  | 63 ++++++++++++-
 net/core/ptp_classifier.c                     | 31 +++++++
 8 files changed, 172 insertions(+), 244 deletions(-)

-- 
2.20.1

[PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Kurt Kanzenbach]

Reason: A lot of the ptp drivers - which implement hardware time stamping - need
specific fields such as the sequence id from the ptp v2 header. Currently all
drivers implement that themselves.

Introduce a generic function to retrieve a pointer to the start of the ptp v2
header.

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 include/linux/ptp_classify.h | 38 ++++++++++++++++++++++++++++++++++++
 net/core/ptp_classifier.c    | 31 +++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index dd00fa41f7e7..26fd38a4bd67 100644
--- a/include/linux/ptp_classify.h
+++ b/include/linux/ptp_classify.h
@@ -44,6 +44,30 @@
 #define OFF_IHL		14
 #define IPV4_HLEN(data) (((struct iphdr *)(data + OFF_IHL))->ihl << 2)
 
+struct clock_identity {
+	u8 id[8];
+} __packed;
+
+struct port_identity {
+	struct clock_identity	clock_identity;
+	__be16			port_number;
+} __packed;
+
+struct ptp_header {
+	u8			tsmt;  /* transportSpecific | messageType */
+	u8			ver;   /* reserved          | versionPTP  */
+	__be16			message_length;
+	u8			domain_number;
+	u8			reserved1;
+	u8			flag_field[2];
+	__be64			correction;
+	__be32			reserved2;
+	struct port_identity	source_port_identity;
+	__be16			sequence_id;
+	u8			control;
+	u8			log_message_interval;
+} __packed;
+
 #if defined(CONFIG_NET_PTP_CLASSIFY)
 /**
  * ptp_classify_raw - classify a PTP packet
@@ -57,6 +81,15 @@
  */
 unsigned int ptp_classify_raw(const struct sk_buff *skb);
 
+/**
+ * ptp_parse_header - Get pointer to the PTP v2 header
+ * @skb: packet buffer
+ * @type: type of the packet (see ptp_classify_raw())
+ *
+ * Return: Pointer to the ptp v2 header or NULL if not found
+ */
+struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type);
+
 void __init ptp_classifier_init(void);
 #else
 static inline void ptp_classifier_init(void)
@@ -66,5 +99,10 @@ static inline unsigned int ptp_classify_raw(struct sk_buff *skb)
 {
 	return PTP_CLASS_NONE;
 }
+static inline struct ptp_header *ptp_parse_header(struct sk_buff *skb,
+						  unsigned int type)
+{
+	return NULL;
+}
 #endif
 #endif /* _PTP_CLASSIFY_H_ */
diff --git a/net/core/ptp_classifier.c b/net/core/ptp_classifier.c
index d964a5147f22..41f373477812 100644
--- a/net/core/ptp_classifier.c
+++ b/net/core/ptp_classifier.c
@@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL_GPL(ptp_classify_raw);
 
+struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
+{
+	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;
+
+	/* Ensure that the entire header is present in this packet. */
+	if (ptr + sizeof(struct ptp_header) > skb->data + skb->len)
+		return NULL;
+
+	return (struct ptp_header *)ptr;
+}
+EXPORT_SYMBOL_GPL(ptp_parse_header);
+
 void __init ptp_classifier_init(void)
 {
 	static struct sock_filter ptp_filter[] __initdata = {
-- 
2.20.1

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Petr Machata]

Kurt Kanzenbach <kurt@linutronix.de> writes:

> @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL_GPL(ptp_classify_raw);
>  
> +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
> +{
> +	u8 *data = skb_mac_header(skb);
> +	u8 *ptr = data;

One of the "data" and "ptr" variables is superfluous.

> +
> +	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;
> +
> +	/* Ensure that the entire header is present in this packet. */
> +	if (ptr + sizeof(struct ptp_header) > skb->data + skb->len)
> +		return NULL;

Looks correct.

> +	return (struct ptp_header *)ptr;
> +}
> +EXPORT_SYMBOL_GPL(ptp_parse_header);
> +
>  void __init ptp_classifier_init(void)
>  {
>  	static struct sock_filter ptp_filter[] __initdata = {

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Kurt Kanzenbach]

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

On Thu Jul 30 2020, Petr Machata wrote:
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
>> @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>>  }
>>  EXPORT_SYMBOL_GPL(ptp_classify_raw);
>>  
>> +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
>> +{
>> +	u8 *data = skb_mac_header(skb);
>> +	u8 *ptr = data;
>
> One of the "data" and "ptr" variables is superfluous.

Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);

However, I'll wait a bit before sending the next version. So, that the
other maintainers have time to test their drivers.

Thanks,
Kurt

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Grygorii Strashko]

On 31/07/2020 13:06, Kurt Kanzenbach wrote:
> On Thu Jul 30 2020, Petr Machata wrote:
>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>
>>> @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>>>   }
>>>   EXPORT_SYMBOL_GPL(ptp_classify_raw);
>>>   
>>> +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
>>> +{
>>> +	u8 *data = skb_mac_header(skb);
>>> +	u8 *ptr = data;
>>
>> One of the "data" and "ptr" variables is superfluous.
> 
> Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);

Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
am571x platform PATCH 6.

The CPSW RX timestamp requested after full packet put in SKB, but
before calling eth_type_trans().

So, skb->data pints on Eth header, but skb_mac_header() return garbage.

Below diff fixes it for me.

--- a/net/core/ptp_classifier.c
+++ b/net/core/ptp_classifier.c
@@ -109,7 +109,7 @@ EXPORT_SYMBOL_GPL(ptp_classify_raw);
  
  struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
  {
-       u8 *data = skb_mac_header(skb);
+       u8 *data = skb->data;
         u8 *ptr = data;
  
         if (type & PTP_CLASS_VLAN)

> 
> However, I'll wait a bit before sending the next version. So, that the
> other maintainers have time to test their drivers.

-- 
Best regards,
grygorii

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Russell King - ARM Linux admin]

On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
> 
> 
> On 31/07/2020 13:06, Kurt Kanzenbach wrote:
> > On Thu Jul 30 2020, Petr Machata wrote:
> > > Kurt Kanzenbach <kurt@linutronix.de> writes:
> > > 
> > > > @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(ptp_classify_raw);
> > > > +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
> > > > +{
> > > > +	u8 *data = skb_mac_header(skb);
> > > > +	u8 *ptr = data;
> > > 
> > > One of the "data" and "ptr" variables is superfluous.
> > 
> > Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
> 
> Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
> am571x platform PATCH 6.
> 
> The CPSW RX timestamp requested after full packet put in SKB, but
> before calling eth_type_trans().
> 
> So, skb->data pints on Eth header, but skb_mac_header() return garbage.
> 
> Below diff fixes it for me.

However, that's likely to break everyone else.

For example, anyone calling this from the mii_timestamper rxtstamp()
method, the skb will have been classified with the MAC header pushed
and restored, so skb->data points at the network header.

Your change means that ptp_parse_header() expects the MAC header to
also be pushed.

Is it possible to adjust CPTS?

Looking at:
drivers/net/ethernet/ti/cpsw.c... yes.
drivers/net/ethernet/ti/cpsw_new.c... yes.
drivers/net/ethernet/ti/netcp_core.c... unclear.

If not, maybe cpts should remain unconverted - I don't see any reason
to provide a generic function for one user.

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Grygorii Strashko]

On 05/08/2020 00:07, Russell King - ARM Linux admin wrote:
> On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
>>
>>
>> On 31/07/2020 13:06, Kurt Kanzenbach wrote:
>>> On Thu Jul 30 2020, Petr Machata wrote:
>>>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>>>
>>>>> @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(ptp_classify_raw);
>>>>> +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
>>>>> +{
>>>>> +	u8 *data = skb_mac_header(skb);
>>>>> +	u8 *ptr = data;
>>>>
>>>> One of the "data" and "ptr" variables is superfluous.
>>>
>>> Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
>>
>> Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
>> am571x platform PATCH 6.
>>
>> The CPSW RX timestamp requested after full packet put in SKB, but
>> before calling eth_type_trans().
>>
>> So, skb->data pints on Eth header, but skb_mac_header() return garbage.
>>
>> Below diff fixes it for me.
> 
> However, that's likely to break everyone else.
> 
> For example, anyone calling this from the mii_timestamper rxtstamp()
> method, the skb will have been classified with the MAC header pushed
> and restored, so skb->data points at the network header.
> 
> Your change means that ptp_parse_header() expects the MAC header to
> also be pushed.
> 
> Is it possible to adjust CPTS?
> 
> Looking at:
> drivers/net/ethernet/ti/cpsw.c... yes.
> drivers/net/ethernet/ti/cpsw_new.c... yes.
> drivers/net/ethernet/ti/netcp_core.c... unclear.
> 
> If not, maybe cpts should remain unconverted - I don't see any reason
> to provide a generic function for one user.
> 

Could it be an option to pass "u8 *ptr" instead of "const struct sk_buff *skb" as
input parameter to ptp_parse_header()?

-- 
Best regards,
grygorii

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Russell King - ARM Linux admin]

On Wed, Aug 05, 2020 at 12:34:47AM +0300, Grygorii Strashko wrote:
> On 05/08/2020 00:07, Russell King - ARM Linux admin wrote:
> > On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
> > > 
> > > 
> > > On 31/07/2020 13:06, Kurt Kanzenbach wrote:
> > > > On Thu Jul 30 2020, Petr Machata wrote:
> > > > > Kurt Kanzenbach <kurt@linutronix.de> writes:
> > > > > 
> > > > > > @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
> > > > > >    }
> > > > > >    EXPORT_SYMBOL_GPL(ptp_classify_raw);
> > > > > > +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
> > > > > > +{
> > > > > > +	u8 *data = skb_mac_header(skb);
> > > > > > +	u8 *ptr = data;
> > > > > 
> > > > > One of the "data" and "ptr" variables is superfluous.
> > > > 
> > > > Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
> > > 
> > > Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
> > > am571x platform PATCH 6.
> > > 
> > > The CPSW RX timestamp requested after full packet put in SKB, but
> > > before calling eth_type_trans().
> > > 
> > > So, skb->data pints on Eth header, but skb_mac_header() return garbage.
> > > 
> > > Below diff fixes it for me.
> > 
> > However, that's likely to break everyone else.
> > 
> > For example, anyone calling this from the mii_timestamper rxtstamp()
> > method, the skb will have been classified with the MAC header pushed
> > and restored, so skb->data points at the network header.
> > 
> > Your change means that ptp_parse_header() expects the MAC header to
> > also be pushed.
> > 
> > Is it possible to adjust CPTS?
> > 
> > Looking at:
> > drivers/net/ethernet/ti/cpsw.c... yes.
> > drivers/net/ethernet/ti/cpsw_new.c... yes.
> > drivers/net/ethernet/ti/netcp_core.c... unclear.
> > 
> > If not, maybe cpts should remain unconverted - I don't see any reason
> > to provide a generic function for one user.
> > 
> 
> Could it be an option to pass "u8 *ptr" instead of "const struct sk_buff *skb" as
> input parameter to ptp_parse_header()?

It needs to read from the buffer, and in order to do that, it needs to
validate that the buffer contains sufficient data.  So, at minimum it
needs to be a pointer and size of valid data.

I was thinking about suggesting that as a core function, with a wrapper
for the existing interface.

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Grygorii Strashko]

On 05/08/2020 00:44, Russell King - ARM Linux admin wrote:
> On Wed, Aug 05, 2020 at 12:34:47AM +0300, Grygorii Strashko wrote:
>> On 05/08/2020 00:07, Russell King - ARM Linux admin wrote:
>>> On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
>>>>
>>>>
>>>> On 31/07/2020 13:06, Kurt Kanzenbach wrote:
>>>>> On Thu Jul 30 2020, Petr Machata wrote:
>>>>>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>>>>>
>>>>>>> @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>>>>>>>     }
>>>>>>>     EXPORT_SYMBOL_GPL(ptp_classify_raw);
>>>>>>> +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
>>>>>>> +{
>>>>>>> +	u8 *data = skb_mac_header(skb);
>>>>>>> +	u8 *ptr = data;
>>>>>>
>>>>>> One of the "data" and "ptr" variables is superfluous.
>>>>>
>>>>> Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
>>>>
>>>> Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
>>>> am571x platform PATCH 6.
>>>>
>>>> The CPSW RX timestamp requested after full packet put in SKB, but
>>>> before calling eth_type_trans().
>>>>
>>>> So, skb->data pints on Eth header, but skb_mac_header() return garbage.
>>>>
>>>> Below diff fixes it for me.
>>>
>>> However, that's likely to break everyone else.
>>>
>>> For example, anyone calling this from the mii_timestamper rxtstamp()
>>> method, the skb will have been classified with the MAC header pushed
>>> and restored, so skb->data points at the network header.
>>>
>>> Your change means that ptp_parse_header() expects the MAC header to
>>> also be pushed.
>>>
>>> Is it possible to adjust CPTS?
>>>
>>> Looking at:
>>> drivers/net/ethernet/ti/cpsw.c... yes.
>>> drivers/net/ethernet/ti/cpsw_new.c... yes.
>>> drivers/net/ethernet/ti/netcp_core.c... unclear.
>>>
>>> If not, maybe cpts should remain unconverted - I don't see any reason
>>> to provide a generic function for one user.
>>>
>>
>> Could it be an option to pass "u8 *ptr" instead of "const struct sk_buff *skb" as
>> input parameter to ptp_parse_header()?
> 
> It needs to read from the buffer, and in order to do that, it needs to
> validate that the buffer contains sufficient data.  So, at minimum it
> needs to be a pointer and size of valid data.
> 
> I was thinking about suggesting that as a core function, with a wrapper
> for the existing interface.
> 

Then length can be added.
Otherwise not only CPTS can't benefit from this new API, but also
drivers like oki-semi/pch_gbe/pch_gbe_main.c -> pch_ptp_match()

or have to two have two APIs (name?).

ptp_parse_header1(struct sk_buff *skb, unsigned int type)
{
	u8 *data = skb_mac_header(skb);

ptp_parse_header2(struct sk_buff *skb, unsigned int type)
{
	u8 *data = skb->data;

everything else is the same.

-- 
Best regards,
grygorii

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Russell King - ARM Linux admin]

On Wed, Aug 05, 2020 at 01:04:31AM +0300, Grygorii Strashko wrote:
> On 05/08/2020 00:44, Russell King - ARM Linux admin wrote:
> > On Wed, Aug 05, 2020 at 12:34:47AM +0300, Grygorii Strashko wrote:
> > > On 05/08/2020 00:07, Russell King - ARM Linux admin wrote:
> > > > On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
> > > > > 
> > > > > 
> > > > > On 31/07/2020 13:06, Kurt Kanzenbach wrote:
> > > > > > On Thu Jul 30 2020, Petr Machata wrote:
> > > > > > > Kurt Kanzenbach <kurt@linutronix.de> writes:
> > > > > > > 
> > > > > > > > @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
> > > > > > > >     }
> > > > > > > >     EXPORT_SYMBOL_GPL(ptp_classify_raw);
> > > > > > > > +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
> > > > > > > > +{
> > > > > > > > +	u8 *data = skb_mac_header(skb);
> > > > > > > > +	u8 *ptr = data;
> > > > > > > 
> > > > > > > One of the "data" and "ptr" variables is superfluous.
> > > > > > 
> > > > > > Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
> > > > > 
> > > > > Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
> > > > > am571x platform PATCH 6.
> > > > > 
> > > > > The CPSW RX timestamp requested after full packet put in SKB, but
> > > > > before calling eth_type_trans().
> > > > > 
> > > > > So, skb->data pints on Eth header, but skb_mac_header() return garbage.
> > > > > 
> > > > > Below diff fixes it for me.
> > > > 
> > > > However, that's likely to break everyone else.
> > > > 
> > > > For example, anyone calling this from the mii_timestamper rxtstamp()
> > > > method, the skb will have been classified with the MAC header pushed
> > > > and restored, so skb->data points at the network header.
> > > > 
> > > > Your change means that ptp_parse_header() expects the MAC header to
> > > > also be pushed.
> > > > 
> > > > Is it possible to adjust CPTS?
> > > > 
> > > > Looking at:
> > > > drivers/net/ethernet/ti/cpsw.c... yes.
> > > > drivers/net/ethernet/ti/cpsw_new.c... yes.
> > > > drivers/net/ethernet/ti/netcp_core.c... unclear.
> > > > 
> > > > If not, maybe cpts should remain unconverted - I don't see any reason
> > > > to provide a generic function for one user.
> > > > 
> > > 
> > > Could it be an option to pass "u8 *ptr" instead of "const struct sk_buff *skb" as
> > > input parameter to ptp_parse_header()?
> > 
> > It needs to read from the buffer, and in order to do that, it needs to
> > validate that the buffer contains sufficient data.  So, at minimum it
> > needs to be a pointer and size of valid data.
> > 
> > I was thinking about suggesting that as a core function, with a wrapper
> > for the existing interface.
> > 
> 
> Then length can be added.

Actually, it needs more than that, because skb->data..skb->len already
may contain the eth header or may not.

> Otherwise not only CPTS can't benefit from this new API, but also
> drivers like oki-semi/pch_gbe/pch_gbe_main.c -> pch_ptp_match()

Again, this looks like it can be solved easily by swapping the position
of these two calls:

                        pch_rx_timestamp(adapter, skb);

                        skb->protocol = eth_type_trans(skb, netdev);

> or have to two have two APIs (name?).
> 
> ptp_parse_header1(struct sk_buff *skb, unsigned int type)
> {
> 	u8 *data = skb_mac_header(skb);
> 
> ptp_parse_header2(struct sk_buff *skb, unsigned int type)
> {
> 	u8 *data = skb->data;
> 
> everything else is the same.

Actually, I really don't think we want 99% of users doing:

	hdr = ptp_parse_header(skb_mac_header(skb), skb->data, skb->len, type)

or

	hdr = ptp_parse_header(skb_mac_header(skb), skb->data + skb->len, type);

because that is what it will take, and this is starting to look
really very horrid.

So, I repeat my question again: can netcp_core.c be adjusted to
ensure that the skb mac header field is correctly set by calling
eth_type_trans() prior to calling the rx hooks?  The other two
cpts cases look easy to change, and the oki-semi also looks the
same.

Otherwise, the easiest thing may be to just revert the change to
cpts.

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Kurt Kanzenbach]

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

On Wed Aug 05 2020, Russell King - ARM Linux admin wrote:
> On Wed, Aug 05, 2020 at 01:04:31AM +0300, Grygorii Strashko wrote:
>> On 05/08/2020 00:44, Russell King - ARM Linux admin wrote:
>> > On Wed, Aug 05, 2020 at 12:34:47AM +0300, Grygorii Strashko wrote:
>> > > On 05/08/2020 00:07, Russell King - ARM Linux admin wrote:
>> > > > On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
>> > > > > 
>> > > > > 
>> > > > > On 31/07/2020 13:06, Kurt Kanzenbach wrote:
>> > > > > > On Thu Jul 30 2020, Petr Machata wrote:
>> > > > > > > Kurt Kanzenbach <kurt@linutronix.de> writes:
>> > > > > > > 
>> > > > > > > > @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>> > > > > > > >     }
>> > > > > > > >     EXPORT_SYMBOL_GPL(ptp_classify_raw);
>> > > > > > > > +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
>> > > > > > > > +{
>> > > > > > > > +	u8 *data = skb_mac_header(skb);
>> > > > > > > > +	u8 *ptr = data;
>> > > > > > > 
>> > > > > > > One of the "data" and "ptr" variables is superfluous.
>> > > > > > 
>> > > > > > Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
>> > > > > 
>> > > > > Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
>> > > > > am571x platform PATCH 6.
>> > > > > 
>> > > > > The CPSW RX timestamp requested after full packet put in SKB, but
>> > > > > before calling eth_type_trans().
>> > > > > 
>> > > > > So, skb->data pints on Eth header, but skb_mac_header() return garbage.
>> > > > > 
>> > > > > Below diff fixes it for me.
>> > > > 
>> > > > However, that's likely to break everyone else.
>> > > > 
>> > > > For example, anyone calling this from the mii_timestamper rxtstamp()
>> > > > method, the skb will have been classified with the MAC header pushed
>> > > > and restored, so skb->data points at the network header.
>> > > > 
>> > > > Your change means that ptp_parse_header() expects the MAC header to
>> > > > also be pushed.
>> > > > 
>> > > > Is it possible to adjust CPTS?
>> > > > 
>> > > > Looking at:
>> > > > drivers/net/ethernet/ti/cpsw.c... yes.
>> > > > drivers/net/ethernet/ti/cpsw_new.c... yes.
>> > > > drivers/net/ethernet/ti/netcp_core.c... unclear.
>> > > > 
>> > > > If not, maybe cpts should remain unconverted - I don't see any reason
>> > > > to provide a generic function for one user.
>> > > > 
>> > > 
>> > > Could it be an option to pass "u8 *ptr" instead of "const struct sk_buff *skb" as
>> > > input parameter to ptp_parse_header()?
>> > 
>> > It needs to read from the buffer, and in order to do that, it needs to
>> > validate that the buffer contains sufficient data.  So, at minimum it
>> > needs to be a pointer and size of valid data.
>> > 
>> > I was thinking about suggesting that as a core function, with a wrapper
>> > for the existing interface.
>> > 
>> 
>> Then length can be added.
>
> Actually, it needs more than that, because skb->data..skb->len already
> may contain the eth header or may not.
>
>> Otherwise not only CPTS can't benefit from this new API, but also
>> drivers like oki-semi/pch_gbe/pch_gbe_main.c -> pch_ptp_match()
>
> Again, this looks like it can be solved easily by swapping the position
> of these two calls:
>
>                         pch_rx_timestamp(adapter, skb);
>
>                         skb->protocol = eth_type_trans(skb, netdev);
>
>> or have to two have two APIs (name?).
>> 
>> ptp_parse_header1(struct sk_buff *skb, unsigned int type)
>> {
>> 	u8 *data = skb_mac_header(skb);
>> 
>> ptp_parse_header2(struct sk_buff *skb, unsigned int type)
>> {
>> 	u8 *data = skb->data;
>> 
>> everything else is the same.
>
> Actually, I really don't think we want 99% of users doing:
>
> 	hdr = ptp_parse_header(skb_mac_header(skb), skb->data, skb->len, type)
>
> or
>
> 	hdr = ptp_parse_header(skb_mac_header(skb), skb->data + skb->len, type);
>
> because that is what it will take, and this is starting to look
> really very horrid.

True.

>
> So, I repeat my question again: can netcp_core.c be adjusted to
> ensure that the skb mac header field is correctly set by calling
> eth_type_trans() prior to calling the rx hooks?  The other two
> cpts cases look easy to change, and the oki-semi also looks the
> same.

I think it's possible to adjust the netcp core. So, the time stamping is
done via

 gbe_rxhook() -> gbe_rxtstamp() -> cpts_rx_timestamp()

The hooks are called in netcp_process_one_rx_packet(). So, moving
eth_type_trans() before executing the hooks should work. Only one hook
is registered.

What do you think about it?

Thanks,
Kurt

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Grygorii Strashko]

On 05/08/2020 12:29, Kurt Kanzenbach wrote:
> On Wed Aug 05 2020, Russell King - ARM Linux admin wrote:
>> On Wed, Aug 05, 2020 at 01:04:31AM +0300, Grygorii Strashko wrote:
>>> On 05/08/2020 00:44, Russell King - ARM Linux admin wrote:
>>>> On Wed, Aug 05, 2020 at 12:34:47AM +0300, Grygorii Strashko wrote:
>>>>> On 05/08/2020 00:07, Russell King - ARM Linux admin wrote:
>>>>>> On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 31/07/2020 13:06, Kurt Kanzenbach wrote:
>>>>>>>> On Thu Jul 30 2020, Petr Machata wrote:
>>>>>>>>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>>>>>>>>
>>>>>>>>>> @@ -107,6 +107,37 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb)
>>>>>>>>>>      }
>>>>>>>>>>      EXPORT_SYMBOL_GPL(ptp_classify_raw);
>>>>>>>>>> +struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type)
>>>>>>>>>> +{
>>>>>>>>>> +	u8 *data = skb_mac_header(skb);
>>>>>>>>>> +	u8 *ptr = data;
>>>>>>>>>
>>>>>>>>> One of the "data" and "ptr" variables is superfluous.
>>>>>>>>
>>>>>>>> Yeah. Can be shortened to u8 *ptr = skb_mac_header(skb);
>>>>>>>
>>>>>>> Actually usage of skb_mac_header(skb) breaks CPTS RX time-stamping on
>>>>>>> am571x platform PATCH 6.
>>>>>>>
>>>>>>> The CPSW RX timestamp requested after full packet put in SKB, but
>>>>>>> before calling eth_type_trans().
>>>>>>>
>>>>>>> So, skb->data pints on Eth header, but skb_mac_header() return garbage.
>>>>>>>
>>>>>>> Below diff fixes it for me.
>>>>>>
>>>>>> However, that's likely to break everyone else.
>>>>>>
>>>>>> For example, anyone calling this from the mii_timestamper rxtstamp()
>>>>>> method, the skb will have been classified with the MAC header pushed
>>>>>> and restored, so skb->data points at the network header.
>>>>>>
>>>>>> Your change means that ptp_parse_header() expects the MAC header to
>>>>>> also be pushed.
>>>>>>
>>>>>> Is it possible to adjust CPTS?
>>>>>>
>>>>>> Looking at:
>>>>>> drivers/net/ethernet/ti/cpsw.c... yes.
>>>>>> drivers/net/ethernet/ti/cpsw_new.c... yes.
>>>>>> drivers/net/ethernet/ti/netcp_core.c... unclear.
>>>>>>
>>>>>> If not, maybe cpts should remain unconverted - I don't see any reason
>>>>>> to provide a generic function for one user.
>>>>>>
>>>>>
>>>>> Could it be an option to pass "u8 *ptr" instead of "const struct sk_buff *skb" as
>>>>> input parameter to ptp_parse_header()?
>>>>
>>>> It needs to read from the buffer, and in order to do that, it needs to
>>>> validate that the buffer contains sufficient data.  So, at minimum it
>>>> needs to be a pointer and size of valid data.
>>>>
>>>> I was thinking about suggesting that as a core function, with a wrapper
>>>> for the existing interface.
>>>>
>>>
>>> Then length can be added.
>>
>> Actually, it needs more than that, because skb->data..skb->len already
>> may contain the eth header or may not.
>>
>>> Otherwise not only CPTS can't benefit from this new API, but also
>>> drivers like oki-semi/pch_gbe/pch_gbe_main.c -> pch_ptp_match()
>>
>> Again, this looks like it can be solved easily by swapping the position
>> of these two calls:
>>
>>                          pch_rx_timestamp(adapter, skb);
>>
>>                          skb->protocol = eth_type_trans(skb, netdev);

Sry, it will not be so "easily", because there is ptp_classify_raw() inside.

So every such case, will require rework and adding magic like this

	__skb_push(skb, ETH_HLEN);

	type = ptp_classify_raw(skb);

	__skb_pull(skb, ETH_HLEN);

in Hot path.

>>
>>> or have to two have two APIs (name?).
>>>
>>> ptp_parse_header1(struct sk_buff *skb, unsigned int type)
>>> {
>>> 	u8 *data = skb_mac_header(skb);
>>>
>>> ptp_parse_header2(struct sk_buff *skb, unsigned int type)
>>> {
>>> 	u8 *data = skb->data;
>>>
>>> everything else is the same.
>>
>> Actually, I really don't think we want 99% of users doing:
>>
>> 	hdr = ptp_parse_header(skb_mac_header(skb), skb->data, skb->len, type)
>>
>> or
>>
>> 	hdr = ptp_parse_header(skb_mac_header(skb), skb->data + skb->len, type);
>>
>> because that is what it will take, and this is starting to look
>> really very horrid.
> 
> True.
> 
>>
>> So, I repeat my question again: can netcp_core.c be adjusted to
>> ensure that the skb mac header field is correctly set by calling
>> eth_type_trans() prior to calling the rx hooks?  The other two
>> cpts cases look easy to change, and the oki-semi also looks the
>> same.
> 
> I think it's possible to adjust the netcp core. So, the time stamping is
> done via
> 
>   gbe_rxhook() -> gbe_rxtstamp() -> cpts_rx_timestamp()
> 
> The hooks are called in netcp_process_one_rx_packet(). So, moving
> eth_type_trans() before executing the hooks should work. Only one hook
> is registered.
> 
> What do you think about it?

I really do not want touch netcp, sry.
There are other internal code based on this even if there is only one hooks in LKML now.
+ my comment above.

I'll try use skb_reset_mac_header(skb);
As spectrum does:
  	skb_reset_mac_header(skb);
	mlxsw_sp1_ptp_got_packet(mlxsw_sp, skb, local_port, true);

if doesn't help PATCH 6 is to drop.

-- 
Best regards,
grygorii

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Kurt Kanzenbach]

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

On Wed Aug 05 2020, Grygorii Strashko wrote:
> I really do not want touch netcp, sry.
> There are other internal code based on this even if there is only one hooks in LKML now.
> + my comment above.

OK, I see. The use of lists makes more sense now.

>
> I'll try use skb_reset_mac_header(skb);
> As spectrum does:
>   	skb_reset_mac_header(skb);
> 	mlxsw_sp1_ptp_got_packet(mlxsw_sp, skb, local_port, true);
>
> if doesn't help PATCH 6 is to drop.

So, only patch 6 is to drop or 5 as well? Anyhow, I'll wait for your
test results. Thanks!

Thanks,
Kurt

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Grygorii Strashko]

On 05/08/2020 16:57, Kurt Kanzenbach wrote:
> On Wed Aug 05 2020, Grygorii Strashko wrote:
>> I really do not want touch netcp, sry.
>> There are other internal code based on this even if there is only one hooks in LKML now.
>> + my comment above.
> 
> OK, I see. The use of lists makes more sense now.
> 
>>
>> I'll try use skb_reset_mac_header(skb);
>> As spectrum does:
>>    	skb_reset_mac_header(skb);
>> 	mlxsw_sp1_ptp_got_packet(mlxsw_sp, skb, local_port, true);
>>
>> if doesn't help PATCH 6 is to drop.
> 
> So, only patch 6 is to drop or 5 as well? Anyhow, I'll wait for your
> test results. Thanks!

Patch 5 not affected as all RX packet have timestamp and it's coming different way.
TX not affected as skb come to .xmit() properly initialized.

As I've just replied for patch 6 - skb_reset_mac_header() helps.

Rhetorical question - is below check really required?
Bad packets (short, crc) expected to be discarded by HW

	/* Ensure that the entire header is present in this packet. */
	if (ptr + sizeof(struct ptp_header) > skb->data + skb->len)
		return NULL;


And I'd like to ask you to update ptp_parse_header() documentation
with description of expected SKB state for this function to work.


-- 
Best regards,
grygorii

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Kurt Kanzenbach]

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

On Wed Aug 05 2020, Grygorii Strashko wrote:
> On 05/08/2020 16:57, Kurt Kanzenbach wrote:
>> So, only patch 6 is to drop or 5 as well? Anyhow, I'll wait for your
>> test results. Thanks!
>
> Patch 5 not affected as all RX packet have timestamp and it's coming different way.
> TX not affected as skb come to .xmit() properly initialized.

OK.

>
> As I've just replied for patch 6 - skb_reset_mac_header() helps.

OK. I'll add it. Thanks for testing and fixing it.

>
> Rhetorical question - is below check really required?
> Bad packets (short, crc) expected to be discarded by HW
>
> 	/* Ensure that the entire header is present in this packet. */
> 	if (ptr + sizeof(struct ptp_header) > skb->data + skb->len)
> 		return NULL;

Even if it's a rhetorical question - Can we rely on the fact that too
short packets (or bad) are discarded? All driver instances I've changed
in this series do the length check somehow.

>
>
> And I'd like to ask you to update ptp_parse_header() documentation
> with description of expected SKB state for this function to work.

Yes, I've wanted to do that anyway.

Thanks,
Kurt

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

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Richard Cochran]

On Tue, Aug 04, 2020 at 11:56:12PM +0300, Grygorii Strashko wrote:
> So, skb->data pints on Eth header, but skb_mac_header() return garbage.

This triggers an ancient memory in my head.  Now I vaguely recall that
there was a reason I made different parsing routines.  :(

Still I think it would be good to have the common PTP header parsing
method that can be used in most cases.

Thanks,
Richard

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Richard Cochran]

On Thu, Jul 30, 2020 at 10:00:40AM +0200, Kurt Kanzenbach wrote:
> Reason: A lot of the ptp drivers - which implement hardware time stamping - need
> specific fields such as the sequence id from the ptp v2 header. Currently all
> drivers implement that themselves.
> 
> Introduce a generic function to retrieve a pointer to the start of the ptp v2
> header.
> 
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH v3 1/9] ptp: Add generic ptp v2 header parsing function

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> Reason: A lot of the ptp drivers - which implement hardware time stamping - need
> specific fields such as the sequence id from the ptp v2 header. Currently all
> drivers implement that themselves.
> 
> Introduce a generic function to retrieve a pointer to the start of the ptp v2
> header.
> 
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v3 2/9] ptp: Add generic ptp message type function

[Kurt Kanzenbach]

The message type is located at different offsets within the ptp header depending
on the ptp version (v1 or v2). Therefore, drivers which also deal with ptp v1
have some code for it.

Extract this into a helper function for drivers to be used.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 include/linux/ptp_classify.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index 26fd38a4bd67..f4dd42fddc0c 100644
--- a/include/linux/ptp_classify.h
+++ b/include/linux/ptp_classify.h
@@ -90,6 +90,30 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb);
  */
 struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type);
 
+/**
+ * ptp_get_msgtype - Extract ptp message type from given header
+ * @hdr: ptp header
+ * @type: type of the packet (see ptp_classify_raw())
+ *
+ * This function returns the message type for a given ptp header. It takes care
+ * of the different ptp header versions (v1 or v2).
+ *
+ * Return: The message type
+ */
+static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
+				 unsigned int type)
+{
+	u8 msgtype;
+
+	if (unlikely(type & PTP_CLASS_V1))
+		/* msg type is located at the control field for ptp v1 */
+		msgtype = hdr->control;
+	else
+		msgtype = hdr->tsmt & 0x0f;
+
+	return msgtype;
+}
+
 void __init ptp_classifier_init(void);
 #else
 static inline void ptp_classifier_init(void)
-- 
2.20.1

Re: [PATCH v3 2/9] ptp: Add generic ptp message type function

[Richard Cochran]

On Thu, Jul 30, 2020 at 10:00:41AM +0200, Kurt Kanzenbach wrote:
> The message type is located at different offsets within the ptp header depending
> on the ptp version (v1 or v2). Therefore, drivers which also deal with ptp v1
> have some code for it.
> 
> Extract this into a helper function for drivers to be used.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Richard Cochran <richardcochran@gmail.com>

CodingStyle nit below...

> ---
>  include/linux/ptp_classify.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
> index 26fd38a4bd67..f4dd42fddc0c 100644
> --- a/include/linux/ptp_classify.h
> +++ b/include/linux/ptp_classify.h
> @@ -90,6 +90,30 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb);
>   */
>  struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type);
>  
> +/**
> + * ptp_get_msgtype - Extract ptp message type from given header
> + * @hdr: ptp header
> + * @type: type of the packet (see ptp_classify_raw())
> + *
> + * This function returns the message type for a given ptp header. It takes care
> + * of the different ptp header versions (v1 or v2).
> + *
> + * Return: The message type
> + */
> +static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
> +				 unsigned int type)
> +{
> +	u8 msgtype;
> +
> +	if (unlikely(type & PTP_CLASS_V1))
> +		/* msg type is located at the control field for ptp v1 */
> +		msgtype = hdr->control;

With the comment, it looks like two statements, and so please use 

	if (...) {
		/**/
		...
	} else {
		...
	}

here.

> +	else
> +		msgtype = hdr->tsmt & 0x0f;
> +
> +	return msgtype;
> +}
> +
>  void __init ptp_classifier_init(void);
>  #else
>  static inline void ptp_classifier_init(void)
> -- 
> 2.20.1
> 

Re: [PATCH v3 2/9] ptp: Add generic ptp message type function

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> The message type is located at different offsets within the ptp header depending
> on the ptp version (v1 or v2). Therefore, drivers which also deal with ptp v1
> have some code for it.
> 
> Extract this into a helper function for drivers to be used.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v3 3/9] net: dsa: mv88e6xxx: Use generic helper function

[Kurt Kanzenbach]

In order to reduce code duplication between ptp drivers, generic helper
functions were introduced. Use them.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 59 ++++++----------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index a4c488b12e8f..094d17a1d037 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -211,49 +211,20 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
 		-EFAULT : 0;
 }
 
-/* Get the start of the PTP header in this skb */
-static u8 *parse_ptp_header(struct sk_buff *skb, unsigned int type)
-{
-	u8 *data = skb_mac_header(skb);
-	unsigned int offset = 0;
-
-	if (type & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (type & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return NULL;
-	}
-
-	/* Ensure that the entire header is present in this packet. */
-	if (skb->len + ETH_HLEN < offset + 34)
-		return NULL;
-
-	return data + offset;
-}
-
 /* Returns a pointer to the PTP header if the caller should time stamp,
  * or NULL if the caller should not.
  */
-static u8 *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
-				   struct sk_buff *skb, unsigned int type)
+static struct ptp_header *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip,
+						  int port, struct sk_buff *skb,
+						  unsigned int type)
 {
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
-	u8 *hdr;
+	struct ptp_header *hdr;
 
 	if (!chip->info->ptp_support)
 		return NULL;
 
-	hdr = parse_ptp_header(skb, type);
+	hdr = ptp_parse_header(skb, type);
 	if (!hdr)
 		return NULL;
 
@@ -275,12 +246,11 @@ static int mv88e6xxx_ts_valid(u16 status)
 static int seq_match(struct sk_buff *skb, u16 ts_seqid)
 {
 	unsigned int type = SKB_PTP_TYPE(skb);
-	u8 *hdr = parse_ptp_header(skb, type);
-	__be16 *seqid;
+	struct ptp_header *hdr;
 
-	seqid = (__be16 *)(hdr + OFF_PTP_SEQUENCE_ID);
+	hdr = ptp_parse_header(skb, type);
 
-	return ts_seqid == ntohs(*seqid);
+	return ts_seqid == ntohs(hdr->sequence_id);
 }
 
 static void mv88e6xxx_get_rxts(struct mv88e6xxx_chip *chip,
@@ -357,9 +327,9 @@ static void mv88e6xxx_rxtstamp_work(struct mv88e6xxx_chip *chip,
 				   &ps->rx_queue2);
 }
 
-static int is_pdelay_resp(u8 *msgtype)
+static int is_pdelay_resp(const struct ptp_header *hdr)
 {
-	return (*msgtype & 0xf) == 3;
+	return (hdr->tsmt & 0xf) == 3;
 }
 
 bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
@@ -367,7 +337,7 @@ bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 {
 	struct mv88e6xxx_port_hwtstamp *ps;
 	struct mv88e6xxx_chip *chip;
-	u8 *hdr;
+	struct ptp_header *hdr;
 
 	chip = ds->priv;
 	ps = &chip->port_hwtstamp[port];
@@ -503,8 +473,7 @@ bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
-	__be16 *seq_ptr;
-	u8 *hdr;
+	struct ptp_header *hdr;
 
 	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
 		return false;
@@ -513,15 +482,13 @@ bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
 	if (!hdr)
 		return false;
 
-	seq_ptr = (__be16 *)(hdr + OFF_PTP_SEQUENCE_ID);
-
 	if (test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
 				  &ps->state))
 		return false;
 
 	ps->tx_skb = clone;
 	ps->tx_tstamp_start = jiffies;
-	ps->tx_seq_id = be16_to_cpup(seq_ptr);
+	ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
 
 	ptp_schedule_worker(chip->ptp_clock, 0);
 	return true;
-- 
2.20.1

Re: [PATCH v3 3/9] net: dsa: mv88e6xxx: Use generic helper function

[Richard Cochran]

On Thu, Jul 30, 2020 at 10:00:42AM +0200, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Tested-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH v3 3/9] net: dsa: mv88e6xxx: Use generic helper function

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Kurt Kanzenbach]

In order to reduce code duplication between ptp drivers, generic helper
functions were introduced. Use them.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 .../ethernet/mellanox/mlxsw/spectrum_ptp.c    | 32 ++++---------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index 9650562fc0ef..ca8090a28dec 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -314,11 +314,9 @@ static int mlxsw_sp_ptp_parse(struct sk_buff *skb,
 			      u8 *p_message_type,
 			      u16 *p_sequence_id)
 {
-	unsigned int offset = 0;
 	unsigned int ptp_class;
-	u8 *data;
+	struct ptp_header *hdr;
 
-	data = skb_mac_header(skb);
 	ptp_class = ptp_classify_raw(skb);
 
 	switch (ptp_class & PTP_CLASS_VMASK) {
@@ -329,30 +327,14 @@ static int mlxsw_sp_ptp_parse(struct sk_buff *skb,
 		return -ERANGE;
 	}
 
-	if (ptp_class & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (ptp_class & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return -ERANGE;
-	}
-
-	/* PTP header is 34 bytes. */
-	if (skb->len < offset + 34)
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
 		return -EINVAL;
 
-	*p_message_type = data[offset] & 0x0f;
-	*p_domain_number = data[offset + 4];
-	*p_sequence_id = (u16)(data[offset + 30]) << 8 | data[offset + 31];
+	*p_message_type	 = ptp_get_msgtype(hdr, ptp_class);
+	*p_domain_number = hdr->domain_number;
+	*p_sequence_id	 = be16_to_cpu(hdr->sequence_id);
+
 	return 0;
 }
 
-- 
2.20.1

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

[Petr Machata]

Kurt Kanzenbach <kurt@linutronix.de> writes:

> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-and-tested-by: Petr Machata <petrm@mellanox.com>

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

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Kurt Kanzenbach]

In order to reduce code duplication between ptp drivers, generic helper
functions were introduced. Use them.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/ti/am65-cpts.c | 37 +++++++----------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index c59a289e428c..2548324afa42 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -748,42 +748,23 @@ EXPORT_SYMBOL_GPL(am65_cpts_rx_enable);
 static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
 {
 	unsigned int ptp_class = ptp_classify_raw(skb);
-	u8 *msgtype, *data = skb->data;
-	unsigned int offset = 0;
-	__be16 *seqid;
+	struct ptp_header *hdr;
+	u8 msgtype;
+	u16 seqid;
 
 	if (ptp_class == PTP_CLASS_NONE)
 		return 0;
 
-	if (ptp_class & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (ptp_class & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return 0;
-	}
-
-	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
 		return 0;
 
-	if (unlikely(ptp_class & PTP_CLASS_V1))
-		msgtype = data + offset + OFF_PTP_CONTROL;
-	else
-		msgtype = data + offset;
+	msgtype = ptp_get_msgtype(hdr, ptp_class);
+	seqid	= be16_to_cpu(hdr->sequence_id);
 
-	seqid = (__be16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
-	*mtype_seqid = (*msgtype << AM65_CPTS_EVENT_1_MESSAGE_TYPE_SHIFT) &
+	*mtype_seqid  = (msgtype << AM65_CPTS_EVENT_1_MESSAGE_TYPE_SHIFT) &
 			AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK;
-	*mtype_seqid |= (ntohs(*seqid) & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK);
+	*mtype_seqid |= (seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK);
 
 	return 1;
 }
-- 
2.20.1

Re: [PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Grygorii Strashko]

On 30/07/2020 11:00, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/ti/am65-cpts.c | 37 +++++++----------------------
>   1 file changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index c59a289e428c..2548324afa42 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -748,42 +748,23 @@ EXPORT_SYMBOL_GPL(am65_cpts_rx_enable);
>   static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>   {
>   	unsigned int ptp_class = ptp_classify_raw(skb);
> -	u8 *msgtype, *data = skb->data;
> -	unsigned int offset = 0;
> -	__be16 *seqid;
> +	struct ptp_header *hdr;
> +	u8 msgtype;
> +	u16 seqid;
>   
>   	if (ptp_class == PTP_CLASS_NONE)
>   		return 0;
>   
> -	if (ptp_class & PTP_CLASS_VLAN)
> -		offset += VLAN_HLEN;
> -
> -	switch (ptp_class & PTP_CLASS_PMASK) {
> -	case PTP_CLASS_IPV4:
> -		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
> -		break;
> -	case PTP_CLASS_IPV6:
> -		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
> -		break;
> -	case PTP_CLASS_L2:
> -		offset += ETH_HLEN;
> -		break;
> -	default:
> -		return 0;
> -	}
> -
> -	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
> +	hdr = ptp_parse_header(skb, ptp_class);
> +	if (!hdr)
>   		return 0;
>   
> -	if (unlikely(ptp_class & PTP_CLASS_V1))
> -		msgtype = data + offset + OFF_PTP_CONTROL;
> -	else
> -		msgtype = data + offset;
> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
> +	seqid	= be16_to_cpu(hdr->sequence_id);

Is there any reason to not use "ntohs()"?

>   
> -	seqid = (__be16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
> -	*mtype_seqid = (*msgtype << AM65_CPTS_EVENT_1_MESSAGE_TYPE_SHIFT) &
> +	*mtype_seqid  = (msgtype << AM65_CPTS_EVENT_1_MESSAGE_TYPE_SHIFT) &
>   			AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK;
> -	*mtype_seqid |= (ntohs(*seqid) & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK);
> +	*mtype_seqid |= (seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK);
>   
>   	return 1;
>   }
> 

I'll try to test it today.
Thank you.

-- 
Best regards,
grygorii

Re: [PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Kurt Kanzenbach]

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

On Thu Jul 30 2020, Grygorii Strashko wrote:
> On 30/07/2020 11:00, Kurt Kanzenbach wrote:
>> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
>> +	seqid	= be16_to_cpu(hdr->sequence_id);
>
> Is there any reason to not use "ntohs()"?

This is just my personal preference, because I think it's more
readable. Internally ntohs() uses be16_to_cpu(). There's no technical
reason for it.

>
>>   
>> -	seqid = (__be16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
>> -	*mtype_seqid = (*msgtype << AM65_CPTS_EVENT_1_MESSAGE_TYPE_SHIFT) &
>> +	*mtype_seqid  = (msgtype << AM65_CPTS_EVENT_1_MESSAGE_TYPE_SHIFT) &
>>   			AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK;
>> -	*mtype_seqid |= (ntohs(*seqid) & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK);
>> +	*mtype_seqid |= (seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK);
>>   
>>   	return 1;
>>   }
>> 
>
> I'll try to test it today.
> Thank you.

Thanks,
Kurt

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

Re: [PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Arnd Bergmann]

On Thu, Jul 30, 2020 at 11:41 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
> On Thu Jul 30 2020, Grygorii Strashko wrote:
> > On 30/07/2020 11:00, Kurt Kanzenbach wrote:
> >> +    msgtype = ptp_get_msgtype(hdr, ptp_class);
> >> +    seqid   = be16_to_cpu(hdr->sequence_id);
> >
> > Is there any reason to not use "ntohs()"?
>
> This is just my personal preference, because I think it's more
> readable. Internally ntohs() uses be16_to_cpu(). There's no technical
> reason for it.

I think for traditional reasons, code in net/* tends to use ntohs()
while code in drivers/*  tends to use be16_to_cpu().

In drivers/net/* the two are used roughly the same, though I guess
one could make the argument that be16_to_cpu() would be
more appropriate for data structures exchanged with hardware
while ntohs() makes sense on data structures sent over the
network.

     Arnd

Re: [PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Kurt Kanzenbach]

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

On Thu Jul 30 2020, Arnd Bergmann wrote:
> On Thu, Jul 30, 2020 at 11:41 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>> On Thu Jul 30 2020, Grygorii Strashko wrote:
>> > On 30/07/2020 11:00, Kurt Kanzenbach wrote:
>> >> +    msgtype = ptp_get_msgtype(hdr, ptp_class);
>> >> +    seqid   = be16_to_cpu(hdr->sequence_id);
>> >
>> > Is there any reason to not use "ntohs()"?
>>
>> This is just my personal preference, because I think it's more
>> readable. Internally ntohs() uses be16_to_cpu(). There's no technical
>> reason for it.
>
> I think for traditional reasons, code in net/* tends to use ntohs()
> while code in drivers/*  tends to use be16_to_cpu().
>
> In drivers/net/* the two are used roughly the same, though I guess
> one could make the argument that be16_to_cpu() would be
> more appropriate for data structures exchanged with hardware
> while ntohs() makes sense on data structures sent over the
> network.

I see, makes sense. I could simply keep it the way it was, or?

Thanks,
Kurt

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

Re: [PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Grygorii Strashko]

On 31/07/2020 14:48, Kurt Kanzenbach wrote:
> On Thu Jul 30 2020, Arnd Bergmann wrote:
>> On Thu, Jul 30, 2020 at 11:41 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>>> On Thu Jul 30 2020, Grygorii Strashko wrote:
>>>> On 30/07/2020 11:00, Kurt Kanzenbach wrote:
>>>>> +    msgtype = ptp_get_msgtype(hdr, ptp_class);
>>>>> +    seqid   = be16_to_cpu(hdr->sequence_id);
>>>>
>>>> Is there any reason to not use "ntohs()"?
>>>
>>> This is just my personal preference, because I think it's more
>>> readable. Internally ntohs() uses be16_to_cpu(). There's no technical
>>> reason for it.
>>
>> I think for traditional reasons, code in net/* tends to use ntohs()
>> while code in drivers/*  tends to use be16_to_cpu().
>>
>> In drivers/net/* the two are used roughly the same, though I guess
>> one could make the argument that be16_to_cpu() would be
>> more appropriate for data structures exchanged with hardware
>> while ntohs() makes sense on data structures sent over the
>> network.
> 
> I see, makes sense. I could simply keep it the way it was, or?

  I prefer ntohs() as this packet data.

-- 
Best regards,
grygorii

Re: [PATCH v3 5/9] ethernet: ti: am65-cpts: Use generic helper function

[Kurt Kanzenbach]

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

On Fri Jul 31 2020, Grygorii Strashko wrote:
> On 31/07/2020 14:48, Kurt Kanzenbach wrote:
>> On Thu Jul 30 2020, Arnd Bergmann wrote:
>>> On Thu, Jul 30, 2020 at 11:41 AM Kurt Kanzenbach <kurt@linutronix.de> wrote:
>>>> On Thu Jul 30 2020, Grygorii Strashko wrote:
>>>>> On 30/07/2020 11:00, Kurt Kanzenbach wrote:
>>>>>> +    msgtype = ptp_get_msgtype(hdr, ptp_class);
>>>>>> +    seqid   = be16_to_cpu(hdr->sequence_id);
>>>>>
>>>>> Is there any reason to not use "ntohs()"?
>>>>
>>>> This is just my personal preference, because I think it's more
>>>> readable. Internally ntohs() uses be16_to_cpu(). There's no technical
>>>> reason for it.
>>>
>>> I think for traditional reasons, code in net/* tends to use ntohs()
>>> while code in drivers/*  tends to use be16_to_cpu().
>>>
>>> In drivers/net/* the two are used roughly the same, though I guess
>>> one could make the argument that be16_to_cpu() would be
>>> more appropriate for data structures exchanged with hardware
>>> while ntohs() makes sense on data structures sent over the
>>> network.
>> 
>> I see, makes sense. I could simply keep it the way it was, or?
>
>   I prefer ntohs() as this packet data.

OK. I'll change it in the next iteration.

Thanks,
Kurt

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

[PATCH v3 6/9] ethernet: ti: cpts: Use generic helper function

[Kurt Kanzenbach]

In order to reduce code duplication between ptp drivers, generic helper
functions were introduced. Use them.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/ti/cpts.c | 37 +++++++++-------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 7c55d395de2c..2c5c05620e6e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -446,41 +446,22 @@ static const struct ptp_clock_info cpts_info = {
 static int cpts_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
 {
 	unsigned int ptp_class = ptp_classify_raw(skb);
-	u8 *msgtype, *data = skb->data;
-	unsigned int offset = 0;
-	u16 *seqid;
+	struct ptp_header *hdr;
+	u8 msgtype;
+	u16 seqid;
 
 	if (ptp_class == PTP_CLASS_NONE)
 		return 0;
 
-	if (ptp_class & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (ptp_class & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return 0;
-	}
-
-	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
 		return 0;
 
-	if (unlikely(ptp_class & PTP_CLASS_V1))
-		msgtype = data + offset + OFF_PTP_CONTROL;
-	else
-		msgtype = data + offset;
+	msgtype = ptp_get_msgtype(hdr, ptp_class);
+	seqid	= be16_to_cpu(hdr->sequence_id);
 
-	seqid = (u16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
-	*mtype_seqid = (*msgtype & MESSAGE_TYPE_MASK) << MESSAGE_TYPE_SHIFT;
-	*mtype_seqid |= (ntohs(*seqid) & SEQUENCE_ID_MASK) << SEQUENCE_ID_SHIFT;
+	*mtype_seqid  = (msgtype & MESSAGE_TYPE_MASK) << MESSAGE_TYPE_SHIFT;
+	*mtype_seqid |= (seqid & SEQUENCE_ID_MASK) << SEQUENCE_ID_SHIFT;
 
 	return 1;
 }
-- 
2.20.1

Re: [PATCH v3 6/9] ethernet: ti: cpts: Use generic helper function

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
[snip]
> -	if (unlikely(ptp_class & PTP_CLASS_V1))
> -		msgtype = data + offset + OFF_PTP_CONTROL;
> -	else
> -		msgtype = data + offset;
> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
> +	seqid	= be16_to_cpu(hdr->sequence_id);

Same comment as patch 5 would probably apply here as well, with using
ntohs():

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian

Re: [PATCH v3 6/9] ethernet: ti: cpts: Use generic helper function

[Grygorii Strashko]

On 02/08/2020 23:22, Florian Fainelli wrote:
> 
> 
> On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
>> In order to reduce code duplication between ptp drivers, generic helper
>> functions were introduced. Use them.
>>
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
> [snip]
>> -	if (unlikely(ptp_class & PTP_CLASS_V1))
>> -		msgtype = data + offset + OFF_PTP_CONTROL;
>> -	else
>> -		msgtype = data + offset;
>> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
>> +	seqid	= be16_to_cpu(hdr->sequence_id);
> 
> Same comment as patch 5 would probably apply here as well, with using
> ntohs():
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>


As reported in [1] this patch as is broke cpts and below diff on top restore it

[1] https://lore.kernel.org/netdev/20200805152503.GB9122@hoboy/T/#mcf2bd0322805e6706ee9fe4f10805e657fd0103e

-- 
Best regards,
grygorii

---------------
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -509,6 +509,11 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
         int ret;
         u64 ns;
  
+       /* cpts_rx_timestamp() is called before eth_type_trans(), so
+        * skb MAC Hdr properties are not configured yet. Hence need to
+        * rest skb MAC header here
+        */
+       skb_reset_mac_header(skb);
         ret = cpts_skb_get_mtype_seqid(skb, &skb_cb->skb_mtype_seqid);
         if (!ret)
                 return;

[PATCH v3 7/9] net: phy: dp83640: Use generic helper function

[Kurt Kanzenbach]

In order to reduce code duplication between ptp drivers, generic helper
functions were introduced. Use them.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/phy/dp83640.c | 69 +++++++++------------------------------
 1 file changed, 16 insertions(+), 53 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 50fb7d16b75a..1cd987e3d0f2 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -803,46 +803,28 @@ static int decode_evnt(struct dp83640_private *dp83640,
 
 static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
 {
-	unsigned int offset = 0;
-	u8 *msgtype, *data = skb_mac_header(skb);
-	__be16 *seqid;
+	struct ptp_header *hdr;
+	u8 msgtype;
+	u16 seqid;
 	u16 hash;
 
 	/* check sequenceID, messageType, 12 bit hash of offset 20-29 */
 
-	if (type & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (type & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
+	hdr = ptp_parse_header(skb, type);
+	if (!hdr)
 		return 0;
-	}
 
-	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
-		return 0;
+	msgtype = ptp_get_msgtype(hdr, type);
 
-	if (unlikely(type & PTP_CLASS_V1))
-		msgtype = data + offset + OFF_PTP_CONTROL;
-	else
-		msgtype = data + offset;
-	if (rxts->msgtype != (*msgtype & 0xf))
+	if (rxts->msgtype != (msgtype & 0xf))
 		return 0;
 
-	seqid = (__be16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
-	if (rxts->seqid != ntohs(*seqid))
+	seqid = be16_to_cpu(hdr->sequence_id);
+	if (rxts->seqid != seqid)
 		return 0;
 
 	hash = ether_crc(DP83640_PACKET_HASH_LEN,
-			 data + offset + DP83640_PACKET_HASH_OFFSET) >> 20;
+			 (unsigned char *)&hdr->source_port_identity) >> 20;
 	if (rxts->hash != hash)
 		return 0;
 
@@ -982,35 +964,16 @@ static void decode_status_frame(struct dp83640_private *dp83640,
 
 static int is_sync(struct sk_buff *skb, int type)
 {
-	u8 *data = skb->data, *msgtype;
-	unsigned int offset = 0;
-
-	if (type & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (type & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return 0;
-	}
-
-	if (type & PTP_CLASS_V1)
-		offset += OFF_PTP_CONTROL;
+	struct ptp_header *hdr;
+	u8 msgtype;
 
-	if (skb->len < offset + 1)
+	hdr = ptp_parse_header(skb, type);
+	if (!hdr)
 		return 0;
 
-	msgtype = data + offset;
+	msgtype = ptp_get_msgtype(hdr, type);
 
-	return (*msgtype & 0xf) == 0;
+	return (msgtype & 0xf) == 0;
 }
 
 static void dp83640_free_clocks(void)
-- 
2.20.1

Re: [PATCH v3 7/9] net: phy: dp83640: Use generic helper function

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 7/9] net: phy: dp83640: Use generic helper function

[Richard Cochran]

On Thu, Jul 30, 2020 at 10:00:46AM +0200, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/phy/dp83640.c | 69 +++++++++------------------------------
>  1 file changed, 16 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index 50fb7d16b75a..1cd987e3d0f2 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -803,46 +803,28 @@ static int decode_evnt(struct dp83640_private *dp83640,
>  
>  static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
>  {
> -	unsigned int offset = 0;
> -	u8 *msgtype, *data = skb_mac_header(skb);
> -	__be16 *seqid;
> +	struct ptp_header *hdr;
> +	u8 msgtype;
> +	u16 seqid;
>  	u16 hash;
>  
>  	/* check sequenceID, messageType, 12 bit hash of offset 20-29 */
>  
> -	if (type & PTP_CLASS_VLAN)
> -		offset += VLAN_HLEN;
> -
> -	switch (type & PTP_CLASS_PMASK) {
> -	case PTP_CLASS_IPV4:
> -		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
> -		break;
> -	case PTP_CLASS_IPV6:
> -		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
> -		break;
> -	case PTP_CLASS_L2:
> -		offset += ETH_HLEN;
> -		break;
> -	default:
> +	hdr = ptp_parse_header(skb, type);
> +	if (!hdr)
>  		return 0;
> -	}
>  
> -	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
> -		return 0;
> +	msgtype = ptp_get_msgtype(hdr, type);
>  
> -	if (unlikely(type & PTP_CLASS_V1))
> -		msgtype = data + offset + OFF_PTP_CONTROL;
> -	else
> -		msgtype = data + offset;
> -	if (rxts->msgtype != (*msgtype & 0xf))
> +	if (rxts->msgtype != (msgtype & 0xf))
>  		return 0;
>  
> -	seqid = (__be16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
> -	if (rxts->seqid != ntohs(*seqid))
> +	seqid = be16_to_cpu(hdr->sequence_id);
> +	if (rxts->seqid != seqid)
>  		return 0;
>  
>  	hash = ether_crc(DP83640_PACKET_HASH_LEN,
> -			 data + offset + DP83640_PACKET_HASH_OFFSET) >> 20;
> +			 (unsigned char *)&hdr->source_port_identity) >> 20;

Looks like DP83640_PACKET_HASH_OFFSET can be removed now.

Tested-by: Richard Cochran <richardcochran@gmail.com>

[PATCH v3 8/9] ptp: ptp_ines: Use generic helper function

[Kurt Kanzenbach]

In order to reduce code duplication between ptp drivers, generic helper
functions were introduced. Use them.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/ptp/ptp_ines.c | 88 ++++++++++++------------------------------
 1 file changed, 25 insertions(+), 63 deletions(-)

diff --git a/drivers/ptp/ptp_ines.c b/drivers/ptp/ptp_ines.c
index 7711651ff19e..d726c589edda 100644
--- a/drivers/ptp/ptp_ines.c
+++ b/drivers/ptp/ptp_ines.c
@@ -93,9 +93,6 @@ MODULE_LICENSE("GPL");
 #define TC_E2E_PTP_V2		2
 #define TC_P2P_PTP_V2		3
 
-#define OFF_PTP_CLOCK_ID	20
-#define OFF_PTP_PORT_NUM	28
-
 #define PHY_SPEED_10		0
 #define PHY_SPEED_100		1
 #define PHY_SPEED_1000		2
@@ -443,57 +440,41 @@ static void ines_link_state(struct mii_timestamper *mii_ts,
 static bool ines_match(struct sk_buff *skb, unsigned int ptp_class,
 		       struct ines_timestamp *ts, struct device *dev)
 {
-	u8 *msgtype, *data = skb_mac_header(skb);
-	unsigned int offset = 0;
-	__be16 *portn, *seqid;
-	__be64 *clkid;
+	struct ptp_header *hdr;
+	u16 portn, seqid;
+	u8 msgtype;
+	u64 clkid;
 
 	if (unlikely(ptp_class & PTP_CLASS_V1))
 		return false;
 
-	if (ptp_class & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
-
-	switch (ptp_class & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return false;
-	}
-
-	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
 		return false;
 
-	msgtype = data + offset;
-	clkid = (__be64 *)(data + offset + OFF_PTP_CLOCK_ID);
-	portn = (__be16 *)(data + offset + OFF_PTP_PORT_NUM);
-	seqid = (__be16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
+	msgtype = ptp_get_msgtype(hdr, ptp_class);
+	clkid = be64_to_cpup((__be64 *)&hdr->source_port_identity.clock_identity.id[0]);
+	portn = be16_to_cpu(hdr->source_port_identity.port_number);
+	seqid = be16_to_cpu(hdr->sequence_id);
 
-	if (tag_to_msgtype(ts->tag & 0x7) != (*msgtype & 0xf)) {
+	if (tag_to_msgtype(ts->tag & 0x7) != msgtype) {
 		dev_dbg(dev, "msgtype mismatch ts %hhu != skb %hhu\n",
-			  tag_to_msgtype(ts->tag & 0x7), *msgtype & 0xf);
+			tag_to_msgtype(ts->tag & 0x7), msgtype);
 		return false;
 	}
-	if (cpu_to_be64(ts->clkid) != *clkid) {
+	if (ts->clkid != clkid) {
 		dev_dbg(dev, "clkid mismatch ts %llx != skb %llx\n",
-			  cpu_to_be64(ts->clkid), *clkid);
+			ts->clkid, clkid);
 		return false;
 	}
-	if (ts->portnum != ntohs(*portn)) {
+	if (ts->portnum != portn) {
 		dev_dbg(dev, "portn mismatch ts %hu != skb %hu\n",
-			  ts->portnum, ntohs(*portn));
+			ts->portnum, portn);
 		return false;
 	}
-	if (ts->seqid != ntohs(*seqid)) {
+	if (ts->seqid != seqid) {
 		dev_dbg(dev, "seqid mismatch ts %hu != skb %hu\n",
-			  ts->seqid, ntohs(*seqid));
+			ts->seqid, seqid);
 		return false;
 	}
 
@@ -694,35 +675,16 @@ static void ines_txtstamp_work(struct work_struct *work)
 
 static bool is_sync_pdelay_resp(struct sk_buff *skb, int type)
 {
-	u8 *data = skb->data, *msgtype;
-	unsigned int offset = 0;
-
-	if (type & PTP_CLASS_VLAN)
-		offset += VLAN_HLEN;
+	struct ptp_header *hdr;
+	u8 msgtype;
 
-	switch (type & PTP_CLASS_PMASK) {
-	case PTP_CLASS_IPV4:
-		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
-		break;
-	case PTP_CLASS_IPV6:
-		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
-		break;
-	case PTP_CLASS_L2:
-		offset += ETH_HLEN;
-		break;
-	default:
-		return 0;
-	}
-
-	if (type & PTP_CLASS_V1)
-		offset += OFF_PTP_CONTROL;
-
-	if (skb->len < offset + 1)
-		return 0;
+	hdr = ptp_parse_header(skb, type);
+	if (!hdr)
+		return false;
 
-	msgtype = data + offset;
+	msgtype = ptp_get_msgtype(hdr, type);
 
-	switch ((*msgtype & 0xf)) {
+	switch ((msgtype & 0xf)) {
 	case SYNC:
 	case PDELAY_RESP:
 		return true;
-- 
2.20.1

Re: [PATCH v3 8/9] ptp: ptp_ines: Use generic helper function

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> In order to reduce code duplication between ptp drivers, generic helper
> functions were introduced. Use them.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH v3 9/9] ptp: Remove unused macro

[Kurt Kanzenbach]

The offset for the control field is not needed anymore. Remove it.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 include/linux/ptp_classify.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index f4dd42fddc0c..88408fbb0ab6 100644
--- a/include/linux/ptp_classify.h
+++ b/include/linux/ptp_classify.h
@@ -36,7 +36,6 @@
 
 #define OFF_PTP_SOURCE_UUID	22 /* PTPv1 only */
 #define OFF_PTP_SEQUENCE_ID	30
-#define OFF_PTP_CONTROL		32 /* PTPv1 only */
 
 /* Below defines should actually be removed at some point in time. */
 #define IP6_HLEN	40
-- 
2.20.1

Re: [PATCH v3 9/9] ptp: Remove unused macro

[Florian Fainelli]

On 7/30/2020 1:00 AM, Kurt Kanzenbach wrote:
> The offset for the control field is not needed anymore. Remove it.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian