diff mbox series

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

Message ID 20230814022948.2019698-1-ruanjinjie@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] ethernet: rocker: Use is_broadcast_ether_addr() and is_multicast_ether_addr() instead of ether_addr_equal() | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1330 this patch: 1330
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1353 this patch: 1353
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1353 this patch: 1353
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 23 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jinjie Ruan Aug. 14, 2023, 2:29 a.m. UTC 
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>
---
 drivers/net/ethernet/rocker/rocker_ofdpa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Simon Horman Aug. 14, 2023, 7:47 a.m. UTC | #1 
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

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

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

>  			priority = OFDPA_PRIORITY_ACL_DFLT;
>  		else if (is_link_local_ether_addr(eth_dst))
>  			priority = OFDPA_PRIORITY_ACL_CTRL;
> -- 
> 2.34.1
> 
>
Jinjie Ruan Aug. 14, 2023, 8:32 a.m. UTC | #2 
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.

> 
>>  			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
>>
>>
Simon Horman Aug. 14, 2023, 3:13 p.m. UTC | #3 
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
> >>
> >>
>
diff mbox series

Patch

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))
 			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))
 			priority = OFDPA_PRIORITY_ACL_DFLT;
 		else if (is_link_local_ether_addr(eth_dst))
 			priority = OFDPA_PRIORITY_ACL_CTRL;