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

[Govinda Tatti <govinda.tatti@oracle.com>]

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@Oracle.COM> 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