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

[Jan Beulich]

>>> On 08.11.17 at 16:44, <govinda.tatti@xxxxxxxxxx> wrote:
> On 11/7/2017 8:40 AM, Jan Beulich wrote:
>>>>> 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.
> Yes, I agree but xen toolstack has already been modified to 
> consume"do_flr" attribute. Hence, we are using the
> function that matches with sysfs attribute.

That's not a valid reason imo: Right now the driver doesn't expose
such an attribute, so if the tool stack was trying to use it, it would
not do what is intended at present anyway (i.e. the code could at
best be called dead). Furthermore, contrary to what you claim in
your reply to Pasi, I can't see where you try an actual FLR first -
you go straight to pci_probe_reset_{slot,bus}(). If you actually
tried FLR first, only falling back to the other methods as "emulation",
I could certainly agree with the file name chosen.

And btw - I don't consider it too good an idea to post the next
version of a patch when discussion of the prior one hasn't really
settled yet.

Jan