lkml.org 
[lkml]   [2017]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Xen-devel] [PATCH V2] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute
From
Date
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

\
 
 \ /
  Last update: 2017-11-29 16:40    [W:0.052 / U:0.668 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site