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 A67D7A0546; Fri, 14 Feb 2020 07:46:54 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8D2AA1BDAE; Fri, 14 Feb 2020 07:46:54 +0100 (CET) Received: from mail-vk1-f195.google.com (mail-vk1-f195.google.com [209.85.221.195]) by dpdk.org (Postfix) with ESMTP id 7CA131BDAE for ; Fri, 14 Feb 2020 07:46:53 +0100 (CET) Received: by mail-vk1-f195.google.com with SMTP id g7so2308921vkl.12 for ; Thu, 13 Feb 2020 22:46:53 -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; bh=8V1y8J01pfxWrhsaf34hdkarmvPj+TsiUi3W/bzUGJo=; b=UDkm1RZw62oSpdW1B3UtuGrs2fAzKjRk1+98eQtPurm8/D4hKq0G8Rw060y/OoK04W vR5OxzyXrZpvLZmN8YP2SLpE19oxJ2wq7aHsGrlewKFEn9GymyXYhInnL1jAUL3UwIot 6iitPjYzTTTUHFWxU9rnoeovDSTskPszQVr0c= 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; bh=8V1y8J01pfxWrhsaf34hdkarmvPj+TsiUi3W/bzUGJo=; b=JtgswFCZEsYEnwCLCzr3tKbjaJK6QE+R0hX4PS7dRFzoctCCrTD39gz1ogEYpKPwIx 3Mujb8fIXCicmM/9+gbZLGDS9tF4NrDLy+ATwpMOzJrskkygiMG7RBNtfZdzyFXxtU4U dB503gahpAF53//OSNMeNK/67XCWJbt0dDycXCbaZ2ROQq5tjGALslcbZ4ESFDKTTTnJ Slv23xdsXWW+XIPMQQVTf5UNzZHYcvizWNIxB9jv5mjCYL6/cincpl+tJAk6wcYkz+On zyUraasUWL4bsc+2Yg6snuwDhDRaJ8KgMaEBswYUEH7zZhmuHpCKmmQ/ufpO4R0W7fb+ AlCA== X-Gm-Message-State: APjAAAXoahFsf73ICCDNexr+CRKpFc1HTXUr2a6w1i+qP7k+NRp4by7j y7mQuVHjjVoocJ0HqC+9993SEOIqdTTtuF6A3xNvnw== X-Google-Smtp-Source: APXvYqzNxHf1taxcQqTXJnkZpvBqR/yhPYO/XID7VPHgICbAexCg3wx//zExzglNLeXOCq9ptZEGMHIN/S9duI8NT5s= X-Received: by 2002:ac5:c314:: with SMTP id j20mr904276vkk.32.1581662812693; Thu, 13 Feb 2020 22:46:52 -0800 (PST) MIME-Version: 1.0 References: <20200204091548.2861-1-somnath.kotur@broadcom.com> <83f7e3a5-c36f-a755-1c71-0c248fe48d0e@u256.net> <926ec7c7-b23d-267a-ed43-6ef4d7bb916a@u256.net> In-Reply-To: <926ec7c7-b23d-267a-ed43-6ef4d7bb916a@u256.net> From: Somnath Kotur Date: Fri, 14 Feb 2020 12:16:41 +0530 Message-ID: To: Gaetan Rivet Cc: dev Content-Type: text/plain; charset="UTF-8" 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" On Tue, Feb 11, 2020 at 2:08 PM Gaetan Rivet wrote: > > [...] > >>> (gdb) p dev2 > >>> $5 = (struct rte_pci_device *) 0x54de5e0 > >>> (gdb) p /x *dev2 > >>> $6 = {next = {tqe_next = 0x5307460, tqe_prev = 0x5539f80}, device = > >>> {next = {tqe_next = 0x0, tqe_prev = 0x0}, name = 0x54e4f00, driver = > >>> 0x3a6b7f0, > >>> bus = 0x3a59a00, numa_node = 0x0, devargs = 0x0}, addr = {domain = > >>> 0x0, bus = 0x6, devid = 0x2, function = 0x0} > >>> > >>> we hit the pci_device ( 0x54de5e0) that was created during the first > >>> time probe of the parent device ? > >>> Since it is already probed, it goes to line 381 where it does frees > >>> the just allocated 'pci_device' inside pci_scan_one() via 'free(dev)' > >>> > >>> As you can see this pci_device does not have it's devargs set (rightly > >>> so as initially there were no devargs for this device) > >>> > >>> But as shown in the stack trace in my previous reply, when > >>> pci_find_device() walks the rte_pci_devices list , it finds this > >>> earlier probed device (without devargs) > >>> > >> > >> Alright, I think you are right, this is what is happening. > >> The issue IMO, is that the PCI scan is thus hitting an already existing device, but we have > >> missed the case where the new device has more info that the previous one (linked devargs). > >> > > That is correct > > > >>> pci_find_device (start=0x0, cmp=0x4b92d8 , > >>> data=0x55a4af8) at /root/dpdk-19.11/drivers/bus/pci/pci_common.c:426 > >>> 426 return &pdev->device; > >>> (gdb) p pdev > >>> $15 = (struct rte_pci_device *) 0x54de5e0 > >>> > >>> (gdb) p /x *pdev > >>> $16 = {next = {tqe_next = 0x5307460, tqe_prev = 0x5539f80}, device = > >>> {next = {tqe_next = 0x0, tqe_prev = 0x0}, name = 0x54e4f00, driver = > >>> 0x3a6b7f0, > >>> bus = 0x3a59a00, numa_node = 0x0, devargs = 0x0}, addr = {domain = > >>> 0x0, bus = 0x6, devid = 0x2, function = 0x0}, > >>> > >>> Hope this explains why the pci_device has NULL devargs at this point > >>> and how my fix to set it at this point helps solve the problem? > >>> > >>> Please let me know your thoughts > >>> > >> > >> I think the proper fix is instead to have a clean update during the PCI scan. > >> The proper way is to keep the old device, but override its fields as new info was found. > > > > Agreed > >> > >> Something like calling pci_name_set(dev2); line 359 maybe. BSD should also be updated for consistency. > >> > >> My issue with your patch is that I think this issue is very specific to the PCI bus and the capabilities > >> of some devices on it. It would be better to have a fully compliant scan + probe ops considering > >> the supported capabilities, instead of forcing this on all buses. > > Sure, so i'm guessing you meant something like this ? > > > > dex 740a2cd..92a41c2 100644 > > --- a/drivers/bus/pci/linux/pci.c > > +++ b/drivers/bus/pci/linux/pci.c > > @@ -377,6 +377,8 @@ > > */ > > RTE_LOG(ERR, EAL, > > "Unexpected device scan at %s!\n", > > filename); > > + else if (dev2->device.devargs > > != dev->device.devargs) > > + pci_name_set(dev2); > > } > > free(dev); > > } > > > > Which is basically checking for devargs mismatch between the 'new' > > device and a match with an > > already probed device and return the old device while adjusting the > > new info (devargs) > > Is that OK? > > > >> > >> I'm wondering whether this can happen with an already existing devargs? If so, is it more correct to keep > >> the new one, or ignore the probe, free the new devargs and report an error? If the former, > >> please clean up properly the devargs using rte_devargs_remove() (before calling pci_name_set() of course). > > Hello Somnath, > > Yes, this is basically it, however you also need to take care of a potential already existing devargs here, because the PCI bus does not guarantee that all devargs of a device will be named the same Thanks a lot Gaetan, have sent out V2 incorporating your suggestions here.. Please ack > > .