RE: [PATCH v4 net-next 04/14] net: enetc: add MAC filter for i.MX95 ENETC PF

[Wei Fang]

> > +
> > +static void enetc4_pf_set_si_mac_hash_filter(struct enetc_hw *hw, int si,
> > +					     int type, u64 hash)
> > +{
> > +	if (type == UC) {
> > +		enetc_port_wr(hw, ENETC4_PSIUMHFR0(si), lower_32_bits(hash));
> > +		enetc_port_wr(hw, ENETC4_PSIUMHFR1(si), upper_32_bits(hash));
> > +	} else { /* MC */
> > +		enetc_port_wr(hw, ENETC4_PSIMMHFR0(si), lower_32_bits(hash));
> > +		enetc_port_wr(hw, ENETC4_PSIMMHFR1(si), upper_32_bits(hash));
> > +	}
> > +}
> 
> Please split into separate functions for unicast and for multicast.
> The implementations don't share any code, and the callers are not in
> common code either.
> 

Just copied from enetc_set_mac_ht_flt (), I can split into two separate
functions.

> > +
> > +static void enetc4_pf_destroy_mac_list(struct enetc_pf *pf)
> > +{
> > +	struct enetc_mac_list_entry *entry;
> > +	struct hlist_node *tmp;
> > +
> > +	mutex_lock(&pf->mac_list_lock);
> 
> The mutex_lock() usage here should raise serious questions. This is
> running right before mutex_destroy(). So if there were any concurrent
> attempt to acquire this lock, that concurrent code would have been broken
> any time it would have lost arbitration, by the fact that it would
> attempt to acquire a destroyed mutex.
> 
> But there's no such concurrent thread, because we run after
> destroy_workqueue()
> which flushes those concurrent calls and prevents new ones. So the mutex
> usage here is not necessary.
> 

You are right, but I'm afraid of the Coverity will report an issue, because the
pf->mac_list and pf->num_mfe are protected by the mac_list_lock in other
functions. And enetc4_pf_destroy_mac_list() will be called in other function
in the subsequent patches. I don't think it is unnecessary.

> [ same thing with mutex_init() immediately followed by mutex_lock().
>   It is an incorrect pattern most of the time. ]
> 
> > +
> > +	hlist_for_each_entry_safe(entry, tmp, &pf->mac_list, node) {
> > +		hlist_del(&entry->node);
> > +		kfree(entry);
> > +	}
> > +
> > +	pf->num_mfe = 0;
> > +
> > +	mutex_unlock(&pf->mac_list_lock);
> > +}
> > +
> > +static bool enetc_mac_filter_type_check(int type, const u8 *addr)
> > +{
> > +	if (type == ENETC_MAC_FILTER_TYPE_UC)
> > +		return !is_multicast_ether_addr(addr);
> > +	else if (type == ENETC_MAC_FILTER_TYPE_MC)
> > +		return is_multicast_ether_addr(addr);
> > +	else
> > +		return true;
> > +}
> > +
> > +static struct enetc_mac_list_entry *
> > +enetc_mac_list_lookup_entry(struct enetc_pf *pf, const unsigned char
> *addr)
> > +{
> > +	struct enetc_mac_list_entry *entry;
> > +
> > +	hlist_for_each_entry(entry, &pf->mac_list, node)
> > +		if (ether_addr_equal(entry->mac, addr))
> > +			return entry;
> > +
> > +	return NULL;
> > +}
> > +
> > +static void enetc_mac_list_add_entry(struct enetc_pf *pf,
> > +				     struct enetc_mac_list_entry *entry)
> > +{
> > +	hlist_add_head(&entry->node, &pf->mac_list);
> > +}
> > +
> > +static void enetc_mac_list_del_entry(struct enetc_mac_list_entry *entry)
> > +{
> > +	hlist_del(&entry->node);
> > +	kfree(entry);
> > +}
> > +
> > +static void enetc_mac_list_del_matched_entries(struct enetc_pf *pf, u16
> si_bit,
> > +					       struct enetc_mac_addr *mac,
> > +					       int mac_cnt)
> > +{
> > +	struct enetc_mac_list_entry *entry;
> > +	int i;
> > +
> > +	for (i = 0; i < mac_cnt; i++) {
> > +		entry = enetc_mac_list_lookup_entry(pf, mac[i].addr);
> > +		if (!entry)
> > +			continue;
> > +
> > +		entry->si_bitmap &= ~si_bit;
> > +		if (entry->si_bitmap)
> > +			continue;
> > +
> > +		enetc_mac_list_del_entry(entry);
> > +		pf->num_mfe--;
> > +	}
> > +}
> > +
> > +static bool enetc_mac_list_is_available(struct enetc_pf *pf,
> > +					struct enetc_mac_addr *mac,
> > +					int mac_cnt)
> > +{
> > +	int max_num_mfe = pf->caps.mac_filter_num;
> > +	struct enetc_mac_list_entry *entry;
> > +	int cur_num_mfe = pf->num_mfe;
> > +	int i, new_mac_cnt = 0;
> > +
> > +	if (mac_cnt > max_num_mfe)
> > +		return false;
> > +
> > +	/* Check MAC filter table whether has enough available entries */
> > +	hlist_for_each_entry(entry, &pf->mac_list, node) {
> > +		for (i = 0; i < mac_cnt; i++) {
> > +			if (ether_addr_equal(entry->mac, mac[i].addr))
> > +				break;
> > +		}
> > +
> > +		if (i == mac_cnt)
> > +			new_mac_cnt++;
> > +
> > +		if ((cur_num_mfe + new_mac_cnt) > max_num_mfe)
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static int enetc4_pf_add_si_mac_exact_filter(struct enetc_pf *pf, int si_id,
> > +					     struct enetc_mac_addr *mac,
> > +					     int mac_cnt)
> > +{
> > +	struct enetc_mac_list_entry *entry;
> > +	struct maft_entry_data data = {0};
> 
> As I've also learned, what you actually want is an empty struct initializer "= {}"
> here:
> https://lore.kernel.org/netdev/20210810091238.GB1343@xxxxxxxxxxxxxxxxxx.u
> k/
> 

Thanks for the info, I will improve all of this in the patch set.

> > +	struct enetc_si *si = pf->si;
> > +	u16 si_bit = BIT(si_id);
> > +	int i, num_mfe, err = 0;
> > +
> > +	mutex_lock(&pf->mac_list_lock);
> > +
> > +	if (!enetc_mac_list_is_available(pf, mac, mac_cnt)) {
> > +		err = -ENOSPC;
> > +		goto mac_list_unlock;
> > +	}
> > +
> > +	num_mfe = pf->num_mfe;
> > +	/* Update mac_list */
> > +	for (i = 0; i < mac_cnt; i++) {
> > +		entry = enetc_mac_list_lookup_entry(pf, mac[i].addr);
> > +		if (entry) {
> > +			entry->si_bitmap |= si_bit;
> > +			continue;
> > +		}
> > +
> > +		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > +		if (unlikely(!entry)) {
> > +			/* Restore MAC list to the state before the update
> > +			 * if an error occurs.
> > +			 */
> > +			enetc_mac_list_del_matched_entries(pf, si_bit,
> > +							   mac, i + 1);
> > +			err = -ENOMEM;
> > +			goto mac_list_unlock;
> > +		}
> > +
> > +		ether_addr_copy(entry->mac, mac[i].addr);
> > +		entry->si_bitmap = si_bit;
> > +		enetc_mac_list_add_entry(pf, entry);
> > +		pf->num_mfe++;
> > +	}
> > +
> > +	/* Clear MAC filter table */
> > +	for (i = 0; i < num_mfe; i++)
> > +		ntmp_maft_delete_entry(&si->ntmp.cbdrs, i);
> > +
> > +	i = 0;
> > +	hlist_for_each_entry(entry, &pf->mac_list, node) {
> > +		data.cfge.si_bitmap = cpu_to_le16(entry->si_bitmap);
> > +		ether_addr_copy(data.keye.mac_addr, entry->mac);
> > +		ntmp_maft_add_entry(&si->ntmp.cbdrs, i++, &data);
> 
> Don't discard error code.

Okay, I will add error check.
> 
> > +	}
> > +
> > +mac_list_unlock:
> > +	mutex_unlock(&pf->mac_list_lock);
> > +
> > +	return err;
> > +}
> > +
> > +static void enetc4_pf_flush_si_mac_exact_filter(struct enetc_pf *pf, int si_id,
> > +						int mac_type)
> > +{
> > +	struct enetc_mac_list_entry *entry;
> > +	struct maft_entry_data data = {0};
> 
> s/{0}/{}/
> 
> > +	struct enetc_si *si = pf->si;
> > +	u16 si_bit = BIT(si_id);
> > +	struct hlist_node *tmp;
> > +	int i, num_mfe;
> > +
> > +	mutex_lock(&pf->mac_list_lock);
> > +
> > +	num_mfe = pf->num_mfe;
> > +	hlist_for_each_entry_safe(entry, tmp, &pf->mac_list, node) {
> > +		if (enetc_mac_filter_type_check(mac_type, entry->mac) &&
> > +		    entry->si_bitmap & si_bit) {
> > +			entry->si_bitmap ^= si_bit;
> > +			if (entry->si_bitmap)
> > +				continue;
> > +
> > +			enetc_mac_list_del_entry(entry);
> > +			pf->num_mfe--;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < num_mfe; i++)
> > +		ntmp_maft_delete_entry(&si->ntmp.cbdrs, i);
> > +
> > +	i = 0;
> > +	hlist_for_each_entry(entry, &pf->mac_list, node) {
> > +		data.cfge.si_bitmap = cpu_to_le16(entry->si_bitmap);
> > +		ether_addr_copy(data.keye.mac_addr, entry->mac);
> > +		ntmp_maft_add_entry(&si->ntmp.cbdrs, i++, &data);
> 
> Don't discard error code.
> 
> Also, can't you edit MAFT entries in-place over NTMP? Deleting and
> re-adding filters creates small time windows where you might have
> RX packet loss, which is not ideal.

Actually MAFT does not support update action, if a MAC address has
been used by a SI, then another SI also wants to filter the same MAC
address, we only can delete the old entry and then add a new entry.
So there will be Rx packet loss as you said.

Second, the MAFT does not support key match, so we only access the
table through entry id. If we don't want to delete and re-add these
existing entries, then the driver needs to record the entry id of each
entry, this will make adding and removing entries more complicated.

So for your question about Rx packet loss, although it is a very corner
case, the solution I can think of is that we can use temporary MAC hash
filters before deleting MAFT entries and delete them after adding the
MAFT entries. Can you accept this proposal?

> 
> > +	}
> > +
> > +	mutex_unlock(&pf->mac_list_lock);
> > +}
> > +
> > +static int enetc4_pf_set_mac_exact_filter(struct enetc_pf *pf, int type)
> > +{
> > +	int max_num_mfe = pf->caps.mac_filter_num;
> > +	struct net_device *ndev = pf->si->ndev;
> > +	struct enetc_mac_addr *mac_tbl;
> > +	struct netdev_hw_addr *ha;
> > +	u8 si_mac[ETH_ALEN];
> > +	int mac_cnt = 0;
> > +	int err;
> > +
> > +	mac_tbl = kcalloc(max_num_mfe, sizeof(*mac_tbl), GFP_KERNEL);
> 
> Can't you know ahead of time, based on netdev_uc_count(), whether you
> will have space for exact match filters, and avoid unnecessary
> allocations if not? enetc_mac_list_is_available() seems way too late.
> 

I can add a check before alloc mac_tbl, but enetc_mac_list_is_available()
is still needed, because enetc4_pf_add_si_mac_exact_filter() is a common
function for all Sis, not only for PSI.

> > +	if (!mac_tbl)
> > +		return -ENOMEM;
> > +
> > +	enetc_get_primary_mac_addr(&pf->si->hw, si_mac);
> > +
> > +	netif_addr_lock_bh(ndev);
> > +	if (type & ENETC_MAC_FILTER_TYPE_UC) {
> > +		netdev_for_each_uc_addr(ha, ndev) {
> > +			if (!is_valid_ether_addr(ha->addr) ||
> > +			    ether_addr_equal(ha->addr, si_mac))
> > +				continue;
> > +
> > +			if (mac_cnt >= max_num_mfe)
> > +				goto err_nospace_out;
> > +
> > +			ether_addr_copy(mac_tbl[mac_cnt++].addr, ha->addr);
> > +		}
> > +	}
> > +
> > +	if (type & ENETC_MAC_FILTER_TYPE_MC) {
> 
> Dead code, you never add multicast addresses as exact match filters.
> Please remove.

Okay, I thought that we could support multicast exact filter for later
SoCs in the future with slight modification.

> 
> > +static void enetc4_pf_do_set_rx_mode(struct work_struct *work)
> > +{
> > +	struct enetc_si *si = container_of(work, struct enetc_si, rx_mode_task);
> > +	struct enetc_pf *pf = enetc_si_priv(si);
> > +	struct net_device *ndev = si->ndev;
> > +	struct enetc_hw *hw = &si->hw;
> > +	bool uc_promisc = false;
> > +	bool mc_promisc = false;
> > +	int type = 0;
> > +
> > +	if (ndev->flags & IFF_PROMISC) {
> > +		uc_promisc = true;
> > +		mc_promisc = true;
> > +	} else if (ndev->flags & IFF_ALLMULTI) {
> > +		mc_promisc = true;
> > +		type = ENETC_MAC_FILTER_TYPE_UC;
> > +	} else {
> > +		type = ENETC_MAC_FILTER_TYPE_ALL;
> > +	}
> > +
> > +	enetc4_pf_set_si_mac_promisc(hw, 0, UC, uc_promisc);
> > +	enetc4_pf_set_si_mac_promisc(hw, 0, MC, mc_promisc);
> 
> Why don't you call the function just once and provide both uc_promisc
> and mc_promisc arguments? You would avoid a useless read of the
> ENETC4_PSIPMMR register.
> 
Okay, I will refine the enetc4_pf_set_si_mac_promisc().

> > +static int enetc4_pf_wq_task_init(struct enetc_si *si)
> > +{
> > +	char wq_name[24];
> > +
> > +	INIT_WORK(&si->rx_mode_task, enetc4_pf_do_set_rx_mode);
> > +	snprintf(wq_name, sizeof(wq_name), "enetc-%s", pci_name(si->pdev));
> > +	si->workqueue = create_singlethread_workqueue(wq_name);
> > +	if (!si->workqueue)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> 
> Naming scheme is inconsistent here: the function is called "pf" but
> takes "si" as argument. Same for enetc4_pf_do_set_rx_mode() where the
> rx_mode_task is part of the station interface structure.
> 

So change 'pf' to 'psi'?

> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> > index 2b9d0f625f01..3b0cb0d8bf48 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> > @@ -22,6 +22,13 @@ struct enetc_port_caps {
> >  	int num_msix;
> >  	int num_rx_bdr;
> >  	int num_tx_bdr;
> > +	int mac_filter_num;
> > +};
> > +
> > +struct enetc_mac_list_entry {
> > +	u8 mac[ETH_ALEN];
> > +	u16 si_bitmap;
> > +	struct hlist_node node;
> >  };
> >
> >  struct enetc_pf;
> > @@ -57,6 +64,10 @@ struct enetc_pf {
> >
> >  	struct enetc_port_caps caps;
> >  	const struct enetc_pf_ops *ops;
> > +
> > +	struct hlist_head mac_list; /* MAC address filter table */
> 
> One thing I don't understand is why, given this implementation and
> final effect, you even bother to keep the mac_list persistently in
> struct enetc_pf. You have:
> 
> enetc4_pf_do_set_rx_mode()
> -> enetc4_pf_flush_si_mac_exact_filter(ENETC_MAC_FILTER_TYPE_ALL)
>    -> hlist_for_each_entry_safe(&pf->mac_list)
>       -> enetc_mac_list_del_entry()
> 
> which practically deletes all &pf->mac_list elements every time.
> So why even store them persistently in the first place? Why not just
> create an on-stack INIT_HLIST_HEAD() list?

The enetc4_pf_add_si_mac_exact_filter() and
enetc4_pf_add_si_mac_exact_filter() are used for all Sis, but only
PF can access MAFT, and PSI and VSIs may share the same MAFT
entry, so we need to store them in struct enetc_pf. Although we
have not added VFs support yet, for such shared functions, we
should design its implementation from the beginning, rather than
modifying them when we add the VFs support.

> 
> > +	struct mutex mac_list_lock; /* mac_list lock */
> 
> Unsatisfactory explanation. If you try to explain why it is needed, you
> will find it's not needed. That's the intention behind checkpatch
> emitting warnings when locks don't have comments. Not to force you to
> write blabla, but to force you to verbalize, and thus to think whether
> the proposed locking scheme makes sense.
> 
> The si->rx_mode_task is an ordered workqueue, which, as per
> include/linux/workqueue.h, "executes at most one work item at any given
> time in the queued order". In other words, enetc4_pf_do_set_rx_mode()
> doesn't race with itself.
> 
> Combined with the previous comment on enetc4_pf_destroy_mac_list(), I
> suggest that there is no need for this lock.
> 

I thought we could lay some foundation for VF's MAC filter support, so that
it would be easier to support VF later. However, judging from the current patch,
what you said is not unreasonable, we should pay more attention to whether
the current implementation is consistent with the current functions to be
supported. I will remove some parts related to VF, thank you.

> > +	int num_mfe;	/* number of mac address filter table entries */
> >  };
> >
> >  #define phylink_to_enetc_pf(config) \
> > --
> > 2.34.1
> >