Re: [PATCH net-next 3/7] net: hibmcge: Add unicast frame filter supported in this module

[Jijie Shao]

on 2024/10/24 20:07, Andrew Lunn wrote:
> On Thu, Oct 24, 2024 at 11:09:22AM +0800, Jijie Shao wrote:
> > on 2024/10/23 22:05, Andrew Lunn wrote:
> > > > +static int hbg_add_mac_to_filter(struct hbg_priv *priv, const u8 *addr)
> > > > +{
> > > > +	u32 index;
> > > > +
> > > > +	/* already exists */
> > > > +	if (!hbg_get_index_from_mac_table(priv, addr, &index))
> > > > +		return 0;
> > > > +
> > > > +	for (index = 0; index < priv->filter.table_max_len; index++)
> > > > +		if (is_zero_ether_addr(priv->filter.mac_table[index].addr)) {
> > > > +			hbg_set_mac_to_mac_table(priv, index, addr);
> > > > +			return 0;
> > > > +		}
> > > > +
> > > > +	if (!priv->filter.table_overflow) {
> > > > +		priv->filter.table_overflow = true;
> > > > +		hbg_update_promisc_mode(priv->netdev);
> > > > +		dev_info(&priv->pdev->dev, "mac table is overflow\n");
> > > > +	}
> > > > +
> > > > +	return -ENOSPC;
> > > I _think_ this is wrong. If you run out of hardware resources, you
> > > should change the interface to promiscuous mode and let the stack do
> > > the filtering. Offloading it to hardware is just an acceleration,
> > > nothing more.
> > >
> > > 	Andrew
> > In hbg_update_promisc_mode():
> > priv->filter.enabled = !(priv->filter.table_overflow || (netdev->flags & IFF_PROMISC));
> > hbg_hw_set_mac_filter_enable(priv, priv->filter.enabled);
> >
> > if table_overflow， and netdev->flags not set IFF_PROMISC，
> > the priv->filter.enabled will set to false， Then， The MAC filter will be closed.
> > I think it's probably the same thing you said
> >
> > In this:
> > +	if (!priv->filter.table_overflow) {
> > +		priv->filter.table_overflow = true;
> > +		hbg_update_promisc_mode(priv->netdev);
> > +		dev_info(&priv->pdev->dev, "mac table is overflow\n");
> > +	}
> > +
> > +	return -ENOSPC;
> >
> > When the first overflow occurs, a log is printed, the MAC filter will be disabled, and -ENOSPC is returned.
> > If continue to add MAC addresses, -ENOSPC is returned only.
> This is not obvious from a quick look at the code. Maybe a comment
> would be good.
>
> 	Andrew

ok, Thanks!