| Message ID | 20200730080048.32553-2-kurt@linutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ptp: Add generic helper functions | expand |
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 = {
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
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>
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>
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.
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.
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()?
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.
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.
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.
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
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.
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
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
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.
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
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 = {
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(+)