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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Mar 18, 2025 at 05:19:51AM +0200, Wei Fang wrote:
> 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.

Sorry, but I can only take the presented code at face value. If the
Coverity tool signals an issue, you can still triage it and explain why
it is a false positive. Or, if it is a real issue, you can add locking
and provide a good justification for it. But the justification is
missing now.

> 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?

That sounds good. I'm thinking, for each MAC address filter, maybe you
need an information whether it is programmed to hardware as an exact
match filter or a hash filter, and make use of that functionality here:
temporarily make all filters be hash-based ones, and then see how many
you can convert to exact matches. With something like this, it should
also be easier to maximize the use of the exact match table. Currently,
AFAIU, if you have 5 MAC address filters, they will all be stored as hash
filters, even if 4 of them could have been exact matches. Maybe that can
also be improved with such extra information.

> > > +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.

From the way in which the discussion is progressing in the replies
above, it sounds to me like maybe this logic will change a bit more.

> > > +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'?

Sounds better.

> > > +	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.

Ok. We need to find a way in which the code also makes sense today
(who knows how much time will pass until VSIs are also supported in the
mainline kernel - we all hope "as soon as possible" but have to plan for
the worst). I don't disagree with the existence of &pf->mac_list,
but it seems slightly inconsistent with the fact that you rebuild it
(for now, completely, but I understand that it the future it will be
just partially) each time ndo_set_rx_mode() is called.

Are you aware of __dev_uc_sync() / __dev_mc_sync()? They are helpers
with explicit sync/unsync callbacks per address, so you don't have to
manually walk using netdev_for_each_uc_addr() / netdev_for_each_mc_addr()
each time, and instead act only on the delta. I haven't thought this
suggestion through, but with you mentioning future VSI mailbox support
for MAC filtering, maybe it is helpful if the PSI's MAC filters are also
structured in this way.