From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH] net/mlx5: allow multi probing Date: Fri, 5 Oct 2018 01:41:59 +0000 Message-ID: <20181005014145.GA5488@mtidpdk.mti.labs.mlnx> References: <1538553677-32010-1-git-send-email-ophirmu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , Asaf Penso , Shahaf Shuler , Thomas Monjalon , Olga Shern To: Ophir Munk Return-path: Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00061.outbound.protection.outlook.com [40.107.0.61]) by dpdk.org (Postfix) with ESMTP id 0CA485F2B for ; Fri, 5 Oct 2018 03:42:02 +0200 (CEST) In-Reply-To: <1538553677-32010-1-git-send-email-ophirmu@mellanox.com> Content-Language: en-US Content-ID: <688C597A8772EE41B2FAEBF1C1BB65F8@eurprd05.prod.outlook.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Oct 03, 2018 at 01:01:23AM -0700, Ophir Munk wrote: > Implement probing of a device multiple times, see [1]. > Consecutive probing requests with a devargs string may contain > repetitive master and representors devices for which eth device should > be created. If an eth device already exists - silently ignore it. >=20 > [1] > Serie: ("eal: allow hotplug to skip an already probed device") A typo? Series? For the title, either 'multi-probing' or 'multiple probing'. And I suggest net/mlx5: allow multiple probing for representor And I have only nitpickings below ;-) > https://patches.dpdk.org/project/dpdk/list/?series=3D1580 >=20 > Signed-off-by: Ophir Munk > --- > drivers/net/mlx5/mlx5.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) >=20 > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c > index b2b0aaa..16a8b9d 100644 > --- a/drivers/net/mlx5/mlx5.c > +++ b/drivers/net/mlx5/mlx5.c > @@ -736,6 +736,7 @@ > struct ether_addr mac; > char name[RTE_ETH_NAME_MAX_LEN]; > int own_domain_id =3D 0; > + uint16_t port_id; > unsigned int i; > =20 > /* Determine if this port representor is supposed to be spawned. */ > @@ -758,6 +759,17 @@ > return NULL; > } > } > + /* Build device name */ Better to put a period (.), if it is a sentence. > + if (!switch_info->representor) > + rte_strlcpy(name, dpdk_dev->name, sizeof(name)); > + else > + snprintf(name, sizeof(name), "%s_representor_%u", > + dpdk_dev->name, switch_info->port_name); > + /* if dev (master or representor) is already spawned - return NULL */ How about, /* Check if the device is already spawned. */ "return NULL" is so obvious from the code. > + if (rte_eth_dev_get_port_by_name(name, &port_id) =3D=3D 0) { > + rte_errno =3D EBUSY; Semantically, shouldn't it be EEXIST and add the condition in probe()? > + return NULL; > + } > /* Prepare shared data between primary and secondary process. */ > mlx5_prepare_shared_data(); > errno =3D 0; > @@ -864,11 +876,6 @@ > DEBUG("ibv_query_device_ex() failed"); > goto error; > } > - if (!switch_info->representor) > - rte_strlcpy(name, dpdk_dev->name, sizeof(name)); > - else > - snprintf(name, sizeof(name), "%s_representor_%u", > - dpdk_dev->name, switch_info->port_name); > DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name); > if (rte_eal_process_type() =3D=3D RTE_PROC_SECONDARY) { > eth_dev =3D rte_eth_dev_attach_secondary(name); > @@ -1298,9 +1305,6 @@ struct mlx5_dev_spawn_data { > assert(pci_drv =3D=3D &mlx5_driver); > errno =3D 0; > =20 > - if (rte_dev_is_probed(&pci_dev->device)) > - return -EEXIST; > - > ibv_list =3D mlx5_glue->get_device_list(&ret); > if (!ibv_list) { > rte_errno =3D errno ? errno : ENOSYS; > @@ -1412,7 +1416,7 @@ struct mlx5_dev_spawn_data { > if (!list[i].eth_dev) { > if (rte_errno !=3D EBUSY) I meant this to be, if (rte_errno !=3D EBUSY && rte_errno !=3D EEXIST) Thanks, Yongseok > break; > - /* Device is disabled, ignore it. */ > + /* Device is disabled or already spawned. Ignore it. */ > continue; > } > restore =3D list[i].eth_dev->data->dev_flags; > --=20 > 1.8.3.1 >=20