linux-kernel - Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 07 Nov 2017 07:40:35 -0700
From:   "Jan Beulich" <JBeulich@...e.com>
To:     "Govinda Tatti" <Govinda.Tatti@...cle.COM>,
        <konrad.wilk@...cle.COM>
Cc:     <xen-devel@...ts.xenproject.org>, <boris.ostrovsky@...cle.COM>,
        "Juergen Gross" <jgross@...e.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus
 reset with 'do_flr' SysFS attribute

>>> On 06.11.17 at 18:48, <Govinda.Tatti@...cle.COM> 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@...ts.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.

> +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.

> +		/*
> +		 * 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.

> +			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.

Jan