Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Sinan Kaya <okaya@kernel.org>]

On 9/25/2018 5:56 PM, Bjorn Helgaas wrote:
>> Use PCI_RESET_LINK when you need link to retrain.
> There are no callers using this.  Is this intended specifically for
> the case of "the hardware wasn't smart enough to train at the correct
> speed, so we fiddled things and want to retrain"?
> 

Yup, I'll go ahead and change the hfi1 driver to use PCI_RESET_LINK
as originally planned so that there is an actual user.

These constants are intended to be used by the user of pci_reset
APIs. I think it makes sense to keep them there as well. I see
your point that PCI_RESET_LINK was not used. We should also make the hfi1
follow the rules.

> Maybe it doesn't need to be exposed in include/linux/pci.h and could
> be defined internally in drivers/pci/pci.c if it's needed there?
> 

see above.

>> Use PCI_RESET_SLOT when you know that this system is hotplug capable
>> by calling probe functions.
> I raise my eyebrows at this because (a) a driver generally can't tell
> whether hotplug is supported and (b) even if the driver does know,
> what is the benefit to the driver to specify this?  What probe
> functions does this refer to?  If I'm a driver writer, I really can't
> tell what I'm supposed to do with this guidance.  If I'm supposed to
> call a probe function, what is it, when should I call it, and what
> should I do with the result?  Am I supposed to know whether hotplug is
> supported?  Why would I care?

That was the whole reason why I hid the secondary bus reset API from the
user and folded into pci_reset_bus() so that PCI core can deal with hotplug
internally and can go to hotplug reset or bus reset internally.

User just calls pci_reset_bus().

I fully agree with your assessment but there are also exceptions like VFIO
where the code finds out which particular devices are part of a slot hierarchy
and for each one it checks that VFIO ownership has been claimed.
(Alex, please correct me if I'm not getting this right. I just read
200 lines of code in VFIO)

		/* Can we do a slot or bus reset or neither? */
		if (!pci_probe_reset_slot(vdev->pdev->slot))
			slot = true;
		else if (pci_probe_reset_bus(vdev->pdev->bus))
			return -ENODEV;

		ret = vfio_pci_for_each_slot_or_bus(vdev->pdev,
						    vfio_pci_fill_devs,
						    &fill, slot);



> 
> I'm sure you have good answers for all these questions; I just don't
> know what they are:)
> 
>> Use PCI_RESET_BUS when you know that this system is not hotplug capable
>> and this endpoint will never be used in such a system.
> How can a driver know this?  And what's the benefit of being specific?

There is really no benefit but there are drivers making assumptions about
the type of platform they want to run like supporting single segment only
etc.

PCI_RESET_BUS gives you direct access to secondary bus reset. If you are
calling this directly, you are responsible for saving and restoring state
like the hfi1 driver and need to ensure that this card will never work
on a hotplug capable system. If not, you are ...ed.

That's why, I'm suggesting that most users will use PCI_RESET_LINK so
that code works on all platforms.

> 
>> I can capture this into the commit message.
> I think it needs to be more accessible, e.g., comments near the constants
> and/or the function declaration.  We shouldn't expect users of the
> interface to dig through the changelogs for it.
> 

I can work on this once we agree if this is the way to go. I was responding
to Alex's request to have a contact between the PCI core and the drivers
because there are some driver that are more intelligent about the PCI tree
than a simple endpoint driver.