From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752649AbdKHPpS (ORCPT ); Wed, 8 Nov 2017 10:45:18 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:31567 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752491AbdKHPpR (ORCPT ); Wed, 8 Nov 2017 10:45:17 -0500 Subject: Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute To: Jan Beulich , Konrad Wilk Cc: xen-devel@lists.xenproject.org, Boris Ostrovsky , Juergen Gross , linux-kernel@vger.kernel.org References: <20171106174842.20276-1-Govinda.Tatti@Oracle.COM> <5A01D3F3020000780018CE86@prv-mh.provo.novell.com> From: Govinda Tatti Organization: Oracle Corporation Message-ID: <8940b38d-715c-9fcb-cc74-46574d416703@oracle.com> Date: Wed, 8 Nov 2017 09:44:48 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <5A01D3F3020000780018CE86@prv-mh.provo.novell.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks Jan for your review comments. Please see below for my comments. On 11/7/2017 8:40 AM, Jan Beulich wrote: >>>> On 06.11.17 at 18:48, wrote: >> --- a/Documentation/ABI/testing/sysfs-driver-pciback >> +++ b/Documentation/ABI/testing/sysfs-driver-pciback >> @@ -11,3 +11,15 @@ Description: >> #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks >> will allow the guest to read and write to the configuration >> register 0x0E. >> + >> +What: /sys/bus/pci/drivers/pciback/do_flr >> +Date: Nov 2017 >> +KernelVersion: 4.15 >> +Contact: xen-devel@lists.xenproject.org >> +Description: >> + An option to perform a slot or bus reset when a PCI device >> + is owned by Xen PCI backend. Writing a string of DDDD:BB:DD.F >> + will cause the pciback driver to perform a slot or bus reset >> + if the device supports it. It also checks to make sure that >> + all of the devices under the bridge are owned by Xen PCI >> + backend. > Why do you name this "do_flr" when you don't even try FLR, but > go to slot or then bus reset right away. Yes, I agree but xen toolstack has already been modified to consume"do_flr" attribute. Hence, we are using the function that matches with sysfs attribute. > >> +static int pcistub_reset_dev(struct pci_dev *dev) >> +{ >> + struct xen_pcibk_dev_data *dev_data; >> + bool slot = false, bus = false; >> + >> + 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)) { >> + /* We won't attempt to reset a root bridge. */ >> + if (!pci_is_root_bus(dev->bus)) >> + bus = true; >> + } >> + >> + if (!bus && !slot) >> + return -EOPNOTSUPP; >> + >> + if (!slot) { >> + struct pcistub_args arg = { .dev = NULL, .dcount = 0 }; > Neither of the two initializers is really needed - just {} will do. OK. > >> + /* >> + * Make sure all devices on this bus are owned by the >> + * PCI backend so that we can safely reset the whole bus. >> + */ >> + 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 the bus is not owned by pcistub\n", >> + pci_name(arg.dev)); > I think "device" is superfluous here, while "the bus" could do with > replacing by something actually identifying the bus. I assume you want "bus" number to be printed in above msg. If yes, will do. > >> + return -EBUSY; >> + } >> + >> + dev_dbg(&dev->dev, "pcistub owns %d devices on the bus\n", >> + arg.dcount); > Same here for "the bus", provided this log message is useful in the > first place. > >> + } > Aren't you missing an "else" here? Aiui in the "slot" case it may > still be multiple devices/functions which are affected. Good catch.  Our original code was performing same check, both for bus and slot case but somehow I removed it during internal review based on some comment. I will post revised patch later this week. Thanks. Cheers GOVINDA