Re: [PATCH RESEND net-next v4 3/4] sfc: support unicast PTP

["Íñigo Huguet" <ihuguet@redhat.com>]

El 14/3/23 a las 17:55, Edward Cree escribió:
> On 14/03/2023 10:09, Íñigo Huguet wrote:
>> When sending a PTP event packet, add the correct filters that will make
>> that future incoming unicast PTP event packets will be timestamped.
>> The unicast address for the filter is gotten from the outgoing skb
>> before sending it.
>>
>> Until now they were not timestamped because only filters that match with
>> the PTP multicast addressed were being configured into the NIC for the
>> PTP special channel. Packets received through different channels are not
>> timestamped, getting "received SYNC without timestamp" error in ptp4l.
>>
>> Note that the inserted filters are never removed unless the NIC is stopped
>> or reconfigured, so efx_ptp_stop is called. Removal of old filters will
>> be handled by the next patch.
>>
>> Additionally, cleanup a bit efx_ptp_xmit_skb_mc to use the reverse xmas
>> tree convention and remove an unnecessary assignment to rc variable in
>> void function.
>>
>> Reported-by: Yalin Li <yalli@redhat.com>
>> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> 
> Few nits below, but still
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> 
>> +static bool efx_ptp_filter_exists(struct list_head *ptp_list,
>> +				  struct efx_filter_spec *spec)
>> +{
>> +	struct efx_ptp_rxfilter *rxfilter;
>> +
>> +	list_for_each_entry(rxfilter, ptp_list, 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 false;
>> +}
> 
> Technically this could be more efficient if we used an rhashtable
>  instead of a list, but I guess we don't expect the list to grow
>  very long.

Yes, I expect the list to be short, so I thought better not to optimize prematurely. However, after giving to it a second thought, maybe the list can grow more if the NIC is acting as unicast PTP master? I mainly considered the slave case...

I've never used rhashlist, it would be a good learning exercise. How do you see if I submit that as an optimization in a future patch?
 
>> +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;
>> +
>> +	if (!efx_ptp_valid_unicast_event_pkt(skb))
>> +		return -EINVAL;
>> +
>> +	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)
>> +			goto fail;
>> +
>> +		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
>> +						addr, PTP_GENERAL_PORT);
>> +		if (rc < 0)
>> +			goto fail;
>> +	} 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)
>> +			goto fail;
>> +
>> +		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
>> +						addr, PTP_GENERAL_PORT);
>> +		if (rc < 0)
>> +			goto fail;
>> +	} else {
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>> +
>> +fail:
>> +	efx_ptp_remove_filters(efx, &ptp->rxfilters_ucast);
>> +	return rc;
>> +}
> 
> Why does failing to insert one filter mean we need to remove *all*
>  the unicast filters we have?  (I'm not even sure it's necessary
>  to remove the new EVENT filter if the GENERAL filter fails.)

Well, my reasoning was that it shouldn't fail in a first place. If it does, it's something very weird. Instead of implementing more complex logic to try checking if the current state is valid or not, just remove all and try to install them again the next time. If it fails again, probably the system is not in a very good state.