From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 07EE05F12 for ; Fri, 16 Mar 2018 16:42:03 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 901B5B40062; Fri, 16 Mar 2018 15:42:00 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Fri, 16 Mar 2018 15:41:55 +0000 To: Olivier Matz , CC: Thomas Monjalon , Ferruh Yigit , Ivan Malov References: <20180227151129.30387-1-olivier.matz@6wind.com> From: Andrew Rybchenko Message-ID: <749ef7af-dde0-529e-f80c-e0b76408cd4c@solarflare.com> Date: Fri, 16 Mar 2018 18:41:51 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180227151129.30387-1-olivier.matz@6wind.com> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23722.003 X-TM-AS-Result: No--26.496500-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1521214921-cF5PhMu-W8aX Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] ethdev: return diagnostic when setting MAC address X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Mar 2018 15:42:03 -0000 Hi Olivier, On 02/27/2018 06:11 PM, Olivier Matz wrote: > Change the prototype and the behavior of dev_ops->eth_mac_addr_set(): a > return code is added to notify the caller (librte_ether) if an error > occurred in the PMD. > > The new default MAC address is now copied in dev->data->mac_addrs[0] > only if the operation is successful. > > The patch also updates all the PMDs accordingly. > > Signed-off-by: Olivier Matz > --- > > Hi, > > This patch is the following of the discussion we had in this thread: > https://dpdk.org/dev/patchwork/patch/32284/ > > I did my best to keep the consistency inside the PMDs. The behavior > of eth_mac_addr_set() is inspired from other fonctions in the same > PMD, usually eth_mac_addr_add(). For instance: > - dpaa and dpaa2 return 0 on error. > - some PMDs (bnxt, mlx5, ...?) do not return a -errno code (-1 or > positive values). > - some PMDs (avf, tap) check if the address is the same and return 0 > in that case. This could go in generic code? > > I tried to use the following errors when relevant: > - -EPERM when a VF is not allowed to do a change > - -ENOTSUP if the function is not supported > - -EIO if this is an unknown error from lower layer (hw or sdk) > - -EINVAL for other unknown errors > > Please, PMD maintainers, feel free to comment if you ahve specific > needs for your driver. > > Thanks > Olivier > > > doc/guides/rel_notes/deprecation.rst | 8 -------- > drivers/net/ark/ark_ethdev.c | 9 ++++++--- > drivers/net/avf/avf_ethdev.c | 12 ++++++++---- > drivers/net/bnxt/bnxt_ethdev.c | 10 ++++++---- > drivers/net/bonding/rte_eth_bond_pmd.c | 8 ++++++-- > drivers/net/dpaa/dpaa_ethdev.c | 4 +++- > drivers/net/dpaa2/dpaa2_ethdev.c | 6 ++++-- > drivers/net/e1000/igb_ethdev.c | 12 +++++++----- > drivers/net/failsafe/failsafe_ops.c | 16 +++++++++++++--- > drivers/net/i40e/i40e_ethdev.c | 24 ++++++++++++++--------- > drivers/net/i40e/i40e_ethdev_vf.c | 12 +++++++----- > drivers/net/ixgbe/ixgbe_ethdev.c | 13 ++++++++----- > drivers/net/mlx4/mlx4.h | 2 +- > drivers/net/mlx4/mlx4_ethdev.c | 7 +++++-- > drivers/net/mlx5/mlx5.h | 2 +- > drivers/net/mlx5/mlx5_mac.c | 7 +++++-- > drivers/net/mrvl/mrvl_ethdev.c | 7 ++++++- > drivers/net/null/rte_eth_null.c | 3 ++- > drivers/net/octeontx/octeontx_ethdev.c | 4 +++- > drivers/net/qede/qede_ethdev.c | 7 +++---- > drivers/net/sfc/sfc_ethdev.c | 14 +++++--------- > drivers/net/szedata2/rte_eth_szedata2.c | 3 ++- > drivers/net/tap/rte_eth_tap.c | 34 +++++++++++++++++++++------------ > drivers/net/virtio/virtio_ethdev.c | 15 ++++++++++----- > drivers/net/vmxnet3/vmxnet3_ethdev.c | 5 +++-- > lib/librte_ether/rte_ethdev.c | 7 +++++-- > lib/librte_ether/rte_ethdev_core.h | 2 +- > test/test/virtual_pmd.c | 3 ++- > 28 files changed, 159 insertions(+), 97 deletions(-) > diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c > index 89a452907..b76d4bb2c 100644 > --- a/drivers/net/sfc/sfc_ethdev.c > +++ b/drivers/net/sfc/sfc_ethdev.c > @@ -920,7 +920,7 @@ sfc_dev_set_mtu(struct rte_eth_dev *dev, uint16_t mtu) > SFC_ASSERT(rc > 0); > return -rc; > } > -static void > +static int > sfc_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) > { > struct sfc_adapter *sa = dev->data->dev_private; > @@ -939,13 +939,14 @@ sfc_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) > if (port->isolated) { > sfc_err(sa, "isolated mode is active on the port"); > sfc_err(sa, "will not set MAC address"); > + rc = ENOTSUP; > goto unlock; > } > > if (sa->state != SFC_ADAPTER_STARTED) { > sfc_info(sa, "the port is not started"); > sfc_info(sa, "the new MAC address will be set on port start"); > - > + rc = EINVAL; > goto unlock; > } > > @@ -982,14 +983,9 @@ 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); > + SFC_ASSERT(rc >= 0); > + return -rc; > } > > It is a bit more complicated for sfc. Please, find the patch from Ivan below:  - it is not an error if isolated mode - we just need to save a new value locally  - it requires careful rollback in the case of failure === diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index 89a4529..f5670a9 100644 --- a/drivers/net/sfc/sfc_ethdev.c +++ b/drivers/net/sfc/sfc_ethdev.c @@ -920,13 +920,14 @@         SFC_ASSERT(rc > 0);         return -rc;  } -static void +static int  sfc_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)  {         struct sfc_adapter *sa = dev->data->dev_private;         const efx_nic_cfg_t *encp = efx_nic_cfg_get(sa->nic);         struct sfc_port *port = &sa->port; -       int rc; +       struct ether_addr *old_addr = &dev->data->mac_addrs[0]; +       int rc = 0;         sfc_adapter_lock(sa); @@ -936,9 +937,16 @@          */         ether_addr_copy(mac_addr, &port->default_mac_addr); +       /* +        * Neither of the two following checks can return +        * an error. The new MAC address is preserved in +        * the device private data and can be activated +        * on the next port start if the user prevents +        * isolated mode from being enabled. +        */         if (port->isolated) { -               sfc_err(sa, "isolated mode is active on the port"); -               sfc_err(sa, "will not set MAC address"); +               sfc_warn(sa, "isolated mode is active on the port"); +               sfc_warn(sa, "will not set MAC address");                 goto unlock;         } @@ -962,8 +970,13 @@                  * we also need to update unicast filters                  */                 rc = sfc_set_rx_mode(sa); -               if (rc != 0) +               if (rc != 0) {                         sfc_err(sa, "cannot set filter (rc = %u)", rc); + +                       /* Rollback the old address */ +                       (void)efx_mac_addr_set(sa->nic, old_addr->addr_bytes); +                       (void)sfc_set_rx_mode(sa); +               }         } else {                 sfc_warn(sa, "cannot set MAC address with filters installed");                 sfc_warn(sa, "adapter will be restarted to pick the new MAC"); @@ -982,14 +995,13 @@         }  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. -        */ +       if (rc != 0) +               ether_addr_copy(old_addr, &port->default_mac_addr); +         sfc_adapter_unlock(sa); + +       SFC_ASSERT(rc >= 0); +       return -rc;  } ===