Received: by 10.223.164.202 with SMTP id h10csp4521903wrb; Wed, 29 Nov 2017 07:38:53 -0800 (PST) X-Google-Smtp-Source: AGs4zMbBTa0SpJa7eM46cTf4CqowxUXpll71noCxZfbP/kbAxP2S5zcbJvV6KPELIvsX4HLBX030 X-Received: by 10.99.183.15 with SMTP id t15mr3286591pgf.128.1511969933614; Wed, 29 Nov 2017 07:38:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1511969933; cv=none; d=google.com; s=arc-20160816; b=TlhziAshDPo/vn5ZUbwcPhJ2fo2db9pGf+wJKlhKc5fw5BwEIstJwYvQ5QoGfqLnpG R4Ltqk/PadQBfkiSRr+2SzVXfLKgvj7ScRd+6oqEr+jcWuVlCbrKYGnDN5usgbPBhgvF TimP3LsicEMN/BiQGwNrH8ynzq1gJOiEiUjzV/Jq6SuNlgiLze3g7hDqxgWUgePxU3Eh /9YayjmEZOOgC65dE/I6Q29VBpMz8Kt2fzj+zFHrSDj1hqDzFlwvc0CgZDJ0XB7Jo8pE WbgEpaX+O0xgY+X2R9hQzZXAZ0d9z0eLKfR2JwW0qYy14IAH6ZXqPi7qcM75+q9Eqj32 5IcQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-language :content-transfer-encoding:in-reply-to:mime-version:user-agent:date :message-id:organization:from:references:cc:to:subject :arc-authentication-results; bh=0X504i+8cFMxT/sKnidxbcvLM5m0YH+e+NncHZHGtT8=; b=ebWDVc4fpBJs7WSBcNHshYeIqZnauSjFIjYZL8etAnNrrv71fjzt8BvKHAL/d5vI+9 2Wo9KnYeM+U3513WafFBedu+oxronyEx2TvcOIQMuHPh//fedx1wt/sGjd73P3mP1HgJ kEg1lHq6HXg8F+Xc5/6wS5MJd8BJfbAiyPd8uCmPe7yh+tH5pneHllNdFVHJI+do7mcJ ymQFWrOaRkoJFfN7pqGdyUQKhpRey7F8gqI92wHdUFjSaxPRT3yqrnW7OQMLoOwYJgZP B4DOJDZh3p2swNEjsFdWtqJMnCRlYdUygTXu7PzhO00L6qfclmtC76Hc91Z0BPi7OYvO HwmQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=oracle.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id e1si1427915pld.824.2017.11.29.07.38.43; Wed, 29 Nov 2017 07:38:53 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=oracle.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933396AbdK2Ph1 (ORCPT + 69 others); Wed, 29 Nov 2017 10:37:27 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:26513 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933180AbdK2PhY (ORCPT ); Wed, 29 Nov 2017 10:37:24 -0500 Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id vATFbH55003902 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 29 Nov 2017 15:37:18 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id vATFbHJI026529 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 29 Nov 2017 15:37:17 GMT Received: from abhmp0003.oracle.com (abhmp0003.oracle.com [141.146.116.9]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id vATFbG4S023248; Wed, 29 Nov 2017 15:37:16 GMT Received: from [10.135.190.159] (/10.135.190.159) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 29 Nov 2017 07:37:16 -0800 Subject: Re: [Xen-devel] [PATCH V2] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute To: Jan Beulich Cc: Juergen Gross , konrad.wilk@oracle.com, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, boris.ostrovsky@oracle.com, roger.pau@citrix.com References: <20171108230654.2981-1-Govinda.Tatti@Oracle.COM> <5A0424B7020000780018D6FA@prv-mh.provo.novell.com> From: Govinda Tatti Organization: Oracle Corporation Message-ID: Date: Wed, 29 Nov 2017 09:37:13 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <5A0424B7020000780018D6FA@prv-mh.provo.novell.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Jan, Please see below for my comments. On 11/9/2017 2:49 AM, Jan Beulich wrote: >>>> On 09.11.17 at 00:06, wrote: >> --- a/drivers/xen/xen-pciback/pci_stub.c >> +++ b/drivers/xen/xen-pciback/pci_stub.c >> @@ -244,6 +244,91 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev, >> return found_dev; >> } >> >> +struct pcistub_args { >> + struct pci_dev *dev; > Please don't ignore prior review comments: Either carry out what > was requested, or explain why the request can't be fulfilled. You > saying "This field will point to first device that is not owned by > pcistub" to Roger's request to make this a pointer to const is not a > valid reason to not do the adjustment; in fact your reply is entirely > unrelated to the request. We can use "const" with above field since we will set this field only once. I will change it. > >> +static int pcistub_search_dev(struct pci_dev *dev, void *data) >> +{ >> + struct pcistub_device *psdev; >> + struct pcistub_args *arg = data; >> + bool found_dev = false; > Purely cosmetical, but anyway: Why not just "found"? What else > could be (not) found here other than the device in question? Sure, I will change it to "found". > >> + unsigned long flags; >> + >> + spin_lock_irqsave(&pcistub_devices_lock, flags); >> + >> + list_for_each_entry(psdev, &pcistub_devices, dev_list) { >> + if (psdev->dev == dev) { >> + found_dev = true; >> + arg->dcount++; >> + break; >> + } >> + } >> + >> + spin_unlock_irqrestore(&pcistub_devices_lock, flags); >> + >> + /* Device not owned by pcistub, someone owns it. Abort the walk */ >> + if (!found_dev) >> + arg->dev = dev; >> + >> + return found_dev ? 0 : 1; > Despite the function needing to return int, this can be simplified to > "return !found_dev". I'd also like to note that the part of the > earlier comment related to this is sort of disconnected. How about > > /* Device not owned by pcistub, someone owns it. Abort the walk */ > if (!found_dev) { > arg->dev = dev; > return 1; > } > > return 0; Fine with me. > > And finally - I don't think the comment is entirely correct - the > device not being owned by pciback doesn't necessarily mean it's > owned by another driver. It could as well be unowned. OK. I will modify this comment as " Device not owned by pcistub. Abort the walk". > >> +static int pcistub_reset_dev(struct pci_dev *dev) >> +{ >> + struct xen_pcibk_dev_data *dev_data; >> + bool slot = false, bus = false; >> + struct pcistub_args arg = {}; >> + >> + if (!dev) >> + return -EINVAL; >> + >> + dev_dbg(&dev->dev, "[%s]\n", __func__); >> + >> + if (!pci_probe_reset_slot(dev->slot)) >> + slot = true; >> + else if ((!pci_probe_reset_bus(dev->bus)) && >> + (!pci_is_root_bus(dev->bus))) >> + bus = true; >> + >> + if (!bus && !slot) >> + return -EOPNOTSUPP; >> + >> + /* >> + * Make sure all devices on this bus are owned by the >> + * PCI backend so that we can safely reset the whole bus. >> + */ > Is that really the case when you mean to do a slot reset? It was for > a reason that I had asked about a missing "else" in v1 review, > rather than questioning the conditional around the logic. In the case of bus or slot reset, our goal is to reset connected PCIe fabric/card/endpoint. The connected card/endpoint can be multi-function device. So, same walk-through and checking is needed irrespective of type of reset being used. > >> + pci_walk_bus(dev->bus, pcistub_search_dev, &arg); >> + >> + /* All devices under the bus should be part of pcistub! */ >> + if (arg.dev) { >> + dev_err(&dev->dev, "%s device on bus 0x%x is not owned by pcistub\n", > %#x > > Yet then, thinking about what would be useful information should the > situation really arise, I'm not convinced printing a bare bus number > here is useful either. Especially for the case of multiple parallel > requests you want to make it possible to match each message to the > original request (guest start or whatever). Hence I think you want > something like > > "%s on the same bus as %s is not owned by " DRV_NAME "\n" Sure, I will make this change. > >> + pci_name(arg.dev), dev->bus->number); >> + >> + return -EBUSY; >> + } >> + >> + dev_dbg(&dev->dev, "pcistub owns %d devices on bus 0x%x\n", >> + arg.dcount, dev->bus->number); > While here the original device is perhaps not necessary to print, > the bare bus number doesn't carry enough information: You'll > want to prefix it by the segment number. Plus you'll want to use > canonical formatting (ssss:bb), so one can get matches when > suitably grep-ing the log. Perhaps bus->name is what you're > after. Sure, I can change it to dev_dbg(&dev->dev, "pcistub owns %d devices on PCI Bus %04x:%02x", pci_domain_nr(bus), bus->number); Cheers GOVINDA From 1583577789759040346@xxx Thu Nov 09 08:51:06 +0000 2017 X-GM-THRID: 1583541656865711781 X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread