Use possibly more efficient ether_addr_equal_64bits
to instead of memcmp.
Modify the MAC_ADDR_COMPARE to MAC_ADDR_EQUAL, this looks more
appropriate.
The comments for the bond 3ad is too old, cleanup some errors
and warming.
Suggested-by: Joe Perches <[email protected]>
Signed-off-by: Ding Tianhong <[email protected]>
---
drivers/net/bonding/bond_3ad.c | 62 ++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 58c2249..5393e1e 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -90,8 +90,9 @@
#define AD_LINK_SPEED_BITMASK_10000MBPS 0x10
//endalloun
-// compare MAC addresses
-#define MAC_ADDRESS_COMPARE(A, B) memcmp(A, B, ETH_ALEN)
+/* compare MAC addresses */
+#define MAC_ADDRESS_EQUAL(A, B) \
+ ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };
static u16 ad_ticks_per_sec;
@@ -417,17 +418,18 @@ static u16 __ad_timer_to_ticks(u16 timer_type, u16 par)
*/
static void __choose_matched(struct lacpdu *lacpdu, struct port *port)
{
- // check if all parameters are alike
+ /* check if all parameters are alike
+ * or this is individual link(aggregation == FALSE)
+ * then update the state machine Matched variable.
+ */
if (((ntohs(lacpdu->partner_port) == port->actor_port_number) &&
(ntohs(lacpdu->partner_port_priority) == port->actor_port_priority) &&
- !MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) &&
+ MAC_ADDRESS_EQUAL(&(lacpdu->partner_system), &(port->actor_system)) &&
(ntohs(lacpdu->partner_system_priority) == port->actor_system_priority) &&
(ntohs(lacpdu->partner_key) == port->actor_oper_port_key) &&
((lacpdu->partner_state & AD_STATE_AGGREGATION) == (port->actor_oper_port_state & AD_STATE_AGGREGATION))) ||
- // or this is individual link(aggregation == FALSE)
((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)
) {
- // update the state machine Matched variable
port->sm_vars |= AD_PORT_MATCHED;
} else {
port->sm_vars &= ~AD_PORT_MATCHED;
@@ -507,14 +509,15 @@ static void __update_selected(struct lacpdu *lacpdu, struct port *port)
if (lacpdu && port) {
const struct port_params *partner = &port->partner_oper;
- // check if any parameter is different
+ /* check if any parameter is different then
+ * update the state machine selected variable.
+ */
if (ntohs(lacpdu->actor_port) != partner->port_number ||
ntohs(lacpdu->actor_port_priority) != partner->port_priority ||
- MAC_ADDRESS_COMPARE(&lacpdu->actor_system, &partner->system) ||
+ !MAC_ADDRESS_EQUAL(&lacpdu->actor_system, &partner->system) ||
ntohs(lacpdu->actor_system_priority) != partner->system_priority ||
ntohs(lacpdu->actor_key) != partner->key ||
(lacpdu->actor_state & AD_STATE_AGGREGATION) != (partner->port_state & AD_STATE_AGGREGATION)) {
- // update the state machine Selected variable
port->sm_vars &= ~AD_PORT_SELECTED;
}
}
@@ -538,15 +541,16 @@ static void __update_default_selected(struct port *port)
const struct port_params *admin = &port->partner_admin;
const struct port_params *oper = &port->partner_oper;
- // check if any parameter is different
+ /* check if any parameter is different then
+ * update the state machine selected variable.
+ */
if (admin->port_number != oper->port_number ||
admin->port_priority != oper->port_priority ||
- MAC_ADDRESS_COMPARE(&admin->system, &oper->system) ||
+ !MAC_ADDRESS_EQUAL(&admin->system, &oper->system) ||
admin->system_priority != oper->system_priority ||
admin->key != oper->key ||
(admin->port_state & AD_STATE_AGGREGATION)
!= (oper->port_state & AD_STATE_AGGREGATION)) {
- // update the state machine Selected variable
port->sm_vars &= ~AD_PORT_SELECTED;
}
}
@@ -566,12 +570,14 @@ static void __update_default_selected(struct port *port)
*/
static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
{
- // validate lacpdu and port
+ /* validate lacpdu and port */
if (lacpdu && port) {
- // check if any parameter is different
+ /* check if any parameter is different then
+ * update the port->ntt.
+ */
if ((ntohs(lacpdu->partner_port) != port->actor_port_number) ||
(ntohs(lacpdu->partner_port_priority) != port->actor_port_priority) ||
- MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) ||
+ !MAC_ADDRESS_EQUAL(&(lacpdu->partner_system), &(port->actor_system)) ||
(ntohs(lacpdu->partner_system_priority) != port->actor_system_priority) ||
(ntohs(lacpdu->partner_key) != port->actor_oper_port_key) ||
((lacpdu->partner_state & AD_STATE_LACP_ACTIVITY) != (port->actor_oper_port_state & AD_STATE_LACP_ACTIVITY)) ||
@@ -579,7 +585,6 @@ static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
((lacpdu->partner_state & AD_STATE_SYNCHRONIZATION) != (port->actor_oper_port_state & AD_STATE_SYNCHRONIZATION)) ||
((lacpdu->partner_state & AD_STATE_AGGREGATION) != (port->actor_oper_port_state & AD_STATE_AGGREGATION))
) {
-
port->ntt = true;
}
}
@@ -1076,9 +1081,8 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
break;
case AD_RX_CURRENT:
- // detect loopback situation
- if (!MAC_ADDRESS_COMPARE(&(lacpdu->actor_system), &(port->actor_system))) {
- // INFO_RECEIVED_LOOPBACK_FRAMES
+ /* detect loopback situation */
+ if (MAC_ADDRESS_EQUAL(&(lacpdu->actor_system), &(port->actor_system))) {
pr_err("%s: An illegal loopback occurred on adapter (%s).\n"
"Check the configuration to verify that all adapters are connected to 802.3ad compliant switch ports\n",
port->slave->bond->dev->name, port->slave->dev->name);
@@ -1090,7 +1094,7 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(port->actor_oper_port_state & AD_STATE_LACP_TIMEOUT));
port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
break;
- default: //to silence the compiler
+ default: /* to silence the compiler */
break;
}
}
@@ -1281,17 +1285,17 @@ static void ad_port_selection_logic(struct port *port)
free_aggregator = aggregator;
continue;
}
- // check if current aggregator suits us
- if (((aggregator->actor_oper_aggregator_key == port->actor_oper_port_key) && // if all parameters match AND
- !MAC_ADDRESS_COMPARE(&(aggregator->partner_system), &(port->partner_oper.system)) &&
+ /* check if current aggregator suits us */
+ if (((aggregator->actor_oper_aggregator_key == port->actor_oper_port_key) && /* if all parameters match AND */
+ MAC_ADDRESS_EQUAL(&(aggregator->partner_system), &(port->partner_oper.system)) &&
(aggregator->partner_system_priority == port->partner_oper.system_priority) &&
(aggregator->partner_oper_aggregator_key == port->partner_oper.key)
) &&
- ((MAC_ADDRESS_COMPARE(&(port->partner_oper.system), &(null_mac_addr)) && // partner answers
- !aggregator->is_individual) // but is not individual OR
+ ((!MAC_ADDRESS_EQUAL(&(port->partner_oper.system), &(null_mac_addr)) && /* partner answers */
+ !aggregator->is_individual) /* but is not individual OR */
)
) {
- // attach to the founded aggregator
+ /* attach to the founded aggregator */
port->aggregator = aggregator;
port->actor_port_aggregator_identifier =
port->aggregator->aggregator_identifier;
@@ -1705,7 +1709,7 @@ static void ad_enable_collecting_distributing(struct port *port)
*/
static void ad_disable_collecting_distributing(struct port *port)
{
- if (port->aggregator && MAC_ADDRESS_COMPARE(&(port->aggregator->partner_system), &(null_mac_addr))) {
+ if (port->aggregator && !MAC_ADDRESS_EQUAL(&(port->aggregator->partner_system), &(null_mac_addr))) {
pr_debug("Disabling port %d(LAG %d)\n",
port->actor_port_number,
port->aggregator->aggregator_identifier);
@@ -1826,8 +1830,8 @@ static u16 aggregator_identifier;
*/
void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution)
{
- // check that the bond is not initialized yet
- if (MAC_ADDRESS_COMPARE(&(BOND_AD_INFO(bond).system.sys_mac_addr),
+ /* check that the bond is not initialized yet */
+ if (!MAC_ADDRESS_EQUAL(&(BOND_AD_INFO(bond).system.sys_mac_addr),
bond->dev->dev_addr)) {
aggregator_identifier = 0;
--
1.8.0
> -// compare MAC addresses
> -#define MAC_ADDRESS_COMPARE(A, B) memcmp(A, B, ETH_ALEN)
> +/* compare MAC addresses */
> +#define MAC_ADDRESS_EQUAL(A, B) \
> + ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
Are the casts needed?
julia
On 2014/1/2 15:39, Julia Lawall wrote:
> Are the casts needed
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
Regards
Ding
On Thu, 2 Jan 2014, Ding Tianhong wrote:
> On 2014/1/2 15:39, Julia Lawall wrote:
> > Are the casts needed
>
>
> Yes, otherwise the warming will report:
>
> /net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
Is it necessary for this driver to use a different type from everyone
else?
julia
On 2014/1/2 16:38, Julia Lawall wrote:
> On Thu, 2 Jan 2014, Ding Tianhong wrote:
>
>> On 2014/1/2 15:39, Julia Lawall wrote:
>>> Are the casts needed
>>
>>
>> Yes, otherwise the warming will report:
>>
>> /net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
>
> Is it necessary for this driver to use a different type from everyone
> else?
>
> julia
>
Did you mean the MAC_ADDRESS_EQUAL is excess?
I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
Regards
Ding
On Thu, 2 Jan 2014, Ding Tianhong wrote:
> On 2014/1/2 16:38, Julia Lawall wrote:
> > On Thu, 2 Jan 2014, Ding Tianhong wrote:
> >
> >> On 2014/1/2 15:39, Julia Lawall wrote:
> >>> Are the casts needed
> >>
> >>
> >> Yes, otherwise the warming will report:
> >>
> >> /net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
> >
> > Is it necessary for this driver to use a different type from everyone
> > else?
> >
> > julia
> >
> Did you mean the MAC_ADDRESS_EQUAL is excess?
> I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other
code just has the array inlined into the containing structure.
julia
On 2014/1/2 17:14, Julia Lawall wrote:
> On Thu, 2 Jan 2014, Ding Tianhong wrote:
>
>> On 2014/1/2 16:38, Julia Lawall wrote:
>>> On Thu, 2 Jan 2014, Ding Tianhong wrote:
>>>
>>>> On 2014/1/2 15:39, Julia Lawall wrote:
>>>>> Are the casts needed
>>>>
>>>>
>>>> Yes, otherwise the warming will report:
>>>>
>>>> /net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
>>>
>>> Is it necessary for this driver to use a different type from everyone
>>> else?
>>>
>>> julia
>>>
>> Did you mean the MAC_ADDRESS_EQUAL is excess?
>> I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
>
> No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other
> code just has the array inlined into the containing structure.
>
> julia
>
Oh, sorry for mismatch.:)
The code for bond_3ad mode is too old and the use for mac addr is not so comfortable.
I think I need to send a patch to fix the unusual mac addr and make it more reasonable.
Thanks for your opinion.
Regards
Ding
On 2014/1/2 18:26, Ding Tianhong wrote:
> On 2014/1/2 17:14, Julia Lawall wrote:
>> On Thu, 2 Jan 2014, Ding Tianhong wrote:
>>
>>> On 2014/1/2 16:38, Julia Lawall wrote:
>>>> On Thu, 2 Jan 2014, Ding Tianhong wrote:
>>>>
>>>>> On 2014/1/2 15:39, Julia Lawall wrote:
>>>>>> Are the casts needed
>>>>>
>>>>>
>>>>> Yes, otherwise the warming will report:
>>>>>
>>>>> /net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
>>>>
>>>> Is it necessary for this driver to use a different type from everyone
>>>> else?
>>>>
>>>> julia
>>>>
>>> Did you mean the MAC_ADDRESS_EQUAL is excess?
>>> I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
>>
>> No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other
>> code just has the array inlined into the containing structure.
>>
>> julia
>>
I reviewed the struct mac_addr again, and feel that even it looks not comfortable, but
make the lacp struct more meaning for 3ad, what do you think about it, I think no need
to revert them to u8.
Regards
Ding
>
> Oh, sorry for mismatch.:)
> The code for bond_3ad mode is too old and the use for mac addr is not so comfortable.
>
> I think I need to send a patch to fix the unusual mac addr and make it more reasonable.
>
> Thanks for your opinion.
>
> Regards
> Ding
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
On Thu, 2 Jan 2014, Ding Tianhong wrote:
> On 2014/1/2 18:26, Ding Tianhong wrote:
> > On 2014/1/2 17:14, Julia Lawall wrote:
> >> On Thu, 2 Jan 2014, Ding Tianhong wrote:
> >>
> >>> On 2014/1/2 16:38, Julia Lawall wrote:
> >>>> On Thu, 2 Jan 2014, Ding Tianhong wrote:
> >>>>
> >>>>> On 2014/1/2 15:39, Julia Lawall wrote:
> >>>>>> Are the casts needed
> >>>>>
> >>>>>
> >>>>> Yes, otherwise the warming will report:
> >>>>>
> >>>>> /net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
> >>>>
> >>>> Is it necessary for this driver to use a different type from everyone
> >>>> else?
> >>>>
> >>>> julia
> >>>>
> >>> Did you mean the MAC_ADDRESS_EQUAL is excess?
> >>> I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
> >>
> >> No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other
> >> code just has the array inlined into the containing structure.
> >>
> >> julia
> >>
>
> I reviewed the struct mac_addr again, and feel that even it looks not comfortable, but
> make the lacp struct more meaning for 3ad, what do you think about it, I think no need
> to revert them to u8.
Personally, when I see things that are different, I start wondering about
why. So if there is no reason for it to be different, I would prefer that
it is the same.
Certainly, a mac_addr type is more meaningful than just an array with size
ETH_ALEN, or worse an array with size 6. But I am not sure that it is
practical to introduce that type everywhere.
In any case, it is not a big issue.
julia