From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v2 2/4] devargs: simplify parameters of
	removal function
Date: Thu, 27 Sep 2018 23:31:50 +0200
Message-ID: <4883182.eS8zcPg0Ii@xps>
References: <20180907222727.20521-1-thomas@monjalon.net>
 <20180926214759.1856-3-thomas@monjalon.net>
 <VI1PR0502MB37437AA3D5F7C6131ECADD0CD1140@VI1PR0502MB3743.eurprd05.prod.outlook.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: "dev@dpdk.org" <dev@dpdk.org>,
 "gaetan.rivet@6wind.com" <gaetan.rivet@6wind.com>,
 "qi.z.zhang@intel.com" <qi.z.zhang@intel.com>,
 "ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
 "ktraynor@redhat.com" <ktraynor@redhat.com>,
 Shahaf Shuler <shahafs@mellanox.com>, Olga Shern <olgas@mellanox.com>
To: Ophir Munk <ophirmu@mellanox.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com
 [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id A31791B14F
 for <dev@dpdk.org>; Thu, 27 Sep 2018 23:31:54 +0200 (CEST)
In-Reply-To: <VI1PR0502MB37437AA3D5F7C6131ECADD0CD1140@VI1PR0502MB3743.eurprd05.prod.outlook.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi,

27/09/2018 10:24, Ophir Munk:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > The function rte_devargs_remove(), which is intended to be internal, can
> > take a devargs structure as argument.
> > The matching is still using string comparison of bus name and device name.
> > It is simpler and may allow a different devargs matching in future.
[...]
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -186,7 +186,7 @@ int __rte_experimental rte_eal_hotplug_add(const
> > char *busname, const char *devn
> >  	return 0;
> > 
> >  err_devarg:
> > -	if (rte_devargs_remove(busname, devname)) {
> > +	if (rte_devargs_remove(da)) {
> 
> Can you please explain why calling with 'da' parameter
> (which may have different internal fields of busname and devname
> than the explicit busname and devname parameters) - is correct?

Before devargs is parsed and inserted in the global list,
devargs values are NULL, so nothing is found (as before this patch).
rte_devargs_remove(da) will have no effect and will return 1,
so the devargs will be freed by code in the if block.

When devargs is inserted in the list, it will have the values
of busname and devname, so it is the same as before this patch.

However, there may be an issue dereferencing devargs->bus->name
when devargs is not parsed. I must add this code in rte_devargs_remove:
    if (devargs->bus == NULL)
        return 1;

[...]
> > @@ -227,7 +227,7 @@ rte_eal_hotplug_remove(const char *busname,
> > const char *devname)
> >  	if (ret)
> >  		RTE_LOG(ERR, EAL, "Driver cannot detach the device
> > (%s)\n",
> >  			dev->name);
> > -	rte_devargs_remove(busname, devname);
> > +	rte_devargs_remove(dev->devargs);
> 
> Can you please explain why calling with 'dev->devargs' parameter
> (which may have different internal fields of busname and devname
> than the explicit busname and devname parameters) - is correct?

The device is found by checking the busname and devname.
I don't see how the devargs values may be different.