Re: [PATCH net-next] ethernet: rocker: Use is_broadcast_ether_addr() and is_multicast_ether_addr() instead of ether_addr_equal()

[Simon Horman <horms@kernel.org>]

On Mon, Aug 14, 2023 at 04:32:56PM +0800, Ruan Jinjie wrote:
> 
> 
> On 2023/8/14 15:47, Simon Horman wrote:
> > On Mon, Aug 14, 2023 at 10:29:48AM +0800, Ruan Jinjie wrote:
> >> Use is_broadcast_ether_addr() and is_multicast_ether_addr() instead of
> >> ether_addr_equal() to check if the ethernet address is broadcast
> >> and multicast address separately.
> >>
> >> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> > 
> > Hi,
> > 
> > Perhaps we could go for a more concise prefix and subject, as the current one
> > is rather long. Maybe something like:
> > 
> > Subject: [PATCH net-next]: rocker: Use helpers to check broadcast and multicast Ether addresses
> 
> Right！That is more concise.
> 
> > 
> >> ---
> >>  drivers/net/ethernet/rocker/rocker_ofdpa.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/rocker/rocker_ofdpa.c b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> >> index 826990459fa4..7f389f3adbf4 100644
> >> --- a/drivers/net/ethernet/rocker/rocker_ofdpa.c
> >> +++ b/drivers/net/ethernet/rocker/rocker_ofdpa.c
> >> @@ -208,7 +208,6 @@ static const u8 zero_mac[ETH_ALEN]   = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
> >>  static const u8 ff_mac[ETH_ALEN]     = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> >>  static const u8 ll_mac[ETH_ALEN]     = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 };
> >>  static const u8 ll_mask[ETH_ALEN]    = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xf0 };
> >> -static const u8 mcast_mac[ETH_ALEN]  = { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00 };
> >>  static const u8 ipv4_mcast[ETH_ALEN] = { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x00 };
> >>  static const u8 ipv4_mask[ETH_ALEN]  = { 0xff, 0xff, 0xff, 0x80, 0x00, 0x00 };
> >>  static const u8 ipv6_mcast[ETH_ALEN] = { 0x33, 0x33, 0x00, 0x00, 0x00, 0x00 };
> >> @@ -939,7 +938,7 @@ static int ofdpa_flow_tbl_bridge(struct ofdpa_port *ofdpa_port,
> >>  	if (eth_dst_mask) {
> >>  		entry->key.bridge.has_eth_dst_mask = 1;
> >>  		ether_addr_copy(entry->key.bridge.eth_dst_mask, eth_dst_mask);
> >> -		if (!ether_addr_equal(eth_dst_mask, ff_mac))
> >> +		if (!is_broadcast_ether_addr(eth_dst_mask))
> > 
> > Probably it is ok, but is_broadcast_ether_addr()
> > covers a set of addresses that includes ff_mac.
> 
> I reconfirmed that they are equivalent, is_broadcast_ether_addr()
> requires all six bytes to be F.

Right, agreed.  Sorry for misunderstanding the implementation of
is_broadcast_ether_addr() when I checked earlier today.

> 
> > 
> >>  			wild = true;
> >>  	}
> >>  
> >> @@ -1012,7 +1011,7 @@ static int ofdpa_flow_tbl_acl(struct ofdpa_port *ofdpa_port, int flags,
> >>  
> >>  	priority = OFDPA_PRIORITY_ACL_NORMAL;
> >>  	if (eth_dst && eth_dst_mask) {
> >> -		if (ether_addr_equal(eth_dst_mask, mcast_mac))
> >> +		if (is_multicast_ether_addr(eth_dst_mask))
> > 
> > Likewise, is_multicast_ether_addr()
> > covers a set of addresses that includes mcast_mac.
> 
> They are not exactly equivalent，the address what the mcast_mac get is
> the subset of is_multicast_ether_addr().
> 
> > 
> >>  			priority = OFDPA_PRIORITY_ACL_DFLT;
> >>  		else if (is_link_local_ether_addr(eth_dst))
> >>  			priority = OFDPA_PRIORITY_ACL_CTRL;
> >> -- 
> >> 2.34.1
> >>
> >>
>