From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933398AbdK2Ph1 (ORCPT ); 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 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 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