| Message ID | 20230201080849.10482-5-ihuguet@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: support unicast PTP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | fail | Was 0 now: 1 |
On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñigo Huguet wrote: > Filters inserted to support unicast PTP mode might become unused after > some time, so we need to remove them to avoid accumulating many of them. > > Actually, it would be a very unusual situation that many different > addresses are used, normally only a small set of predefined > addresses are tried. Anyway, some cleanup is necessary because > maintaining old filters forever makes very little sense. > > Reported-by: Yalin Li <yalli@redhat.com> > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> > --- > drivers/net/ethernet/sfc/ptp.c | 121 +++++++++++++++++++++------------ > 1 file changed, 77 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c > index a3e827cd84a8..dd46ca6c070e 100644 > --- a/drivers/net/ethernet/sfc/ptp.c > +++ b/drivers/net/ethernet/sfc/ptp.c > @@ -75,6 +75,9 @@ > /* How long an unmatched event or packet can be held */ > #define PKT_EVENT_LIFETIME_MS 10 > > +/* How long unused unicast filters can be held */ > +#define UCAST_FILTER_EXPIRY_JIFFIES msecs_to_jiffies(30000) This seems like something that should be tunable, with this as a default value. > + > /* Offsets into PTP packet for identification. These offsets are from the > * start of the IP header, not the MAC header. Note that neither PTP V1 nor > * PTP V2 permit the use of IPV4 options. > @@ -220,6 +223,7 @@ struct efx_ptp_timeset { > * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6) > * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT) > * @loc_host: IPv4/v6 address of the filter > + * @expiry: time when the filter expires, in jiffies > * @handle: Handle ID for the MCDI filters table > */ > struct efx_ptp_rxfilter { > @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter { > __be16 ether_type; > __be16 loc_port; > __be32 loc_host[4]; > + unsigned long expiry; > int handle; > }; > > @@ -1320,8 +1325,8 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb) > local_bh_enable(); > } > > -static bool efx_ptp_filter_exists(struct list_head *ptp_list, > - struct efx_filter_spec *spec) > +static struct efx_ptp_rxfilter * > +efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec) > { > struct efx_ptp_rxfilter *rxfilter; > > @@ -1329,10 +1334,19 @@ static bool efx_ptp_filter_exists(struct list_head *ptp_list, > if (rxfilter->ether_type == spec->ether_type && > rxfilter->loc_port == spec->loc_port && > !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host))) > - return true; > + return rxfilter; > } > > - return false; > + return NULL; > +} > + > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx, > + struct efx_ptp_rxfilter *rxfilter) As others noted, don't use inline in .c files. > +{ > + efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, > + rxfilter->handle); > + list_del(&rxfilter->list); > + kfree(rxfilter); > } > > static void efx_ptp_remove_filters(struct efx_nic *efx, > @@ -1341,10 +1355,7 @@ static void efx_ptp_remove_filters(struct efx_nic *efx, > struct efx_ptp_rxfilter *rxfilter, *tmp; > > list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) { > - efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, > - rxfilter->handle); > - list_del(&rxfilter->list); > - kfree(rxfilter); > + efx_ptp_remove_one_filter(efx, rxfilter); > } > } > > @@ -1358,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx, > efx_rx_queue_index(queue)); > } > > -static int efx_ptp_insert_filter(struct efx_nic *efx, > - struct list_head *ptp_list, > - struct efx_filter_spec *spec) > +static struct efx_ptp_rxfilter * > +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list, > + struct efx_filter_spec *spec) This API change and the following ones are all for very little gain, they are just do set the new expiry attribute. And in the end the pointers all get converted back to an integer return code. In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter() and efx_ptp_insert_ipv6_filter(). > { > struct efx_ptp_rxfilter *rxfilter; > int rc; > > - if (efx_ptp_filter_exists(ptp_list, spec)) > - return 0; > + rxfilter = efx_ptp_find_filter(ptp_list, spec); > + if (rxfilter) > + return rxfilter; > > rc = efx_filter_insert_filter(efx, spec, true); > if (rc < 0) > - return rc; > + return ERR_PTR(rc); > > rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL); > if (!rxfilter) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > rxfilter->handle = rc; > rxfilter->ether_type = spec->ether_type; > @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx, > memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)); > list_add(&rxfilter->list, ptp_list); > > - return 0; > + return rxfilter; > } > > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, > - struct list_head *ptp_list, > - __be32 addr, u16 port) > +static struct efx_ptp_rxfilter * > +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list, > + __be32 addr, u16 port) > { > struct efx_filter_spec spec; > > @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, > return efx_ptp_insert_filter(efx, ptp_list, &spec); > } > > -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, > - struct list_head *ptp_list, > - struct in6_addr *addr, u16 port) > +static struct efx_ptp_rxfilter * > +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list, > + struct in6_addr *addr, u16 port) > { > struct efx_filter_spec spec; > > @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, > return efx_ptp_insert_filter(efx, ptp_list, &spec); > } > > -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > +static struct efx_ptp_rxfilter * > +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > { > const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER; > struct efx_filter_spec spec; > @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > { > struct efx_ptp_data *ptp = efx->ptp_data; > - int rc; > + struct efx_ptp_rxfilter *rc; > > if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast)) > return 0; > @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > */ > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, > htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT); > - if (rc < 0) > + if (IS_ERR(rc)) > goto fail; > > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, > htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT); > - if (rc < 0) > + if (IS_ERR(rc)) > goto fail; > > /* if the NIC supports hw timestamps by the MAC, we can support > @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, > &ipv6_addr, PTP_EVENT_PORT); > - if (rc < 0) > + if (IS_ERR(rc)) > goto fail; > > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, > &ipv6_addr, PTP_GENERAL_PORT); > - if (rc < 0) > + if (IS_ERR(rc)) > goto fail; > > rc = efx_ptp_insert_eth_multicast_filter(efx); > - if (rc < 0) > + if (IS_ERR(rc)) > goto fail; > } > > @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > fail: > efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast); > - return rc; > + return PTR_ERR(rc); > } > > static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb) > @@ -1484,7 +1497,7 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > struct sk_buff *skb) > { > struct efx_ptp_data *ptp = efx->ptp_data; > - int rc; > + struct efx_ptp_rxfilter *rxfilter; > > if (!efx_ptp_valid_unicast_event_pkt(skb)) > return -EINVAL; > @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > if (skb->protocol == htons(ETH_P_IP)) { > __be32 addr = ip_hdr(skb)->saddr; > > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > - addr, PTP_EVENT_PORT); > - if (rc < 0) > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > + addr, PTP_EVENT_PORT); > + if (IS_ERR(rxfilter)) > goto fail; > > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > - addr, PTP_GENERAL_PORT); > - if (rc < 0) > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > + > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > + addr, PTP_GENERAL_PORT); > + if (IS_ERR(rxfilter)) > goto fail; > + > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > } else if (efx_ptp_use_mac_tx_timestamps(efx)) { > /* IPv6 PTP only supported by devices with MAC hw timestamp */ > struct in6_addr *addr = &ipv6_hdr(skb)->saddr; > > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > - addr, PTP_EVENT_PORT); > - if (rc < 0) > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > + addr, PTP_EVENT_PORT); > + if (IS_ERR(rxfilter)) > goto fail; > > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > - addr, PTP_GENERAL_PORT); > - if (rc < 0) > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > + > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > + addr, PTP_GENERAL_PORT); > + if (IS_ERR(rxfilter)) > goto fail; > + > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > } else { > return -EOPNOTSUPP; > } > @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > fail: > efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast); > - return rc; > + return PTR_ERR(rxfilter); > +} > + > +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx) > +{ > + struct efx_ptp_data *ptp = efx->ptp_data; > + struct efx_ptp_rxfilter *rxfilter, *tmp; > + > + list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) { > + if (time_is_before_jiffies(rxfilter->expiry)) Shouldn't this be time_is_after_jiffies? > + efx_ptp_remove_one_filter(efx, rxfilter); > + } > } > > static int efx_ptp_start(struct efx_nic *efx) > @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work) > } > > efx_ptp_drop_time_expired_events(efx); > + efx_ptp_drop_expired_unicast_filters(efx); This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because that will delay processing that is done below. So do this at the end of the function. Martin > > __skb_queue_head_init(&tempq); > efx_ptp_process_events(efx, &tempq); > -- > 2.34.3
On Thu, Feb 2, 2023 at 3:12 PM Martin Habets <habetsm.xilinx@gmail.com> wrote: > > On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñigo Huguet wrote: > > Filters inserted to support unicast PTP mode might become unused after > > some time, so we need to remove them to avoid accumulating many of them. > > > > Actually, it would be a very unusual situation that many different > > addresses are used, normally only a small set of predefined > > addresses are tried. Anyway, some cleanup is necessary because > > maintaining old filters forever makes very little sense. > > > > Reported-by: Yalin Li <yalli@redhat.com> > > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> > > --- > > drivers/net/ethernet/sfc/ptp.c | 121 +++++++++++++++++++++------------ > > 1 file changed, 77 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c > > index a3e827cd84a8..dd46ca6c070e 100644 > > --- a/drivers/net/ethernet/sfc/ptp.c > > +++ b/drivers/net/ethernet/sfc/ptp.c > > @@ -75,6 +75,9 @@ > > /* How long an unmatched event or packet can be held */ > > #define PKT_EVENT_LIFETIME_MS 10 > > > > +/* How long unused unicast filters can be held */ > > +#define UCAST_FILTER_EXPIRY_JIFFIES msecs_to_jiffies(30000) > > This seems like something that should be tunable, with this as a > default value. > > > + > > /* Offsets into PTP packet for identification. These offsets are from the > > * start of the IP header, not the MAC header. Note that neither PTP V1 nor > > * PTP V2 permit the use of IPV4 options. > > @@ -220,6 +223,7 @@ struct efx_ptp_timeset { > > * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6) > > * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT) > > * @loc_host: IPv4/v6 address of the filter > > + * @expiry: time when the filter expires, in jiffies > > * @handle: Handle ID for the MCDI filters table > > */ > > struct efx_ptp_rxfilter { > > @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter { > > __be16 ether_type; > > __be16 loc_port; > > __be32 loc_host[4]; > > + unsigned long expiry; > > int handle; > > }; > > > > @@ -1320,8 +1325,8 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb) > > local_bh_enable(); > > } > > > > -static bool efx_ptp_filter_exists(struct list_head *ptp_list, > > - struct efx_filter_spec *spec) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec) > > { > > struct efx_ptp_rxfilter *rxfilter; > > > > @@ -1329,10 +1334,19 @@ static bool efx_ptp_filter_exists(struct list_head *ptp_list, > > if (rxfilter->ether_type == spec->ether_type && > > rxfilter->loc_port == spec->loc_port && > > !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host))) > > - return true; > > + return rxfilter; > > } > > > > - return false; > > + return NULL; > > +} > > + > > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx, > > + struct efx_ptp_rxfilter *rxfilter) > > As others noted, don't use inline in .c files. > > > +{ > > + efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, > > + rxfilter->handle); > > + list_del(&rxfilter->list); > > + kfree(rxfilter); > > } > > > > static void efx_ptp_remove_filters(struct efx_nic *efx, > > @@ -1341,10 +1355,7 @@ static void efx_ptp_remove_filters(struct efx_nic *efx, > > struct efx_ptp_rxfilter *rxfilter, *tmp; > > > > list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) { > > - efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, > > - rxfilter->handle); > > - list_del(&rxfilter->list); > > - kfree(rxfilter); > > + efx_ptp_remove_one_filter(efx, rxfilter); > > } > > } > > > > @@ -1358,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx, > > efx_rx_queue_index(queue)); > > } > > > > -static int efx_ptp_insert_filter(struct efx_nic *efx, > > - struct list_head *ptp_list, > > - struct efx_filter_spec *spec) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list, > > + struct efx_filter_spec *spec) > > This API change and the following ones are all for very little gain, > they are just do set the new expiry attribute. And in the end the > pointers all get converted back to an integer return code. > In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter() > and efx_ptp_insert_ipv6_filter(). I wanted to save computation time when inserting multicast filters, but given that it's done only once, it's a bit pointless. I also wanted to show clearly in the code that only unicast filters care about expiration, but maybe just passing a constant 0 to your expiry argument clearly shows that multicast doesn't care about it. I also had the feeling that I was overcomplicating the API for little gain, I like your approach more. Also, reviewing this has made me to realize that I'm inserting pointless unicast filters when using multicast PTP. Probably I should add a check `ip_hdr(skb)->daddr != htnonl(PTP_ADDR_IPV4)` to efx_ptp_valid_unicast_event_pkt (and the equivalent for IPv6). > > > { > > struct efx_ptp_rxfilter *rxfilter; > > int rc; > > > > - if (efx_ptp_filter_exists(ptp_list, spec)) > > - return 0; > > + rxfilter = efx_ptp_find_filter(ptp_list, spec); > > + if (rxfilter) > > + return rxfilter; > > > > rc = efx_filter_insert_filter(efx, spec, true); > > if (rc < 0) > > - return rc; > > + return ERR_PTR(rc); > > > > rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL); > > if (!rxfilter) > > - return -ENOMEM; > > + return ERR_PTR(-ENOMEM); > > > > rxfilter->handle = rc; > > rxfilter->ether_type = spec->ether_type; > > @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx, > > memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)); > > list_add(&rxfilter->list, ptp_list); > > > > - return 0; > > + return rxfilter; > > } > > > > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, > > - struct list_head *ptp_list, > > - __be32 addr, u16 port) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list, > > + __be32 addr, u16 port) > > { > > struct efx_filter_spec spec; > > > > @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, > > return efx_ptp_insert_filter(efx, ptp_list, &spec); > > } > > > > -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, > > - struct list_head *ptp_list, > > - struct in6_addr *addr, u16 port) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list, > > + struct in6_addr *addr, u16 port) > > { > > struct efx_filter_spec spec; > > > > @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, > > return efx_ptp_insert_filter(efx, ptp_list, &spec); > > } > > > > -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > > { > > const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER; > > struct efx_filter_spec spec; > > @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > > static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > { > > struct efx_ptp_data *ptp = efx->ptp_data; > > - int rc; > > + struct efx_ptp_rxfilter *rc; > > > > if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast)) > > return 0; > > @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > */ > > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, > > htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, > > htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > /* if the NIC supports hw timestamps by the MAC, we can support > > @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > > > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, > > &ipv6_addr, PTP_EVENT_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, > > &ipv6_addr, PTP_GENERAL_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > rc = efx_ptp_insert_eth_multicast_filter(efx); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > } > > > > @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > > > fail: > > efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast); > > - return rc; > > + return PTR_ERR(rc); > > } > > > > static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb) > > @@ -1484,7 +1497,7 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > struct sk_buff *skb) > > { > > struct efx_ptp_data *ptp = efx->ptp_data; > > - int rc; > > + struct efx_ptp_rxfilter *rxfilter; > > > > if (!efx_ptp_valid_unicast_event_pkt(skb)) > > return -EINVAL; > > @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > if (skb->protocol == htons(ETH_P_IP)) { > > __be32 addr = ip_hdr(skb)->saddr; > > > > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_EVENT_PORT); > > - if (rc < 0) > > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_EVENT_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > > > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_GENERAL_PORT); > > - if (rc < 0) > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > + > > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_GENERAL_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > + > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > } else if (efx_ptp_use_mac_tx_timestamps(efx)) { > > /* IPv6 PTP only supported by devices with MAC hw timestamp */ > > struct in6_addr *addr = &ipv6_hdr(skb)->saddr; > > > > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_EVENT_PORT); > > - if (rc < 0) > > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_EVENT_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > > > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_GENERAL_PORT); > > - if (rc < 0) > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > + > > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_GENERAL_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > + > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > } else { > > return -EOPNOTSUPP; > > } > > @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > > > fail: > > efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast); > > - return rc; > > + return PTR_ERR(rxfilter); > > +} > > + > > +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx) > > +{ > > + struct efx_ptp_data *ptp = efx->ptp_data; > > + struct efx_ptp_rxfilter *rxfilter, *tmp; > > + > > + list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) { > > + if (time_is_before_jiffies(rxfilter->expiry)) > > Shouldn't this be time_is_after_jiffies? > > > + efx_ptp_remove_one_filter(efx, rxfilter); > > + } > > } > > > > static int efx_ptp_start(struct efx_nic *efx) > > @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work) > > } > > > > efx_ptp_drop_time_expired_events(efx); > > + efx_ptp_drop_expired_unicast_filters(efx); > > This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because > that will delay processing that is done below. So do this at the end of > the function. > > Martin > > > > > __skb_queue_head_init(&tempq); > > efx_ptp_process_events(efx, &tempq); > > -- > > 2.34.3 >
On Thu, Feb 2, 2023 at 3:12 PM Martin Habets <habetsm.xilinx@gmail.com> wrote: > > On Wed, Feb 01, 2023 at 09:08:49AM +0100, Íñigo Huguet wrote: > > Filters inserted to support unicast PTP mode might become unused after > > some time, so we need to remove them to avoid accumulating many of them. > > > > Actually, it would be a very unusual situation that many different > > addresses are used, normally only a small set of predefined > > addresses are tried. Anyway, some cleanup is necessary because > > maintaining old filters forever makes very little sense. > > > > Reported-by: Yalin Li <yalli@redhat.com> > > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> > > --- > > drivers/net/ethernet/sfc/ptp.c | 121 +++++++++++++++++++++------------ > > 1 file changed, 77 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c > > index a3e827cd84a8..dd46ca6c070e 100644 > > --- a/drivers/net/ethernet/sfc/ptp.c > > +++ b/drivers/net/ethernet/sfc/ptp.c > > @@ -75,6 +75,9 @@ > > /* How long an unmatched event or packet can be held */ > > #define PKT_EVENT_LIFETIME_MS 10 > > > > +/* How long unused unicast filters can be held */ > > +#define UCAST_FILTER_EXPIRY_JIFFIES msecs_to_jiffies(30000) > > This seems like something that should be tunable, with this as a > default value. Not sure how valuable it would be. The filter is not going to expire while it's in use, and it will expire 30s (or whatever) after last usage. If it expires, if used again it's reinserted, with only the penalty of a MCDI call. I wouldn't make it tunable, but if you insist, how would you do it? modparam? Other? > > + > > /* Offsets into PTP packet for identification. These offsets are from the > > * start of the IP header, not the MAC header. Note that neither PTP V1 nor > > * PTP V2 permit the use of IPV4 options. > > @@ -220,6 +223,7 @@ struct efx_ptp_timeset { > > * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6) > > * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT) > > * @loc_host: IPv4/v6 address of the filter > > + * @expiry: time when the filter expires, in jiffies > > * @handle: Handle ID for the MCDI filters table > > */ > > struct efx_ptp_rxfilter { > > @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter { > > __be16 ether_type; > > __be16 loc_port; > > __be32 loc_host[4]; > > + unsigned long expiry; > > int handle; > > }; > > > > @@ -1320,8 +1325,8 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb) > > local_bh_enable(); > > } > > > > -static bool efx_ptp_filter_exists(struct list_head *ptp_list, > > - struct efx_filter_spec *spec) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec) > > { > > struct efx_ptp_rxfilter *rxfilter; > > > > @@ -1329,10 +1334,19 @@ static bool efx_ptp_filter_exists(struct list_head *ptp_list, > > if (rxfilter->ether_type == spec->ether_type && > > rxfilter->loc_port == spec->loc_port && > > !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host))) > > - return true; > > + return rxfilter; > > } > > > > - return false; > > + return NULL; > > +} > > + > > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx, > > + struct efx_ptp_rxfilter *rxfilter) > > As others noted, don't use inline in .c files. > > > +{ > > + efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, > > + rxfilter->handle); > > + list_del(&rxfilter->list); > > + kfree(rxfilter); > > } > > > > static void efx_ptp_remove_filters(struct efx_nic *efx, > > @@ -1341,10 +1355,7 @@ static void efx_ptp_remove_filters(struct efx_nic *efx, > > struct efx_ptp_rxfilter *rxfilter, *tmp; > > > > list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) { > > - efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, > > - rxfilter->handle); > > - list_del(&rxfilter->list); > > - kfree(rxfilter); > > + efx_ptp_remove_one_filter(efx, rxfilter); > > } > > } > > > > @@ -1358,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx, > > efx_rx_queue_index(queue)); > > } > > > > -static int efx_ptp_insert_filter(struct efx_nic *efx, > > - struct list_head *ptp_list, > > - struct efx_filter_spec *spec) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list, > > + struct efx_filter_spec *spec) > > This API change and the following ones are all for very little gain, > they are just do set the new expiry attribute. And in the end the > pointers all get converted back to an integer return code. > In stead, just pass expiry as a new argument to efx_ptp_insert_ipv4_filter() > and efx_ptp_insert_ipv6_filter(). > > > { > > struct efx_ptp_rxfilter *rxfilter; > > int rc; > > > > - if (efx_ptp_filter_exists(ptp_list, spec)) > > - return 0; > > + rxfilter = efx_ptp_find_filter(ptp_list, spec); > > + if (rxfilter) > > + return rxfilter; > > > > rc = efx_filter_insert_filter(efx, spec, true); > > if (rc < 0) > > - return rc; > > + return ERR_PTR(rc); > > > > rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL); > > if (!rxfilter) > > - return -ENOMEM; > > + return ERR_PTR(-ENOMEM); > > > > rxfilter->handle = rc; > > rxfilter->ether_type = spec->ether_type; > > @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx, > > memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)); > > list_add(&rxfilter->list, ptp_list); > > > > - return 0; > > + return rxfilter; > > } > > > > -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, > > - struct list_head *ptp_list, > > - __be32 addr, u16 port) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list, > > + __be32 addr, u16 port) > > { > > struct efx_filter_spec spec; > > > > @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, > > return efx_ptp_insert_filter(efx, ptp_list, &spec); > > } > > > > -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, > > - struct list_head *ptp_list, > > - struct in6_addr *addr, u16 port) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list, > > + struct in6_addr *addr, u16 port) > > { > > struct efx_filter_spec spec; > > > > @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, > > return efx_ptp_insert_filter(efx, ptp_list, &spec); > > } > > > > -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > > +static struct efx_ptp_rxfilter * > > +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > > { > > const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER; > > struct efx_filter_spec spec; > > @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) > > static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > { > > struct efx_ptp_data *ptp = efx->ptp_data; > > - int rc; > > + struct efx_ptp_rxfilter *rc; > > > > if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast)) > > return 0; > > @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > */ > > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, > > htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, > > htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > /* if the NIC supports hw timestamps by the MAC, we can support > > @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > > > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, > > &ipv6_addr, PTP_EVENT_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, > > &ipv6_addr, PTP_GENERAL_PORT); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > > > rc = efx_ptp_insert_eth_multicast_filter(efx); > > - if (rc < 0) > > + if (IS_ERR(rc)) > > goto fail; > > } > > > > @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) > > > > fail: > > efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast); > > - return rc; > > + return PTR_ERR(rc); > > } > > > > static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb) > > @@ -1484,7 +1497,7 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > struct sk_buff *skb) > > { > > struct efx_ptp_data *ptp = efx->ptp_data; > > - int rc; > > + struct efx_ptp_rxfilter *rxfilter; > > > > if (!efx_ptp_valid_unicast_event_pkt(skb)) > > return -EINVAL; > > @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > if (skb->protocol == htons(ETH_P_IP)) { > > __be32 addr = ip_hdr(skb)->saddr; > > > > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_EVENT_PORT); > > - if (rc < 0) > > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_EVENT_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > > > - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_GENERAL_PORT); > > - if (rc < 0) > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > + > > + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_GENERAL_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > + > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > } else if (efx_ptp_use_mac_tx_timestamps(efx)) { > > /* IPv6 PTP only supported by devices with MAC hw timestamp */ > > struct in6_addr *addr = &ipv6_hdr(skb)->saddr; > > > > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_EVENT_PORT); > > - if (rc < 0) > > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_EVENT_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > > > - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > - addr, PTP_GENERAL_PORT); > > - if (rc < 0) > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > + > > + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, > > + addr, PTP_GENERAL_PORT); > > + if (IS_ERR(rxfilter)) > > goto fail; > > + > > + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; > > } else { > > return -EOPNOTSUPP; > > } > > @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, > > > > fail: > > efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast); > > - return rc; > > + return PTR_ERR(rxfilter); > > +} > > + > > +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx) > > +{ > > + struct efx_ptp_data *ptp = efx->ptp_data; > > + struct efx_ptp_rxfilter *rxfilter, *tmp; > > + > > + list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) { > > + if (time_is_before_jiffies(rxfilter->expiry)) > > Shouldn't this be time_is_after_jiffies? No, if time_is_before_jiffies means that jiffies are higher than the value of expiry, so filter needs to be removed. > > > + efx_ptp_remove_one_filter(efx, rxfilter); > > + } > > } > > > > static int efx_ptp_start(struct efx_nic *efx) > > @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work) > > } > > > > efx_ptp_drop_time_expired_events(efx); > > + efx_ptp_drop_expired_unicast_filters(efx); > > This grabs locks in efx_mcdi_filter_remove_safe(), which is bad because > that will delay processing that is done below. So do this at the end of > the function. > > Martin > > > > > __skb_queue_head_init(&tempq); > > efx_ptp_process_events(efx, &tempq); > > -- > > 2.34.3 >
diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c index a3e827cd84a8..dd46ca6c070e 100644 --- a/drivers/net/ethernet/sfc/ptp.c +++ b/drivers/net/ethernet/sfc/ptp.c @@ -75,6 +75,9 @@ /* How long an unmatched event or packet can be held */ #define PKT_EVENT_LIFETIME_MS 10 +/* How long unused unicast filters can be held */ +#define UCAST_FILTER_EXPIRY_JIFFIES msecs_to_jiffies(30000) + /* Offsets into PTP packet for identification. These offsets are from the * start of the IP header, not the MAC header. Note that neither PTP V1 nor * PTP V2 permit the use of IPV4 options. @@ -220,6 +223,7 @@ struct efx_ptp_timeset { * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6) * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT) * @loc_host: IPv4/v6 address of the filter + * @expiry: time when the filter expires, in jiffies * @handle: Handle ID for the MCDI filters table */ struct efx_ptp_rxfilter { @@ -227,6 +231,7 @@ struct efx_ptp_rxfilter { __be16 ether_type; __be16 loc_port; __be32 loc_host[4]; + unsigned long expiry; int handle; }; @@ -1320,8 +1325,8 @@ static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb) local_bh_enable(); } -static bool efx_ptp_filter_exists(struct list_head *ptp_list, - struct efx_filter_spec *spec) +static struct efx_ptp_rxfilter * +efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec) { struct efx_ptp_rxfilter *rxfilter; @@ -1329,10 +1334,19 @@ static bool efx_ptp_filter_exists(struct list_head *ptp_list, if (rxfilter->ether_type == spec->ether_type && rxfilter->loc_port == spec->loc_port && !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host))) - return true; + return rxfilter; } - return false; + return NULL; +} + +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx, + struct efx_ptp_rxfilter *rxfilter) +{ + efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, + rxfilter->handle); + list_del(&rxfilter->list); + kfree(rxfilter); } static void efx_ptp_remove_filters(struct efx_nic *efx, @@ -1341,10 +1355,7 @@ static void efx_ptp_remove_filters(struct efx_nic *efx, struct efx_ptp_rxfilter *rxfilter, *tmp; list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) { - efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED, - rxfilter->handle); - list_del(&rxfilter->list); - kfree(rxfilter); + efx_ptp_remove_one_filter(efx, rxfilter); } } @@ -1358,23 +1369,24 @@ static void efx_ptp_init_filter(struct efx_nic *efx, efx_rx_queue_index(queue)); } -static int efx_ptp_insert_filter(struct efx_nic *efx, - struct list_head *ptp_list, - struct efx_filter_spec *spec) +static struct efx_ptp_rxfilter * +efx_ptp_insert_filter(struct efx_nic *efx, struct list_head *ptp_list, + struct efx_filter_spec *spec) { struct efx_ptp_rxfilter *rxfilter; int rc; - if (efx_ptp_filter_exists(ptp_list, spec)) - return 0; + rxfilter = efx_ptp_find_filter(ptp_list, spec); + if (rxfilter) + return rxfilter; rc = efx_filter_insert_filter(efx, spec, true); if (rc < 0) - return rc; + return ERR_PTR(rc); rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL); if (!rxfilter) - return -ENOMEM; + return ERR_PTR(-ENOMEM); rxfilter->handle = rc; rxfilter->ether_type = spec->ether_type; @@ -1382,12 +1394,12 @@ static int efx_ptp_insert_filter(struct efx_nic *efx, memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)); list_add(&rxfilter->list, ptp_list); - return 0; + return rxfilter; } -static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, - struct list_head *ptp_list, - __be32 addr, u16 port) +static struct efx_ptp_rxfilter * +efx_ptp_insert_ipv4_filter(struct efx_nic *efx, struct list_head *ptp_list, + __be32 addr, u16 port) { struct efx_filter_spec spec; @@ -1396,9 +1408,9 @@ static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx, return efx_ptp_insert_filter(efx, ptp_list, &spec); } -static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, - struct list_head *ptp_list, - struct in6_addr *addr, u16 port) +static struct efx_ptp_rxfilter * +efx_ptp_insert_ipv6_filter(struct efx_nic *efx, struct list_head *ptp_list, + struct in6_addr *addr, u16 port) { struct efx_filter_spec spec; @@ -1407,7 +1419,8 @@ static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx, return efx_ptp_insert_filter(efx, ptp_list, &spec); } -static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) +static struct efx_ptp_rxfilter * +efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) { const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER; struct efx_filter_spec spec; @@ -1422,7 +1435,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx) static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) { struct efx_ptp_data *ptp = efx->ptp_data; - int rc; + struct efx_ptp_rxfilter *rc; if (!ptp->channel || !list_empty(&ptp->rxfilters_mcast)) return 0; @@ -1432,12 +1445,12 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) */ rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT); - if (rc < 0) + if (IS_ERR(rc)) goto fail; rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast, htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT); - if (rc < 0) + if (IS_ERR(rc)) goto fail; /* if the NIC supports hw timestamps by the MAC, we can support @@ -1448,16 +1461,16 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, &ipv6_addr, PTP_EVENT_PORT); - if (rc < 0) + if (IS_ERR(rc)) goto fail; rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast, &ipv6_addr, PTP_GENERAL_PORT); - if (rc < 0) + if (IS_ERR(rc)) goto fail; rc = efx_ptp_insert_eth_multicast_filter(efx); - if (rc < 0) + if (IS_ERR(rc)) goto fail; } @@ -1465,7 +1478,7 @@ static int efx_ptp_insert_multicast_filters(struct efx_nic *efx) fail: efx_ptp_remove_filters(efx, &ptp->rxfilters_mcast); - return rc; + return PTR_ERR(rc); } static bool efx_ptp_valid_unicast_event_pkt(struct sk_buff *skb) @@ -1484,7 +1497,7 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, struct sk_buff *skb) { struct efx_ptp_data *ptp = efx->ptp_data; - int rc; + struct efx_ptp_rxfilter *rxfilter; if (!efx_ptp_valid_unicast_event_pkt(skb)) return -EINVAL; @@ -1492,28 +1505,36 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, if (skb->protocol == htons(ETH_P_IP)) { __be32 addr = ip_hdr(skb)->saddr; - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, - addr, PTP_EVENT_PORT); - if (rc < 0) + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, + addr, PTP_EVENT_PORT); + if (IS_ERR(rxfilter)) goto fail; - rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, - addr, PTP_GENERAL_PORT); - if (rc < 0) + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; + + rxfilter = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast, + addr, PTP_GENERAL_PORT); + if (IS_ERR(rxfilter)) goto fail; + + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; } else if (efx_ptp_use_mac_tx_timestamps(efx)) { /* IPv6 PTP only supported by devices with MAC hw timestamp */ struct in6_addr *addr = &ipv6_hdr(skb)->saddr; - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, - addr, PTP_EVENT_PORT); - if (rc < 0) + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, + addr, PTP_EVENT_PORT); + if (IS_ERR(rxfilter)) goto fail; - rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, - addr, PTP_GENERAL_PORT); - if (rc < 0) + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; + + rxfilter = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast, + addr, PTP_GENERAL_PORT); + if (IS_ERR(rxfilter)) goto fail; + + rxfilter->expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES; } else { return -EOPNOTSUPP; } @@ -1522,7 +1543,18 @@ static int efx_ptp_insert_unicast_filter(struct efx_nic *efx, fail: efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast); - return rc; + return PTR_ERR(rxfilter); +} + +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx) +{ + struct efx_ptp_data *ptp = efx->ptp_data; + struct efx_ptp_rxfilter *rxfilter, *tmp; + + list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) { + if (time_is_before_jiffies(rxfilter->expiry)) + efx_ptp_remove_one_filter(efx, rxfilter); + } } static int efx_ptp_start(struct efx_nic *efx) @@ -1616,6 +1648,7 @@ static void efx_ptp_worker(struct work_struct *work) } efx_ptp_drop_time_expired_events(efx); + efx_ptp_drop_expired_unicast_filters(efx); __skb_queue_head_init(&tempq); efx_ptp_process_events(efx, &tempq);
Filters inserted to support unicast PTP mode might become unused after some time, so we need to remove them to avoid accumulating many of them. Actually, it would be a very unusual situation that many different addresses are used, normally only a small set of predefined addresses are tried. Anyway, some cleanup is necessary because maintaining old filters forever makes very little sense. Reported-by: Yalin Li <yalli@redhat.com> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> --- drivers/net/ethernet/sfc/ptp.c | 121 +++++++++++++++++++++------------ 1 file changed, 77 insertions(+), 44 deletions(-)