From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B0A97A0538; Wed, 5 Feb 2020 09:52:42 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EDA1A1C117; Wed, 5 Feb 2020 09:52:41 +0100 (CET) Received: from mail-vs1-f66.google.com (mail-vs1-f66.google.com [209.85.217.66]) by dpdk.org (Postfix) with ESMTP id 0EEBF1C115 for ; Wed, 5 Feb 2020 09:52:41 +0100 (CET) Received: by mail-vs1-f66.google.com with SMTP id p6so816335vsj.11 for ; Wed, 05 Feb 2020 00:52:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=zECMpFQjr1nyNAr+Jd3up6kbyvV+bR8dt5+mQXxIf9U=; b=RCJ3qmaajz1klYrvLPfAGrwCXMOMgPlkjdfthgVP6mgYssc62aZpWchcNIWAfZ08NA SAfSQqCRn+R1I/iF7E/2whYyPcjvG4mHxZqJSrsiP3Nz8Wb87eIHexbNbh4tQK/ylyeM j3cXo5U3mzuDKAlReKrrLm/uLHj6HYBysNnKU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=zECMpFQjr1nyNAr+Jd3up6kbyvV+bR8dt5+mQXxIf9U=; b=nAqaH5sL6uKaf0Yf5IFBzlVne8PLI1MubBgVIEGCwqOmd6qCxXPM3Dz8k7gqkWjDxf 3oRywoVZddhIUlFypAmR8DmSl1nvL8tWY+Q82Hm90aC9PKlOuhLAgJUUyDFfLf4AFHw5 qZxnbGJX/Zp8LnWGbFpVXwsTuzgbTphLNGmxVq/kcG+/19YxKjYrzgKEQ7UvmC0QLr42 g7QEAc02RqBUGe8s6K8NBZ7XrqwgeXTTD7W2GyV56ymQaQRd8q3Tw3HJgebr3IirFVNV mTwzjc9+gAhSIVlYy+9dudtrpYGYEFcwnVOo5KMakWgmzD2fR+z0Vp7AAsjYPcxNhg43 3mnw== X-Gm-Message-State: APjAAAV0YhH448jU864z57osVpt+AsnjGvj09flJMp6thS3K/QbwOJK4 nd4FkIe2OS9y+dLb+U6iYr2G5sKw7O8yMYM3PWc4uIU8CeQ= X-Google-Smtp-Source: APXvYqzIxYV8TzGQy2vmlaaLzhK+kSXMdLf38JC0l2zD+M3S437ccmAcjixmebPcDLZXEVQvyYdwEatxu2qIiEcNoEQ= X-Received: by 2002:a05:6102:38b:: with SMTP id m11mr21370584vsq.187.1580892760096; Wed, 05 Feb 2020 00:52:40 -0800 (PST) MIME-Version: 1.0 References: <20200204091548.2861-1-somnath.kotur@broadcom.com> In-Reply-To: From: Somnath Kotur Date: Wed, 5 Feb 2020 14:22:28 +0530 Message-ID: To: Gaetan Rivet Cc: dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH] eal: fix to set the rte_device ptr's device args before hotplug 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Gaetan, On Tue, Feb 4, 2020 at 3:19 PM Gaetan Rivet wrote: > > On 04/02/2020 10:15, Somnath Kotur wrote: > > As per the comments in this code section, "since there is a matching de= vice, > > it is now its responsibility to manage the devargs we've just inserted.= " > > But the matching device ptr's devargs is still uninitialized or not poi= nting > > to the newest dev_args that were passed as a parameter to local_dev_pro= be(). > > This is needed particularly in the case when *probe is called again* on= an > > already probed device(the parent device for the representor) as part of= adding > > a representor port to an OVS switch(OVS-DPDK) like so: > > ovs-vsctl add-port ovsbr0 vfrep1 -- set Interface vfrep1 type=3Ddpdk \ > > options:dpdk-devargs=3D0000:06:02.0,representor=3D[1] > > > > Fixes: 7e8b26650146 ("eal: fix hotplug add / remove") > > > > Signed-off-by: Somnath Kotur > > --- > > lib/librte_eal/common/eal_common_dev.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/co= mmon/eal_common_dev.c > > index 9e4f09d..311eef5 100644 > > --- a/lib/librte_eal/common/eal_common_dev.c > > +++ b/lib/librte_eal/common/eal_common_dev.c > > @@ -171,6 +171,7 @@ static int cmp_dev_name(const struct rte_device *de= v, const void *_name) > > * those devargs shouldn't be removed manually anymore. > > */ > > > > + dev->devargs =3D da; > > ret =3D dev->bus->plug(dev); > > if (ret > 0) > > ret =3D -ENOTSUP; > > > > Hello Somnath, > > On a surface level, the fix does not seem correct. > The comment > > "Since there is a matching device, it is now its responsibility > to manage the devargs we've just inserted. From this point, > those devargs should'nt be removed manually anymore." > > means that the err_devarg label is not correct, on further error in the > function, returning the error code and cleaning up the device is sufficie= nt. > > Setting the devargs for a device is the responsibility of the bus scan fu= nction. > In the PCI bus for example, this is done in pci_name_set(), called once a= device name is fully qualified > after scanning the system and thus being able to match a devargs to the d= evice name. > > Can you please give more information about the device bus, and maybe trac= e the path taken > by the line "ret =3D da->bus->scan();" a few lines above your edit? If yo= ur dev->devargs is not set > afterward, it seems the bug would be there. > Sure, here is the stack trace from the pci_name_set() pci_name_set (dev=3D0x570eab0) at /root/dpdk-int_nxt/drivers/bus/pci/pci_common.c:65 65 dev->name, sizeof(dev->name)); (gdb) p /x *dev $1 =3D {next =3D {tqe_next =3D 0x0, tqe_prev =3D 0x0}, device =3D {next =3D {tqe_next =3D 0x0, tqe_prev =3D 0x0}, name =3D 0x0, driver =3D 0x0, bus =3D 0x3a59a00, numa_node =3D 0x0, devargs =3D 0x0}, addr =3D {domain =3D 0x0, b= us =3D 0x6, devid =3D 0x2, function =3D 0x1}, id =3D {class_id =3D 0x20000, vendor_id =3D 0x14e4, device_id =3D 0x16dc, subsystem_vendor_id =3D 0x14e4, subsystem_device_id =3D 0x16d7}......... (gdb) bt #0 pci_name_set (dev=3D0x570eab0) at /root/dpdk19.11/drivers/bus/pci/pci_common.c:65 #1 0x0000000000457b69 in pci_scan_one (dirname=3D0x7ffdb46ef230 "/sys/bus/pci/devices/0000:06:02.1", addr=3D0x7ffdb46ef220) at /root/dpdk-19.11/drivers/bus/pci/linux/pci.c:305 #2 0x0000000000458188 in rte_pci_scan () at /root/dpdk-int_nxt/drivers/bus/pci/linux/pci.c:488 #3 0x00000000004b9530 in local_dev_probe (devargs=3D0x527cc10 "0000:06:02.0,representor=3D[1]", new_dev=3D0x7ffdb46f02b8) at /root/dpdk-19.11/lib/librte_eal/common/eal_common_dev.c:158 #4 0x00000000004b9726 in rte_dev_probe (devargs=3D0x527cc10 "0000:06:02.0,representor=3D[1]") at /root/dpdk-int_nxt/lib/librte_eal/common/eal_common_dev.c:227 #5 0x00000000033d196a in netdev_dpdk_process_devargs (dev=3D0x2000e4800, devargs=3D0x527cc10 "0000:06:02.0,representor=3D[1]", errp=3D0x7ffdb46f0478) at lib/netdev-dpdk.c:1798 #6 0x00000000033d1d94 in netdev_dpdk_set_config (netdev=3D0x2000e4880, args=3D0x56edf10, errp=3D0x7ffdb46f0478) at lib/netdev-dpdk.c:1921 #7 0x00000000032c2e48 in netdev_set_config (netdev=3D0x2000e4880, args=3D0x56edf10, errp=3D0x7ffdb46f0598) at lib/netdev.c:494 #8 0x00000000031e9a4e in iface_set_netdev_config (iface_cfg=3D0x56edc90, netdev=3D0x2000e4880, errp=3D0x7ffdb46f0598) at vswitchd/bridge.c:2015 #9 0x00000000031e9bd1 in iface_do_create (br=3D0x52801d0, iface_cfg=3D0x56edc90, ofp_portp=3D0x7ffdb46f05a4, netdevp=3D0x7ffdb46f05a8= , errp=3D0x7ffdb46f0598) at vswitchd/bridge.c:2049 #10 0x00000000031e9da6 in iface_create (br=3D0x52801d0, iface_cfg=3D0x56edc90, port_cfg=3D0x56bae30) at vswitchd/bridge.c:2100 #11 0x00000000031e7494 in bridge_add_ports__ (br=3D0x52801d0, wanted_ports=3D0x52802b0, with_requested_port=3Dfalse) at vswitchd/bridge.c:1164 #12 0x00000000031e7525 in bridge_add_ports (br=3D0x52801d0, wanted_ports=3D0x52802b0) at vswitchd/bridge.c:1180 #13 0x00000000031e6a4f in bridge_reconfigure (ovs_cfg=3D0x52b2c40) at vswitchd/bridge.c:893 #14 0x00000000031ed37c in bridge_run () at vswitchd/bridge.c:3324 #15 0x00000000031f2977 in main (argc=3D11, argv=3D0x7ffdb46f0878) at vswitchd/ovs-vswitchd.c:127 > It could be the bus that is not properly implemented, the devargs name no= t fully qualified, a special path taken by a vdev (I'm not sure what the ov= sbr0 device is for example), or a specific representor pluging function, di= fficult to pinpoint exactly right now. As you can see , the DBDF along with the 'representor=3D[1]' is passed all the way down to local_dev_probe(). However when i traced further inside pci_name_set() =3D>pci_devargs_lookup() static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev) { struct rte_devargs *devargs; struct rte_pci_addr addr; RTE_EAL_DEVARGS_FOREACH("pci", devargs) { devargs->bus->parse(devargs->name, &addr); =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D> pci_parse() [1] if (!rte_pci_addr_cmp(&dev->addr, &addr)) return devargs; } return NULL; } [1]: This is the value of dev_args at this point in time - p *devargs $2 =3D {next =3D {tqe_next =3D 0x0, tqe_prev =3D 0x3a67970 }, type =3D RTE_DEVTYPE_WHITELISTED_PCI, policy =3D RTE_DEV_WHITELISTED, name =3D "0000:06:02.0", '\000' , {args =3D 0x56eec00 "representor=3D[1]", drv_str =3D 0x56eec00 "representor=3D[1]"}, bus =3D 0x3a59a00 , cls =3D 0x0, bus_str =3D 0x0, cls_str = =3D 0x0, data =3D 0x0} It has the 'representor' keyword still intact, but what gets passed to pci_parse() is pci_parse (name=3D0x56d76b8 "0000:06:02.0", addr=3D0x7ffdb46ed180) i.e just the original DBDF of the function that was already probed once during OVS-DPDK init... I'm not sure if that is the reason , but eventually it walks the entire for_each loop and returns NULL ...i.e pci_devargs_lookup() returns NULL ... So this is where the 'devargs' value passed is getting lost Hope that gives what you were looking for ? Thanks Som