[dpdk-dev] net/sfc: fix main MAC address handling
Checks
Commit Message
From: Ivan Malov <ivan.malov@oktetlabs.ru>
There is a school of thought that rte_eth_dev_default_mac_addr_set()
must call the PMD callback first and then save the new MAC address
in dev->data->mac_addrs[0]. If this concept gets approved, it will
break the current approach used in sfc driver as the latter relies
on the assumption that the new MAC address is already contained in
dev->data->mac_addrs[0], and, if adapter restart is needed to make
the HW apply the new address, the outdated value will be retrieved
from dev->data. In order to preclude any possible bugs, this patch
adds device private storage for the up-to-date copy of the address.
Fixes: c100fd464bb7 ("net/sfc: support main MAC address change")
Cc: stable@dpdk.org
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
drivers/net/sfc/sfc.h | 2 ++
drivers/net/sfc/sfc_ethdev.c | 19 ++++++++++++++++---
drivers/net/sfc/sfc_port.c | 10 ++++++++--
3 files changed, 26 insertions(+), 5 deletions(-)
Comments
On 12/20/2017 1:52 AM, Andrew Rybchenko wrote:
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>
> There is a school of thought that rte_eth_dev_default_mac_addr_set()
> must call the PMD callback first and then save the new MAC address
> in dev->data->mac_addrs[0].
:)
> If this concept gets approved, it will
> break the current approach used in sfc driver as the latter relies
> on the assumption that the new MAC address is already contained in
> dev->data->mac_addrs[0], and, if adapter restart is needed to make
> the HW apply the new address, the outdated value will be retrieved
> from dev->data. In order to preclude any possible bugs, this patch
> adds device private storage for the up-to-date copy of the address.
Thank you for taking care of this.
>
> Fixes: c100fd464bb7 ("net/sfc: support main MAC address change")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Applied to dpdk-next-net/master, thanks.
@@ -158,6 +158,8 @@ struct sfc_port {
boolean_t promisc;
boolean_t allmulti;
+ struct ether_addr default_mac_addr;
+
unsigned int max_mcast_addrs;
unsigned int nb_mcast_addrs;
uint8_t *mcast_addrs;
@@ -926,6 +926,12 @@ sfc_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
sfc_adapter_lock(sa);
+ /*
+ * Copy the address to the device private data so that
+ * it could be recalled in the case of adapter restart.
+ */
+ ether_addr_copy(mac_addr, &port->default_mac_addr);
+
if (port->isolated) {
sfc_err(sa, "isolated mode is active on the port");
sfc_err(sa, "will not set MAC address");
@@ -961,9 +967,9 @@ sfc_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
/*
* Since setting MAC address with filters installed is not
- * allowed on the adapter, one needs to simply restart adapter
- * so that the new MAC address will be taken from an outer
- * storage and set flawlessly by means of sfc_start() call
+ * allowed on the adapter, the new MAC address will be set
+ * by means of adapter restart. sfc_start() shall retrieve
+ * the new address from the device private data and set it.
*/
sfc_stop(sa);
rc = sfc_start(sa);
@@ -972,6 +978,13 @@ sfc_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
}
unlock:
+ /*
+ * In the case of failure sa->port->default_mac_addr does not
+ * need rollback since no error code is returned, and the upper
+ * API will anyway update the external MAC address storage.
+ * To be consistent with that new value it is better to keep
+ * the device private value the same.
+ */
sfc_adapter_unlock(sa);
}
@@ -188,10 +188,10 @@ sfc_port_start(struct sfc_adapter *sa)
goto fail_mac_pdu_set;
if (!port->isolated) {
- struct ether_addr *mac_addrs = sa->eth_dev->data->mac_addrs;
+ struct ether_addr *addr = &port->default_mac_addr;
sfc_log_init(sa, "set MAC address");
- rc = efx_mac_addr_set(sa->nic, mac_addrs[0].addr_bytes);
+ rc = efx_mac_addr_set(sa->nic, addr->addr_bytes);
if (rc != 0)
goto fail_mac_addr_set;
@@ -342,6 +342,8 @@ int
sfc_port_attach(struct sfc_adapter *sa)
{
struct sfc_port *port = &sa->port;
+ const efx_nic_cfg_t *encp = efx_nic_cfg_get(sa->nic);
+ const struct ether_addr *from;
long kvarg_stats_update_period_ms;
int rc;
@@ -353,6 +355,10 @@ sfc_port_attach(struct sfc_adapter *sa)
port->flow_ctrl = EFX_FCNTL_RESPOND | EFX_FCNTL_GENERATE;
port->flow_ctrl_autoneg = B_TRUE;
+ RTE_BUILD_BUG_ON(sizeof(encp->enc_mac_addr) != sizeof(*from));
+ from = (const struct ether_addr *)(encp->enc_mac_addr);
+ ether_addr_copy(from, &port->default_mac_addr);
+
port->max_mcast_addrs = EFX_MAC_MULTICAST_LIST_MAX;
port->nb_mcast_addrs = 0;
port->mcast_addrs = rte_calloc_socket("mcast_addr_list_buf",