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

[Govinda Tatti]

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