* [PATCH v2 1/9] ptp: Add generic ptp v2 header parsing function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
@ 2020-07-27 9:05 ` Kurt Kanzenbach
2020-07-27 9:05 ` [PATCH v2 2/9] ptp: Add generic ptp message type function Kurt Kanzenbach
` (8 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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.
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
include/linux/ptp_classify.h | 38 ++++++++++++++++++++++++++++++++++++
net/core/ptp_classifier.c | 30 ++++++++++++++++++++++++++++
2 files changed, 68 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..dc12338bf3cd 100644
--- a/net/core/ptp_classifier.c
+++ b/net/core/ptp_classifier.c
@@ -107,6 +107,36 @@ 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);
+ 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 + sizeof(struct ptp_header))
+ return NULL;
+
+ return (struct ptp_header *)(data + offset);
+}
+EXPORT_SYMBOL_GPL(ptp_parse_header);
+
void __init ptp_classifier_init(void)
{
static struct sock_filter ptp_filter[] __initdata = {
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 2/9] ptp: Add generic ptp message type function
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 ` Kurt Kanzenbach
2020-07-27 9:05 ` [PATCH v2 3/9] net: dsa: mv88e6xxx: Use generic helper function Kurt Kanzenbach
` (7 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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..e13f9c6150ad 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 @ offset 20 for ptp v1 */
+ msgtype = hdr->source_port_identity.clock_identity.id[0];
+ else
+ msgtype = hdr->tsmt & 0x0f;
+
+ return msgtype;
+}
+
void __init ptp_classifier_init(void);
#else
static inline void ptp_classifier_init(void)
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 3/9] net: dsa: mv88e6xxx: Use generic helper function
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 ` Kurt Kanzenbach
2020-07-27 9:05 ` [PATCH v2 4/9] mlxsw: spectrum_ptp: " Kurt Kanzenbach
` (6 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (2 preceding siblings ...)
2020-07-27 9:05 ` [PATCH v2 3/9] net: dsa: mv88e6xxx: Use generic helper function Kurt Kanzenbach
@ 2020-07-27 9:05 ` Kurt Kanzenbach
2020-07-27 13:00 ` Petr Machata
2020-07-27 9:05 ` [PATCH v2 5/9] ethernet: ti: am65-cpts: " Kurt Kanzenbach
` (5 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
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
0 siblings, 1 reply; 23+ messages in thread
From: Petr Machata @ 2020-07-27 13:00 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Richard Cochran, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
Heiner Kallweit, Russell King, Grygorii Strashko,
Ivan Khoronzhuk, Samuel Zou, netdev
Kurt Kanzenbach <kurt@linutronix.de> writes:
> @@ -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;
So this looks good, and works, but I'm wondering about one thing.
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.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
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
0 siblings, 2 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-28 13:29 UTC (permalink / raw)
To: Petr Machata
Cc: Richard Cochran, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
Heiner Kallweit, Russell King, Grygorii Strashko,
Ivan Khoronzhuk, Samuel Zou, netdev
[-- 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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-28 13:29 ` Kurt Kanzenbach
@ 2020-07-28 13:41 ` Kurt Kanzenbach
2020-07-28 21:06 ` Petr Machata
1 sibling, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-28 13:41 UTC (permalink / raw)
To: Petr Machata
Cc: Richard Cochran, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
Heiner Kallweit, Russell King, Grygorii Strashko,
Ivan Khoronzhuk, Samuel Zou, netdev
[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]
On Tue Jul 28 2020, Kurt Kanzenbach wrote:
> 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);
> |}
Sorry, that was the wrong function. I meant this one here:
|static void mlxsw_sp_rx_ptp_listener(struct sk_buff *skb, u8 local_port,
| void *trap_ctx)
|{
| [...]
| /* The PTP handler expects skb->data to point to the start of the
| * Ethernet header.
| */
| skb_push(skb, ETH_HLEN);
| mlxsw_sp_ptp_receive(mlxsw_sp, skb, local_port);
|}
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
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
2020-07-29 12:23 ` Grygorii Strashko
1 sibling, 2 replies; 23+ messages in thread
From: Petr Machata @ 2020-07-28 21:06 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Richard Cochran, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
Heiner Kallweit, Russell King, Grygorii Strashko,
Ivan Khoronzhuk, Samuel Zou, netdev
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.
However, I find it odd that ptp_classify_raw() assumes that ->data
points at Ethernet, while ptp_parse_header() makes the contrary
assumption that ->len does not cover Ethernet. Those functions are
likely to be used just a couple calls away from each other, if not
outright next to each other.
I suspect that ti/cpts.c and ti/am65-cpts.c (patches 5 and 6) actually
hit an issue in this. ptp_classify_raw() is called without a surrounding
_push / _pull (unlike DSA), which would imply skb->data points at
Ethernet header, and indeed, the way the "data" variable is used
confirms it. (At the same time the code adds ETH_HLEN to skb->len, but
maybe it is just a cut'n'paste.) But then ptp_parse_header() is called,
and that makes the assumption that skb->len does not cover the Ethernet
header.
> 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?
Yeah, I noticed that as well. That was a bug in the mlxsw code. Good
riddance :)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-28 21:06 ` Petr Machata
@ 2020-07-29 10:02 ` Russell King - ARM Linux admin
2020-07-29 10:29 ` Kurt Kanzenbach
2020-07-29 13:22 ` Richard Cochran
2020-07-29 12:23 ` Grygorii Strashko
1 sibling, 2 replies; 23+ messages in thread
From: Russell King - ARM Linux admin @ 2020-07-29 10:02 UTC (permalink / raw)
To: Petr Machata
Cc: Kurt Kanzenbach, Richard Cochran, Andrew Lunn, Vivien Didelot,
Florian Fainelli, David S. Miller, Jakub Kicinski, Jiri Pirko,
Ido Schimmel, Heiner Kallweit, Grygorii Strashko,
Ivan Khoronzhuk, Samuel Zou, netdev
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!
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-29 10:02 ` Russell King - ARM Linux admin
@ 2020-07-29 10:29 ` Kurt Kanzenbach
2020-07-29 13:49 ` Richard Cochran
2020-07-29 14:07 ` Petr Machata
2020-07-29 13:22 ` Richard Cochran
1 sibling, 2 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-29 10:29 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Petr Machata
Cc: Richard Cochran, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
Heiner Kallweit, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev
[-- 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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
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
1 sibling, 1 reply; 23+ messages in thread
From: Richard Cochran @ 2020-07-29 13:49 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Russell King - ARM Linux admin, Petr Machata, Andrew Lunn,
Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou, netdev
On Wed, Jul 29, 2020 at 12:29:08PM +0200, Kurt Kanzenbach wrote:
> 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?
My source back in the day was the John Eidson book. In Appendix A it claims
Table A.1. Common PTP message fields
Field name Purpose
--------------------------------------------------------------------
messageType Identifies message as event or general
control Indicates the message type, e.g., Sync
So I think the code is correct.
Thanks,
Richard
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-29 13:49 ` Richard Cochran
@ 2020-07-29 14:26 ` Kurt Kanzenbach
0 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-29 14:26 UTC (permalink / raw)
To: Richard Cochran
Cc: Russell King - ARM Linux admin, Petr Machata, Andrew Lunn,
Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou, netdev
[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]
On Wed Jul 29 2020, Richard Cochran wrote:
> On Wed, Jul 29, 2020 at 12:29:08PM +0200, Kurt Kanzenbach wrote:
>> 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?
>
>
> My source back in the day was the John Eidson book. In Appendix A it claims
>
>
> Table A.1. Common PTP message fields
>
> Field name Purpose
> --------------------------------------------------------------------
> messageType Identifies message as event or general
> control Indicates the message type, e.g., Sync
OK. That was the missing piece. I'll adjust patch 2 accordingly. Thanks
a lot.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-29 10:29 ` Kurt Kanzenbach
2020-07-29 13:49 ` Richard Cochran
@ 2020-07-29 14:07 ` Petr Machata
1 sibling, 0 replies; 23+ messages in thread
From: Petr Machata @ 2020-07-29 14:07 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Russell King - ARM Linux admin, Richard Cochran, Andrew Lunn,
Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou, netdev
Kurt Kanzenbach <kurt@linutronix.de> writes:
> On Wed Jul 29 2020, Russell King - ARM Linux admin wrote:
>> 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?
I like this!
> 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?
0x20 == 32? I see it at offset 32 in IEEE 1588.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-29 10:02 ` Russell King - ARM Linux admin
2020-07-29 10:29 ` Kurt Kanzenbach
@ 2020-07-29 13:22 ` Richard Cochran
1 sibling, 0 replies; 23+ messages in thread
From: Richard Cochran @ 2020-07-29 13:22 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Petr Machata, Kurt Kanzenbach, Andrew Lunn, Vivien Didelot,
Florian Fainelli, David S. Miller, Jakub Kicinski, Jiri Pirko,
Ido Schimmel, Heiner Kallweit, Grygorii Strashko,
Ivan Khoronzhuk, Samuel Zou, netdev
On Wed, Jul 29, 2020 at 11:02:57AM +0100, Russell King - ARM Linux admin wrote:
> in other words, compare pointers, so that whether skb_push() etc has
> been used on the skb is irrelevant?
+1
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function
2020-07-28 21:06 ` Petr Machata
2020-07-29 10:02 ` Russell King - ARM Linux admin
@ 2020-07-29 12:23 ` Grygorii Strashko
1 sibling, 0 replies; 23+ messages in thread
From: Grygorii Strashko @ 2020-07-29 12:23 UTC (permalink / raw)
To: Petr Machata, Kurt Kanzenbach
Cc: Richard Cochran, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S. Miller, Jakub Kicinski, Jiri Pirko, Ido Schimmel,
Heiner Kallweit, Russell King, Ivan Khoronzhuk, Samuel Zou,
netdev
On 29/07/2020 00:06, 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.
>
> However, I find it odd that ptp_classify_raw() assumes that ->data
> points at Ethernet, while ptp_parse_header() makes the contrary
> assumption that ->len does not cover Ethernet. Those functions are
> likely to be used just a couple calls away from each other, if not
> outright next to each other.
>
> I suspect that ti/cpts.c and ti/am65-cpts.c (patches 5 and 6) actually
> hit an issue in this. ptp_classify_raw() is called without a surrounding
> _push / _pull (unlike DSA), which would imply skb->data points at
> Ethernet header, and indeed, the way the "data" variable is used
> confirms it.
Both drivers, in all cases, will get
->data points at Ethernet header and
->len covers ETH_HLEN
So, yes below check is incorrect, in general, and will be false always if other
calculation are correct
(only IPV4_HLEN(data + offset) can cause issues).
if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
return 0;
it might be good to update ptp_parse_header() documentation with expected state of skb.
(At the same time the code adds ETH_HLEN to skb->len, but
> maybe it is just a cut'n'paste.) But then ptp_parse_header() is called,
> and that makes the assumption that skb->len does not cover the Ethernet
> header.
>
>> 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?
>
> Yeah, I noticed that as well. That was a bug in the mlxsw code. Good
> riddance :)
>
--
Best regards,
grygorii
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 5/9] ethernet: ti: am65-cpts: Use generic helper function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (3 preceding siblings ...)
2020-07-27 9:05 ` [PATCH v2 4/9] mlxsw: spectrum_ptp: " Kurt Kanzenbach
@ 2020-07-27 9:05 ` Kurt Kanzenbach
2020-07-27 9:05 ` [PATCH v2 6/9] ethernet: ti: cpts: " Kurt Kanzenbach
` (4 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 6/9] ethernet: ti: cpts: Use generic helper function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (4 preceding siblings ...)
2020-07-27 9:05 ` [PATCH v2 5/9] ethernet: ti: am65-cpts: " Kurt Kanzenbach
@ 2020-07-27 9:05 ` Kurt Kanzenbach
2020-07-27 9:05 ` [PATCH v2 7/9] net: phy: dp83640: " Kurt Kanzenbach
` (3 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 7/9] net: phy: dp83640: Use generic helper function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (5 preceding siblings ...)
2020-07-27 9:05 ` [PATCH v2 6/9] ethernet: ti: cpts: " Kurt Kanzenbach
@ 2020-07-27 9:05 ` Kurt Kanzenbach
2020-07-27 9:06 ` [PATCH v2 8/9] ptp: ptp_ines: " Kurt Kanzenbach
` (2 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:05 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 8/9] ptp: ptp_ines: Use generic helper function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (6 preceding siblings ...)
2020-07-27 9:05 ` [PATCH v2 7/9] net: phy: dp83640: " Kurt Kanzenbach
@ 2020-07-27 9:06 ` 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
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:06 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 9/9] ptp: Remove unused macro
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (7 preceding siblings ...)
2020-07-27 9:06 ` [PATCH v2 8/9] ptp: ptp_ines: " Kurt Kanzenbach
@ 2020-07-27 9:06 ` Kurt Kanzenbach
2020-07-29 0:47 ` [PATCH v2 0/9] ptp: Add generic header parsing function David Miller
9 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-27 9:06 UTC (permalink / raw)
To: Richard Cochran
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, Jiri Pirko, Ido Schimmel, Heiner Kallweit,
Russell King, Grygorii Strashko, Ivan Khoronzhuk, Samuel Zou,
netdev, 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 e13f9c6150ad..d75ae03cd601 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v2 0/9] ptp: Add generic header parsing function
2020-07-27 9:05 [PATCH v2 0/9] ptp: Add generic header parsing function Kurt Kanzenbach
` (8 preceding siblings ...)
2020-07-27 9:06 ` [PATCH v2 9/9] ptp: Remove unused macro Kurt Kanzenbach
@ 2020-07-29 0:47 ` David Miller
2020-07-29 9:31 ` Kurt Kanzenbach
9 siblings, 1 reply; 23+ messages in thread
From: David Miller @ 2020-07-29 0:47 UTC (permalink / raw)
To: kurt
Cc: richardcochran, andrew, vivien.didelot, f.fainelli, kuba, jiri,
idosch, hkallweit1, linux, grygorii.strashko, ivan.khoronzhuk,
zou_wei, netdev
From: Kurt Kanzenbach <kurt@linutronix.de>
Date: Mon, 27 Jul 2020 11:05:52 +0200
> in order to reduce code duplication in the ptp code of DSA, Ethernet and Phy
> drivers, move the header parsing function to ptp_classify. This way all drivers
> can share the same implementation. Implemented as discussed [1] [2] [3].
>
> This is version two and contains more driver conversions.
>
> Richard, can you test with your hardware? I'll do the same e.g. on the bbb.
>
> Version 1 can be found here:
>
> * https://lkml.kernel.org/netdev/20200723074946.14253-1-kurt@linutronix.de/
It looks like some mlxsw et al. issues wrt. which header is expected at
skb->data when certain helper functions are invoked need to be resolved
still.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 0/9] ptp: Add generic header parsing function
2020-07-29 0:47 ` [PATCH v2 0/9] ptp: Add generic header parsing function David Miller
@ 2020-07-29 9:31 ` Kurt Kanzenbach
0 siblings, 0 replies; 23+ messages in thread
From: Kurt Kanzenbach @ 2020-07-29 9:31 UTC (permalink / raw)
To: David Miller
Cc: richardcochran, andrew, vivien.didelot, f.fainelli, kuba, jiri,
idosch, hkallweit1, linux, grygorii.strashko, ivan.khoronzhuk,
zou_wei, netdev
[-- Attachment #1: Type: text/plain, Size: 301 bytes --]
On Tue Jul 28 2020, David Miller wrote:
> It looks like some mlxsw et al. issues wrt. which header is expected at
> skb->data when certain helper functions are invoked need to be resolved
> still.
Yes, the length check needs to be sorted out first. So, please don't
merge this version.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread