Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.11.17 at 18:48, <Govinda.Tatti@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx 
> +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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel