On Tue, Sep 25, 2018 at 05:15:52PM -0400, Sinan Kaya wrote:
> On 9/25/2018 4:54 PM, Bjorn Helgaas wrote:
> > On Wed, Sep 19, 2018 at 04:30:37PM +0000, Sinan Kaya wrote:
> > > Looking to have more control between the users of the API vs. what the API
> > > can do internally. The new reset_type tells the PCI core about the bounds
> > > of the request.
> > >
> > > Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxx>
> > > ---
> > > drivers/pci/pci.c | 18 +++++++++++++++---
> > > drivers/vfio/pci/vfio_pci.c | 6 ++++--
> > > include/linux/pci.h | 2 +-
> > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b4f14419bd51..f11d29f32119 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)
> > > /**
> > > * pci_reset_bus - Try to reset a PCI bus
> > > * @pdev: top level PCI device to reset via slot/bus
> > > + * @reset_type: resets to try
> > > *
> > > * Same as above except return -EAGAIN if the bus cannot be locked
> > > */
> > > -int pci_reset_bus(struct pci_dev *pdev)
> > > +int pci_reset_bus(struct pci_dev *pdev, u32 reset_type)
> > > {
> > > - return (!pci_probe_reset_slot(pdev->slot)) ?
> > > - __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
> > > + if (reset_type & PCI_RESET_LINK) {
> > > + return (!pci_probe_reset_slot(pdev->slot)) ?
> > > + __pci_reset_slot(pdev->slot) :
> > > + __pci_reset_bus(pdev->bus);
> > > + }
> > > +
> > > + if (reset_type & PCI_RESET_BUS)
> > > + return __pci_reset_bus(pdev->bus);
> > > +
> > > + if (reset_type & PCI_RESET_SLOT)
> > > + return __pci_reset_slot(pdev->slot);
> >
> > I don't understand this code. We have
> >
> > #define PCI_RESET_LINK (PCI_RESET_SLOT | PCI_RESET_BUS)
> >
> > so if either PCI_RESET_BUS or PCI_RESET_SLOT is set, we should take
> > the first "if" above, which always returns, and the last two should
> > never be reached. What am I missing?
>
> Yup, this is broken. I need to follow up with another patchset.
>
> I was trying to cover the case where user said,
>
> "I need a link reset. I don't care whether it is a slot reset or bus reset
> as long as we achieve a reset."
>
> That's when PCI_RESET_LINK is used. We expect most drivers to PCI_RESET_LINK
> if they are interested in a link reset and doesn't need to know about the
> hotplug capability of a particular slot.
>
> >
> > > +
> > > + return -EINVAL;
> > > }
> > > EXPORT_SYMBOL_GPL(pci_reset_bus);
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index fe7ada997c51..0e80c72b1eaa 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data,
> > > &info, slot);
> > > if (!ret)
> > > /* User has access, do the reset */
> > > - ret = pci_reset_bus(vdev->pdev);
> > > + ret = pci_reset_bus(vdev->pdev,
> > > + slot ? PCI_RESET_SLOT : PCI_RESET_BUS);
> >
> > This is the only place in the whole series where a caller uses
> > something other than PCI_RESET_ANY. Inquiring minds want to know why.
> > I'm sure it's the right thing to do, it's just that we'll need to know
> > how to choose in future cases.
>
> Use PCI_RESET_ANY for the existing behavior where you want one type of
> reset and don't care if it is PM/function/slot/bus/specific.
>
> 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"?
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?
> 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?
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?
> 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.
> > > hot_reset_release:
> > > for (i--; i >= 0; i--)
> > > @@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
> > > }
> > > if (needs_reset)
> > > - ret = pci_reset_bus(vdev->pdev);
> > > + ret = pci_reset_bus(vdev->pdev,
> > > + slot ? PCI_RESET_SLOT : PCI_RESET_BUS);
> > > put_devs:
> > > for (i = 0; i < devs.cur_index; i++) {
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 4fdddcb85066..85f48e156753 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
> > > int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
> > > int pci_probe_reset_slot(struct pci_slot *slot);
> > > int pci_probe_reset_bus(struct pci_bus *bus);
> > > -int pci_reset_bus(struct pci_dev *dev);
> > > +int pci_reset_bus(struct pci_dev *dev, u32 reset_type);
> > > void pci_reset_secondary_bus(struct pci_dev *dev);
> > > void pcibios_reset_secondary_bus(struct pci_dev *dev);
> > > void pci_update_resource(struct pci_dev *dev, int resno);
> > > --
> > > 2.18.0
> > >
> >
>
