On 12/21/2021 7:57 PM, Robert Sanford wrote:
> - Clean up minor typos in comments, strings, and private names.
> - Fix whitespace in log messages and function formatting
> (new line before open brace).
> - Move closing C++ wrapper to the end of rte_eth_bond_8023ad.h.
>
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
> app/test-pmd/cmdline.c | 4 ++--
> app/test/test_link_bonding_mode4.c | 28 +++++++++++++--------------
> drivers/net/bonding/eth_bond_8023ad_private.h | 12 ++++++------
> drivers/net/bonding/rte_eth_bond_8023ad.c | 22 ++++++++++-----------
> drivers/net/bonding/rte_eth_bond_8023ad.h | 15 +++++++-------
> drivers/net/bonding/rte_eth_bond_pmd.c | 13 ++++++++-----
> 6 files changed, 49 insertions(+), 45 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6e10afe..9fd2c2a 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -630,8 +630,8 @@ static void cmd_help_long_parsed(void *parsed_result,
> "set bonding mac_addr (port_id) (address)\n"
> " Set the MAC address of a bonded device.\n\n"
>
> - "set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)"
> - " Set Aggregation mode for IEEE802.3AD (mode 4)"
> + "set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
> + " Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"
ack
>
> "set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
> " Set the transmit balance policy for bonded device running in balance mode.\n\n"
> diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
> index 351129d..2be86d5 100644
> --- a/app/test/test_link_bonding_mode4.c
> +++ b/app/test/test_link_bonding_mode4.c
> @@ -58,11 +58,11 @@ static const struct rte_ether_addr slave_mac_default = {
> { 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 }
> };
>
> -static const struct rte_ether_addr parnter_mac_default = {
> +static const struct rte_ether_addr partner_mac_default = {
ack
<...>
> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index 9b5738a..60db31e 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -15,12 +15,12 @@
> #include "rte_eth_bond_8023ad.h"
>
> #define BOND_MODE_8023AX_UPDATE_TIMEOUT_MS 100
> -/** Maximum number of packets to one slave queued in TX ring. */
> +/** Maximum number of packets to one slave queued in RX ring. */
> #define BOND_MODE_8023AX_SLAVE_RX_PKTS 3
> /** Maximum number of LACP packets from one slave queued in TX ring. */
> #define BOND_MODE_8023AX_SLAVE_TX_PKTS 1
> /**
> - * Timeouts deffinitions (5.4.4 in 802.1AX documentation).
> + * Timeouts definitions (6.4.4 in 802.1AX documentation).
There are multiple updates in the document reference, section 5 -> 6,
since the old code is consistent about section 5, can it be because
of document version change?
If so should we document the document version to prevent same thing
happen again?
<...>
>
> -#ifdef __cplusplus
> -}
> -#endif
> -
> /**
> * Configure a slave port to start collecting.
> *
> @@ -331,4 +327,9 @@ rte_eth_bond_8023ad_agg_selection_get(uint16_t port_id);
> int
> rte_eth_bond_8023ad_agg_selection_set(uint16_t port_id,
> enum rte_bond_8023ad_agg_selection agg_selection);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
This is not typo or whitespace, it seems we misplaced the cpp block, so this
may result issues for cpp applications using this header, I wonder if we should
have separate patch for fixes?
> #endif /* RTE_ETH_BOND_8023AD_H_ */
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 84f4900..f6003b0 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -158,7 +158,8 @@ const struct rte_flow_attr flow_attr_8023ad = {
>
> int
> bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
> - uint16_t slave_port) {
> + uint16_t slave_port)
> +{
OK to typo fixes, but not sure about the syntax fixes, they corrupt the git
history for little benefit, I think better to fix these when this code is
changed for some other functional reason.
What to you think to drop these changes?
And overall perhaps this change can be split into more patches, that also
helps backporting:
- spelling error, typo, whitespace fixes
- Document reference fix
- ifdef __cplusplus fix
@@ -630,8 +630,8 @@ static void cmd_help_long_parsed(void *parsed_result,
"set bonding mac_addr (port_id) (address)\n"
" Set the MAC address of a bonded device.\n\n"
- "set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)"
- " Set Aggregation mode for IEEE802.3AD (mode 4)"
+ "set bonding mode IEEE802.3AD aggregator policy (port_id) (agg_name)\n"
+ " Set Aggregation mode for IEEE802.3AD (mode 4)\n\n"
"set bonding balance_xmit_policy (port_id) (l2|l23|l34)\n"
" Set the transmit balance policy for bonded device running in balance mode.\n\n"
@@ -58,11 +58,11 @@ static const struct rte_ether_addr slave_mac_default = {
{ 0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 }
};
-static const struct rte_ether_addr parnter_mac_default = {
+static const struct rte_ether_addr partner_mac_default = {
{ 0x22, 0xBB, 0xFF, 0xBB, 0x00, 0x00 }
};
-static const struct rte_ether_addr parnter_system = {
+static const struct rte_ether_addr partner_system = {
{ 0x33, 0xFF, 0xBB, 0xFF, 0x00, 0x00 }
};
@@ -76,7 +76,7 @@ struct slave_conf {
uint16_t port_id;
uint8_t bonded : 1;
- uint8_t lacp_parnter_state;
+ uint8_t lacp_partner_state;
};
struct ether_vlan_hdr {
@@ -258,7 +258,7 @@ add_slave(struct slave_conf *slave, uint8_t start)
TEST_ASSERT_EQUAL(rte_is_same_ether_addr(&addr, &addr_check), 1,
"Slave MAC address is not as expected");
- RTE_VERIFY(slave->lacp_parnter_state == 0);
+ RTE_VERIFY(slave->lacp_partner_state == 0);
return 0;
}
@@ -288,7 +288,7 @@ remove_slave(struct slave_conf *slave)
test_params.bonded_port_id);
slave->bonded = 0;
- slave->lacp_parnter_state = 0;
+ slave->lacp_partner_state = 0;
return 0;
}
@@ -501,20 +501,20 @@ make_lacp_reply(struct slave_conf *slave, struct rte_mbuf *pkt)
slow_hdr = rte_pktmbuf_mtod(pkt, struct slow_protocol_frame *);
/* Change source address to partner address */
- rte_ether_addr_copy(&parnter_mac_default, &slow_hdr->eth_hdr.src_addr);
+ rte_ether_addr_copy(&partner_mac_default, &slow_hdr->eth_hdr.src_addr);
slow_hdr->eth_hdr.src_addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] =
slave->port_id;
lacp = (struct lacpdu *) &slow_hdr->slow_protocol;
/* Save last received state */
- slave->lacp_parnter_state = lacp->actor.state;
+ slave->lacp_partner_state = lacp->actor.state;
/* Change it into LACP replay by matching parameters. */
memcpy(&lacp->partner.port_params, &lacp->actor.port_params,
sizeof(struct port_params));
lacp->partner.state = lacp->actor.state;
- rte_ether_addr_copy(&parnter_system, &lacp->actor.port_params.system);
+ rte_ether_addr_copy(&partner_system, &lacp->actor.port_params.system);
lacp->actor.state = STATE_LACP_ACTIVE |
STATE_SYNCHRONIZATION |
STATE_AGGREGATION |
@@ -580,7 +580,7 @@ bond_handshake_done(struct slave_conf *slave)
const uint8_t expected_state = STATE_LACP_ACTIVE | STATE_SYNCHRONIZATION |
STATE_AGGREGATION | STATE_COLLECTING | STATE_DISTRIBUTING;
- return slave->lacp_parnter_state == expected_state;
+ return slave->lacp_partner_state == expected_state;
}
static unsigned
@@ -1165,7 +1165,7 @@ init_marker(struct rte_mbuf *pkt, struct slave_conf *slave)
&marker_hdr->eth_hdr.dst_addr);
/* Init source address */
- rte_ether_addr_copy(&parnter_mac_default,
+ rte_ether_addr_copy(&partner_mac_default,
&marker_hdr->eth_hdr.src_addr);
marker_hdr->eth_hdr.src_addr.addr_bytes[RTE_ETHER_ADDR_LEN - 1] =
slave->port_id;
@@ -1353,7 +1353,7 @@ test_mode4_expired(void)
/* After test only expected slave should be in EXPIRED state */
FOR_EACH_SLAVE(i, slave) {
if (slave == exp_slave)
- TEST_ASSERT(slave->lacp_parnter_state & STATE_EXPIRED,
+ TEST_ASSERT(slave->lacp_partner_state & STATE_EXPIRED,
"Slave %u should be in expired.", slave->port_id);
else
TEST_ASSERT_EQUAL(bond_handshake_done(slave), 1,
@@ -1392,7 +1392,7 @@ test_mode4_ext_ctrl(void)
},
};
- rte_ether_addr_copy(&parnter_system, &src_mac);
+ rte_ether_addr_copy(&partner_system, &src_mac);
rte_ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
@@ -1446,7 +1446,7 @@ test_mode4_ext_lacp(void)
},
};
- rte_ether_addr_copy(&parnter_system, &src_mac);
+ rte_ether_addr_copy(&partner_system, &src_mac);
rte_ether_addr_copy(&slow_protocol_mac_addr, &dst_mac);
initialize_eth_header(&lacpdu.eth_hdr, &src_mac, &dst_mac,
@@ -1535,7 +1535,7 @@ check_environment(void)
if (port->bonded != 0)
env_state |= 0x04;
- if (port->lacp_parnter_state != 0)
+ if (port->lacp_partner_state != 0)
env_state |= 0x08;
if (env_state != 0)
@@ -15,12 +15,12 @@
#include "rte_eth_bond_8023ad.h"
#define BOND_MODE_8023AX_UPDATE_TIMEOUT_MS 100
-/** Maximum number of packets to one slave queued in TX ring. */
+/** Maximum number of packets to one slave queued in RX ring. */
#define BOND_MODE_8023AX_SLAVE_RX_PKTS 3
/** Maximum number of LACP packets from one slave queued in TX ring. */
#define BOND_MODE_8023AX_SLAVE_TX_PKTS 1
/**
- * Timeouts deffinitions (5.4.4 in 802.1AX documentation).
+ * Timeouts definitions (6.4.4 in 802.1AX documentation).
*/
#define BOND_8023AD_FAST_PERIODIC_MS 900
#define BOND_8023AD_SLOW_PERIODIC_MS 29000
@@ -34,7 +34,7 @@
/**
* Interval of showing warning message from state machines. All messages will
* be held (and gathered together) to prevent flooding.
- * This is no parto of 802.1AX standard.
+ * This is not part of 802.1AX standard.
*/
#define BOND_8023AD_WARNINGS_PERIOD_MS 1000
@@ -83,7 +83,7 @@
#define PARTNER_STATE_SET(_p, _f) SET_FLAGS((_p)->partner_state, STATE_ ## _f)
#define PARTNER_STATE_CLR(_p, _f) CLEAR_FLAGS((_p)->partner_state, STATE_ ## _f)
-/** Variables associated with each port (5.4.7 in 802.1AX documentation). */
+/** Variables associated with each port (6.4.7 in 802.1AX documentation). */
struct port {
/**
* The operational values of the Actor's state parameters. Bitmask
@@ -124,7 +124,7 @@ struct port {
uint64_t wait_while_timer;
uint64_t tx_machine_timer;
uint64_t tx_marker_timer;
- /* Agregator parameters */
+ /* Aggregator parameters */
/** Used aggregator port ID */
uint16_t aggregator_port_id;
@@ -280,7 +280,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *dev, uint16_t port_id);
/**
* @internal
*
- * Denitializes and removes given slave from 802.1AX mode.
+ * Deinitializes and removes given slave from 802.1AX mode.
*
* @param dev Bonded interface.
* @param slave_num Position of slave in active_slaves array
@@ -207,15 +207,15 @@ show_warnings(uint16_t slave_id)
if (warnings & WRN_RX_QUEUE_FULL) {
RTE_BOND_LOG(DEBUG,
"Slave %u: failed to enqueue LACP packet into RX ring.\n"
- "Receive and transmit functions must be invoked on bonded"
- "interface at least 10 times per second or LACP will notwork correctly",
+ "Receive and transmit functions must be invoked on bonded\n"
+ "interface at least 10 times per second or LACP will not work correctly",
slave_id);
}
if (warnings & WRN_TX_QUEUE_FULL) {
RTE_BOND_LOG(DEBUG,
"Slave %u: failed to enqueue LACP packet into TX ring.\n"
- "Receive and transmit functions must be invoked on bonded"
+ "Receive and transmit functions must be invoked on bonded\n"
"interface at least 10 times per second or LACP will not work correctly",
slave_id);
}
@@ -250,7 +250,7 @@ record_default(struct port *port)
/** Function handles rx state machine.
*
- * This function implements Receive State Machine from point 5.4.12 in
+ * This function implements Receive State Machine from point 6.4.12 in
* 802.1AX documentation. It should be called periodically.
*
* @param lacpdu LACPDU received.
@@ -384,7 +384,7 @@ rx_machine(struct bond_dev_private *internals, uint16_t slave_id,
/**
* Function handles periodic tx state machine.
*
- * Function implements Periodic Transmission state machine from point 5.4.13
+ * Function implements Periodic Transmission state machine from point 6.4.13
* in 802.1AX documentation. It should be called periodically.
*
* @param port Port to handle state machine.
@@ -446,7 +446,7 @@ periodic_machine(struct bond_dev_private *internals, uint16_t slave_id)
/**
* Function handles mux state machine.
*
- * Function implements Mux Machine from point 5.4.15 in 802.1AX documentation.
+ * Function implements Mux Machine from point 6.4.15 in 802.1AX documentation.
* It should be called periodically.
*
* @param port Port to handle state machine.
@@ -549,7 +549,7 @@ mux_machine(struct bond_dev_private *internals, uint16_t slave_id)
/**
* Function handles transmit state machine.
*
- * Function implements Transmit Machine from point 5.4.16 in 802.1AX
+ * Function implements Transmit Machine from point 6.4.16 in 802.1AX
* documentation.
*
* @param port
@@ -1051,14 +1051,14 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
struct bond_tx_queue *bd_tx_q;
uint16_t q_id;
- /* Given slave mus not be in active list */
+ /* Given slave must not be in active list. */
RTE_ASSERT(find_slave_by_id(internals->active_slaves,
internals->active_slave_count, slave_id) == internals->active_slave_count);
RTE_SET_USED(internals); /* used only for assert when enabled */
memcpy(&port->actor, &initial, sizeof(struct port_params));
- /* Standard requires that port ID must be grater than 0.
- * Add 1 do get corresponding port_number */
+ /* Standard requires that port ID must be greater than 0.
+ * Add 1 to get corresponding port_number. */
port->actor.port_number = rte_cpu_to_be_16(slave_id + 1);
memcpy(&port->partner, &initial, sizeof(struct port_params));
@@ -1069,7 +1069,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
port->partner_state = STATE_LACP_ACTIVE | STATE_AGGREGATION;
port->sm_flags = SM_FLAGS_BEGIN;
- /* use this port as agregator */
+ /* Use this port as aggregator. */
port->aggregator_port_id = slave_id;
if (bond_mode_8023ad_register_lacp_mac(slave_id) < 0) {
@@ -68,7 +68,7 @@ struct port_params {
struct rte_ether_addr system;
/**< System ID - Slave MAC address, same as bonding MAC address */
uint16_t key;
- /**< Speed information (implementation dependednt) and duplex. */
+ /**< Speed information (implementation dependent) and duplex. */
uint16_t port_priority;
/**< Priority of this (unused in current implementation) */
uint16_t port_number;
@@ -83,7 +83,7 @@ struct lacpdu_actor_partner_params {
uint8_t reserved_3[3];
} __rte_packed __rte_aligned(2);
-/** LACPDU structure (5.4.2 in 802.1AX documentation). */
+/** LACPDU structure (6.4.2 in 802.1AX documentation). */
struct lacpdu {
uint8_t subtype;
uint8_t version_number;
@@ -153,7 +153,7 @@ struct rte_eth_bond_8023ad_slave_info {
/**
* @internal
*
- * Function returns current configuration of 802.3AX mode.
+ * Function returns current configuration of 802.1AX mode.
*
* @param port_id Bonding device id
* @param conf Pointer to timeout structure.
@@ -197,10 +197,6 @@ int
rte_eth_bond_8023ad_slave_info(uint16_t port_id, uint16_t slave_id,
struct rte_eth_bond_8023ad_slave_info *conf);
-#ifdef __cplusplus
-}
-#endif
-
/**
* Configure a slave port to start collecting.
*
@@ -331,4 +327,9 @@ rte_eth_bond_8023ad_agg_selection_get(uint16_t port_id);
int
rte_eth_bond_8023ad_agg_selection_set(uint16_t port_id,
enum rte_bond_8023ad_agg_selection agg_selection);
+
+#ifdef __cplusplus
+}
+#endif
+
#endif /* RTE_ETH_BOND_8023AD_H_ */
@@ -158,7 +158,8 @@ const struct rte_flow_attr flow_attr_8023ad = {
int
bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
- uint16_t slave_port) {
+ uint16_t slave_port)
+{
struct rte_eth_dev_info slave_info;
struct rte_flow_error error;
struct bond_dev_private *internals = bond_dev->data->dev_private;
@@ -207,7 +208,8 @@ bond_ethdev_8023ad_flow_verify(struct rte_eth_dev *bond_dev,
}
int
-bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id) {
+bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id)
+{
struct rte_eth_dev *bond_dev = &rte_eth_devices[port_id];
struct bond_dev_private *internals = bond_dev->data->dev_private;
struct rte_eth_dev_info bond_info;
@@ -240,8 +242,8 @@ bond_8023ad_slow_pkt_hw_filter_supported(uint16_t port_id) {
}
int
-bond_ethdev_8023ad_flow_set(struct rte_eth_dev *bond_dev, uint16_t slave_port) {
-
+bond_ethdev_8023ad_flow_set(struct rte_eth_dev *bond_dev, uint16_t slave_port)
+{
struct rte_flow_error error;
struct bond_dev_private *internals = bond_dev->data->dev_private;
struct rte_flow_action_queue lacp_queue_conf = {
@@ -809,7 +811,8 @@ struct bwg_slave {
};
void
-bond_tlb_activate_slave(struct bond_dev_private *internals) {
+bond_tlb_activate_slave(struct bond_dev_private *internals)
+{
int i;
for (i = 0; i < internals->active_slave_count; i++) {