[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH V2] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute
Jan,
Please see below for my comments.
On 11/9/2017 2:49 AM, Jan Beulich wrote:
On 09.11.17 at 00:06, <Govinda.Tatti@xxxxxxxxxx> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
