From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 460935587 for ; Thu, 4 Oct 2018 06:02:47 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2018 21:02:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,338,1534834800"; d="scan'208";a="76058909" Received: from jguo15x-mobl.ccr.corp.intel.com (HELO [10.249.174.2]) ([10.249.174.2]) by fmsmga008.fm.intel.com with ESMTP; 03 Oct 2018 20:58:15 -0700 To: "Burakov, Anatoly" , stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, konstantin.ananyev@intel.com, gaetan.rivet@6wind.com, jingjing.wu@intel.com, thomas@monjalon.net, motih@mellanox.com, matan@mellanox.com, harry.van.haaren@intel.com, qi.z.zhang@intel.com, shaopeng.he@intel.com, bernard.iremonger@intel.com, arybchenko@solarflare.com, wenzhuo.lu@intel.com, jerin.jacob@caviumnetworks.com Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com References: <1498711073-42917-1-git-send-email-jia.guo@intel.com> <1538483726-96411-1-git-send-email-jia.guo@intel.com> <1538483726-96411-5-git-send-email-jia.guo@intel.com> <45ac1ad2-d31e-fa7f-9342-75a3c69fb0ca@intel.com> From: Jeff Guo Message-ID: <284c205a-b315-d0e7-0178-9b13c3141c6e@intel.com> Date: Thu, 4 Oct 2018 11:58:15 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <45ac1ad2-d31e-fa7f-9342-75a3c69fb0ca@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v12 4/7] bus/pci: implement sigbus handler ops 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: Thu, 04 Oct 2018 04:02:47 -0000 On 10/2/2018 10:39 PM, Burakov, Anatoly wrote: > On 02-Oct-18 1:35 PM, Jeff Guo wrote: >> This patch implements the ops for the PCI bus sigbus handler. It >> finds the >> PCI device that is being hot-unplugged and calls the relevant ops of the >> hot-unplug handler to handle the hot-unplug failure of the device. >> >> Signed-off-by: Jeff Guo >> Acked-by: Shaopeng He >> --- >> v12->v11: >> no change. >> --- >>   drivers/bus/pci/pci_common.c | 53 >> ++++++++++++++++++++++++++++++++++++++++++++ >>   1 file changed, 53 insertions(+) >> >> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c >> index d286234..f313fe9 100644 >> --- a/drivers/bus/pci/pci_common.c >> +++ b/drivers/bus/pci/pci_common.c >> @@ -405,6 +405,36 @@ pci_find_device(const struct rte_device *start, >> rte_dev_cmp_t cmp, >>       return NULL; >>   } >>   +/** >> + * find the device which encounter the failure, by iterate over all >> device on >> + * PCI bus to check if the memory failure address is located in the >> range >> + * of the BARs of the device. >> + */ >> +static struct rte_pci_device * >> +pci_find_device_by_addr(const void *failure_addr) >> +{ >> +    struct rte_pci_device *pdev = NULL; >> +    int i; >> + >> +    FOREACH_DEVICE_ON_PCIBUS(pdev) { >> +        for (i = 0; i != RTE_DIM(pdev->mem_resource); i++) { >> +            if ((uint64_t)(uintptr_t)failure_addr >= >> + (uint64_t)(uintptr_t)pdev->mem_resource[i].addr && >> +                (uint64_t)(uintptr_t)failure_addr < >> + (uint64_t)(uintptr_t)pdev->mem_resource[i].addr + >> +                pdev->mem_resource[i].len) { > > You must *really* dislike local variables :) Suggested rewriting: > > const void *start, *end; > size_t len; > > start = pdev->mem_resource[i].addr; > len = pdev->mem_resource[i].len; > end = RTE_PTR_ADD(start, len); > > if (failure_addr >= start && failure_addr < end) { >     ... > } > > I think this is way clearer. > good point, local variable might be good and helpful. Thanks. >> +                RTE_LOG(INFO, EAL, "Failure address " >> +                    "%16.16"PRIx64" belongs to " >> +                    "device %s!\n", >> +                    (uint64_t)(uintptr_t)failure_addr, >> +                    pdev->device.name); > > I feel like this should have DEBUG level, rather than INFO. > Alternatively, if you really feel like this should be at level INFO, > the message should be reworded because the word "failure" might give > the wrong impression :) > > (but really, i think this is info useful for debugging purposes but > not interesting generally, so it should be under DEBUG IMO) > ok, i accept it.