| Message ID | 20220825090242.12848-3-ihuguet@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: add support for PTP over IPv6 and 802.3 | expand |
On Thu, 25 Aug 2022 11:02:41 +0200 Íñigo Huguet wrote: > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, u16 port) > +static inline void efx_ptp_init_filter(struct efx_nic *efx, > + struct efx_filter_spec *rxfilter) No static inline in sources unless you actually checked and the compiler does something stupid (pls mention it in the commit message in that case). > +static inline int > +efx_filter_set_ipv6_local(struct efx_filter_spec *spec, u8 proto, > + const struct in6_addr *host, __be16 port) also - unclear why this is defined in the header
On Fri, Aug 26, 2022 at 3:32 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 25 Aug 2022 11:02:41 +0200 Íñigo Huguet wrote: > > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, u16 port) > > +static inline void efx_ptp_init_filter(struct efx_nic *efx, > > + struct efx_filter_spec *rxfilter) > > No static inline in sources unless you actually checked and the > compiler does something stupid (pls mention it in the commit message > in that case). OK, I will change it (I think I should read again and remember the coding style document) > > > +static inline int > > +efx_filter_set_ipv6_local(struct efx_filter_spec *spec, u8 proto, > > + const struct in6_addr *host, __be16 port) > > also - unclear why this is defined in the header > This is just because it's the equivalent of other already existing similar functions in that file. I think I should keep this one untouched for cohesion.
On Fri, 26 Aug 2022 08:39:44 +0200 Íñigo Huguet wrote: > > > +static inline int > > > +efx_filter_set_ipv6_local(struct efx_filter_spec *spec, u8 proto, > > > + const struct in6_addr *host, __be16 port) > > > > also - unclear why this is defined in the header > > This is just because it's the equivalent of other already existing > similar functions in that file. I think I should keep this one > untouched for cohesion. We usually defer refactoring for coding style issues until someone is otherwise touching the code, so surrounding code doing something against the guidance may be misleading.
On Sat, Aug 27, 2022 at 1:27 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 26 Aug 2022 08:39:44 +0200 Íñigo Huguet wrote: > > > > +static inline int > > > > +efx_filter_set_ipv6_local(struct efx_filter_spec *spec, u8 proto, > > > > + const struct in6_addr *host, __be16 port) > > > > > > also - unclear why this is defined in the header > > > > This is just because it's the equivalent of other already existing > > similar functions in that file. I think I should keep this one > > untouched for cohesion. > > We usually defer refactoring for coding style issues until someone > is otherwise touching the code, so surrounding code doing something > against the guidance may be misleading. > Yes but I'm not sure what I should do in this case... all other efx_filter_xxx functions are in filter.h, so putting this one in a different place could make it difficult to understand how the files are organized. Should I put the declaration in the header (without `inline`) and the definition in a new filter.c file? Should I move all other definitions to this new file? Also, what's exactly the rule, apart from not using `inline`, to avoid doing the same thing again: to avoid function definitions directly in header files? Thanks
On Mon, 29 Aug 2022 09:03:44 +0200 Íñigo Huguet wrote: > > We usually defer refactoring for coding style issues until someone > > is otherwise touching the code, so surrounding code doing something > > against the guidance may be misleading. > > Yes but I'm not sure what I should do in this case... all other > efx_filter_xxx functions are in filter.h, so putting this one in a > different place could make it difficult to understand how the files > are organized. Should I put the declaration in the header (without > `inline`) and the definition in a new filter.c file? Should I move all > other definitions to this new file? Hm, I see, perhaps adding a new filter.c would be too much for your set. Let's leave the definition in the header then. > Also, what's exactly the rule, apart from not using `inline`, to avoid > doing the same thing again: to avoid function definitions directly in > header files? Not sure I'm parsing the question right, but it's okay to add small functions in local headers. Here it seem to have only been used in one place, and I didn't see the context.
On Tue, Aug 30, 2022 at 2:29 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 29 Aug 2022 09:03:44 +0200 Íñigo Huguet wrote: > > > We usually defer refactoring for coding style issues until someone > > > is otherwise touching the code, so surrounding code doing something > > > against the guidance may be misleading. > > > > Yes but I'm not sure what I should do in this case... all other > > efx_filter_xxx functions are in filter.h, so putting this one in a > > different place could make it difficult to understand how the files > > are organized. Should I put the declaration in the header (without > > `inline`) and the definition in a new filter.c file? Should I move all > > other definitions to this new file? > > Hm, I see, perhaps adding a new filter.c would be too much for your set. > Let's leave the definition in the header then. > > > Also, what's exactly the rule, apart from not using `inline`, to avoid > > doing the same thing again: to avoid function definitions directly in > > header files? > > Not sure I'm parsing the question right, but it's okay to add small > functions in local headers. Here it seem to have only been used in > one place, and I didn't see the context. > I expresed it terribly badly, but you parsed it right. Thanks, now I understand what your concern was.
On 26/08/2022 07:39, Íñigo Huguet wrote: > On Fri, Aug 26, 2022 at 3:32 AM Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Thu, 25 Aug 2022 11:02:41 +0200 Íñigo Huguet wrote: >>> +static inline int >>> +efx_filter_set_ipv6_local(struct efx_filter_spec *spec, u8 proto, >>> + const struct in6_addr *host, __be16 port) >> >> also - unclear why this is defined in the header >> > > This is just because it's the equivalent of other already existing > similar functions in that file. I think I should keep this one > untouched for cohesion. My preference would be to keep this in filter.h as Íñigo currently has it, to follow the existing pattern. These "populate a filter spec" functions are really just typesafe macros. -ed
diff --git a/drivers/net/ethernet/sfc/filter.h b/drivers/net/ethernet/sfc/filter.h index 4d928839d292..be72e71da027 100644 --- a/drivers/net/ethernet/sfc/filter.h +++ b/drivers/net/ethernet/sfc/filter.h @@ -9,6 +9,7 @@ #include <linux/types.h> #include <linux/if_ether.h> +#include <linux/in6.h> #include <asm/byteorder.h> /** @@ -223,6 +224,27 @@ efx_filter_set_ipv4_local(struct efx_filter_spec *spec, u8 proto, return 0; } +/** + * efx_filter_set_ipv6_local - specify IPv6 host, transport protocol and port + * @spec: Specification to initialise + * @proto: Transport layer protocol number + * @host: Local host address (network byte order) + * @port: Local port (network byte order) + */ +static inline int +efx_filter_set_ipv6_local(struct efx_filter_spec *spec, u8 proto, + const struct in6_addr *host, __be16 port) +{ + spec->match_flags |= + EFX_FILTER_MATCH_ETHER_TYPE | EFX_FILTER_MATCH_IP_PROTO | + EFX_FILTER_MATCH_LOC_HOST | EFX_FILTER_MATCH_LOC_PORT; + spec->ether_type = htons(ETH_P_IPV6); + spec->ip_proto = proto; + memcpy(spec->loc_host, host, sizeof(spec->loc_host)); + spec->loc_port = port; + return 0; +} + /** * efx_filter_set_ipv4_full - specify IPv4 hosts, transport protocol and ports * @spec: Specification to initialise diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c index 719005d79943..060525ac2baf 100644 --- a/drivers/net/ethernet/sfc/ptp.c +++ b/drivers/net/ethernet/sfc/ptp.c @@ -118,9 +118,11 @@ #define PTP_MIN_LENGTH 63 -#define PTP_RXFILTERS_LEN 2 +#define PTP_RXFILTERS_LEN 4 -#define PTP_ADDRESS 0xe0000181 /* 224.0.1.129 */ +#define PTP_ADDR_IPV4 0xe0000181 /* 224.0.1.129 */ +#define PTP_ADDR_IPV6 {0xff, 0x0e, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, \ + 0, 0x01, 0x81} /* ff0e::181 */ #define PTP_EVENT_PORT 319 #define PTP_GENERAL_PORT 320 @@ -1297,29 +1299,49 @@ static void efx_ptp_remove_multicast_filters(struct efx_nic *efx) } } -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, u16 port) +static inline void efx_ptp_init_filter(struct efx_nic *efx, + struct efx_filter_spec *rxfilter) { - struct efx_ptp_data *ptp = efx->ptp_data; - struct efx_filter_spec rxfilter; - int rc; + struct efx_channel *channel = efx->ptp_data->channel; + struct efx_rx_queue *queue = efx_channel_get_rx_queue(channel); - efx_filter_init_rx(&rxfilter, EFX_FILTER_PRI_REQUIRED, 0, - efx_rx_queue_index( - efx_channel_get_rx_queue(ptp->channel))); + efx_filter_init_rx(rxfilter, EFX_FILTER_PRI_REQUIRED, 0, + efx_rx_queue_index(queue)); +} - efx_filter_set_ipv4_local(&rxfilter, IPPROTO_UDP, htonl(PTP_ADDRESS), - htons(port)); +static inline int efx_ptp_insert_filter(struct efx_nic *efx, + struct efx_filter_spec *rxfilter) +{ + struct efx_ptp_data *ptp = efx->ptp_data; - rc = efx_filter_insert_filter(efx, &rxfilter, true); + int rc = efx_filter_insert_filter(efx, rxfilter, true); if (rc < 0) return rc; - ptp->rxfilters[ptp->rxfilters_count] = rc; ptp->rxfilters_count++; - return 0; } +static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, u16 port) +{ + struct efx_filter_spec rxfilter; + + efx_ptp_init_filter(efx, &rxfilter); + efx_filter_set_ipv4_local(&rxfilter, IPPROTO_UDP, htonl(PTP_ADDR_IPV4), + htons(port)); + return efx_ptp_insert_filter(efx, &rxfilter); +} + +static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, u16 port) +{ + const struct in6_addr addr = {{PTP_ADDR_IPV6}}; + struct efx_filter_spec rxfilter; + + efx_ptp_init_filter(efx, &rxfilter); + efx_filter_set_ipv6_local(&rxfilter, IPPROTO_UDP, &addr, htons(port)); + return efx_ptp_insert_filter(efx, &rxfilter); +} + static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) { struct efx_ptp_data *ptp = efx->ptp_data; @@ -1336,6 +1358,19 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) if (rc < 0) goto fail; + /* if the NIC supports hw timestamps by the MAC, we can support + * PTP over IPv6 + */ + if (efx_ptp_use_mac_tx_timestamps(efx)) { + rc = efx_ptp_insert_ipv6_filter(efx, PTP_EVENT_PORT); + if (rc < 0) + goto fail; + + rc = efx_ptp_insert_ipv6_filter(efx, PTP_GENERAL_PORT); + if (rc < 0) + goto fail; + } + return 0; fail:
commit bd4a2697e5e2 ("sfc: use hardware tx timestamps for more than PTP") added support for hardware timestamping on TX for cards of the 8000 series and newer, in an effort to provide support for other transports other than IPv4/UDP. However, timestamping was still not working on RX for these other transports. This patch add support for PTP over IPv6/UDP. Tested: sync as master and as slave is correct using ptp4l from linuxptp package, both with IPv4 and IPv6. Suggested-by: Edward Cree <ecree.xilinx@gmail.com> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> --- drivers/net/ethernet/sfc/filter.h | 22 +++++++++++ drivers/net/ethernet/sfc/ptp.c | 63 ++++++++++++++++++++++++------- 2 files changed, 71 insertions(+), 14 deletions(-)