| Message ID | 20200727090601.6500-5-kurt@linutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ptp: Add generic header parsing function | expand |
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.
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
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
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 :)
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?
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
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 :) >
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
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
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.
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
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; }
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(-)