On 21/06/17 06:13PM, Bjorn Helgaas wrote:
> "Add new" in subject and below is slightly redundant.
>
> On Tue, Jun 08, 2021 at 11:18:51AM +0530, Amey Narkhede wrote:
> > Introduce a new array reset_methods in struct pci_dev to keep track of
> > reset mechanisms supported by the device and their ordering.
> > Also refactor probing and reset functions to take advantage of calling
> > convention of reset functions.
> >
> > Reviewed-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Reviewed-by: Raphael Norwitz <raphael.norwitz@xxxxxxxxxxx>
> > Co-developed-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@xxxxxxxxx>
> > ---
> > drivers/pci/pci.c | 108 ++++++++++++++++++++++++++------------------
> > drivers/pci/pci.h | 8 +++-
> > drivers/pci/probe.c | 5 +-
> > include/linux/pci.h | 7 +++
> > 4 files changed, 81 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 3bf36924c..39a9ea8bb 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -72,6 +72,14 @@ static void pci_dev_d3_sleep(struct pci_dev *dev)
> > msleep(delay);
> > }
> >
> > +bool pci_reset_supported(struct pci_dev *dev)
> > +{
> > + u8 null_reset_methods[PCI_RESET_METHODS_NUM] = { 0 };
> > +
> > + return memcmp(null_reset_methods,
> > + dev->reset_methods, PCI_RESET_METHODS_NUM);
>
> memcmp() doesn't actually return a bool. Either just return int
> and rely on the C "anything non-zero is true, zero is false" or
> convert the memcmp result to bool, i.e., something like:
>
> if (memcmp(...) == 0)
> return true;
> return false;
>
> > +}
> > +
> > #ifdef CONFIG_PCI_DOMAINS
> > int pci_domains_supported = 1;
> > #endif
> > @@ -5107,6 +5115,18 @@ static void pci_dev_restore(struct pci_dev *dev)
> > err_handler->reset_done(dev);
> > }
> >
> > +/*
> > + * The ordering for functions in pci_reset_fn_methods is required for
> > + * reset_methods byte array defined in struct pci_dev.
>
> I'm not quite sure what this comment is telling me. What breaks if I
> change the order? If I add a new method, how do I know where to put
> it?
>
> By reading the code, I infer that:
>
> - Each dev has dev->reset_methods[PCI_RESET_METHODS_NUM]
>
> - dev->reset_methods[i] corresponds to pci_reset_fn_methods[i]
>
> - dev->reset_methods[i] == 0 means dev doesn't support that method
>
> - Otherwise, dev->reset_methods[i] is a value in the range of
> [1, PCI_RESET_METHODS_NUM], and the higher the number, the higher
> the reset method priority
>
> - The order in pci_reset_fn_methods[] determines the initial
> priority via pci_init_reset_methods(), but the priority can be
> changed via sysfs
>
Correct. I agree the comment is not clear. Adding new reset method won't break
anything unless default order is changed and user has some assumptions from
previous versions of kernel.
> > + */
> > +const struct pci_reset_fn_method pci_reset_fn_methods[] = {
> > + { &pci_dev_specific_reset, .name = "device_specific" },
> > + { &pcie_reset_flr, .name = "flr" },
> > + { &pci_af_flr, .name = "af_flr" },
> > + { &pci_pm_reset, .name = "pm" },
> > + { &pci_reset_bus_function, .name = "bus" },
> > +};
> > +
> > /**
> > * __pci_reset_function_locked - reset a PCI device function while holding
> > * the @dev mutex lock.
> > @@ -5129,65 +5149,67 @@ static void pci_dev_restore(struct pci_dev *dev)
> > */
> > int __pci_reset_function_locked(struct pci_dev *dev)
> > {
> > - int rc;
> > + int i, rc = -ENOTTY;
> > + u8 prio;
> >
> > might_sleep();
> >
> > - /*
> > - * A reset method returns -ENOTTY if it doesn't support this device
> > - * and we should try the next method.
> > - *
> > - * If it returns 0 (success), we're finished. If it returns any
> > - * other error, we're also finished: this indicates that further
> > - * reset mechanisms might be broken on the device.
> > - */
> > - rc = pci_dev_specific_reset(dev, 0);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - rc = pcie_reset_flr(dev, 0);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - rc = pci_af_flr(dev, 0);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - rc = pci_pm_reset(dev, 0);
> > - if (rc != -ENOTTY)
> > - return rc;
> > - return pci_reset_bus_function(dev, 0);
> > + for (prio = PCI_RESET_METHODS_NUM; prio; prio--) {
> > + for (i = 0; i < PCI_RESET_METHODS_NUM; i++) {
> > + if (dev->reset_methods[i] == prio) {
> > + /*
> > + * A reset method returns -ENOTTY if it doesn't
> > + * support this device and we should try the
> > + * next method.
> > + *
> > + * If it returns 0 (success), we're finished.
> > + * If it returns any other error, we're also
> > + * finished: this indicates that further reset
> > + * mechanisms might be broken on the device.
> > + */
> > + rc = pci_reset_fn_methods[i].reset_fn(dev, 0);
> > + if (rc != -ENOTTY)
> > + return rc;
>
> Maybe leave the comment outside the loop where it used to be so the
> text lines are longer and it's easier to read.
>
> > + break;
> > + }
> > + }
> > + if (i == PCI_RESET_METHODS_NUM)
> > + break;
> > + }
> > + return rc;
>
> I wonder if this would be easier if dev->reset_methods[] contained
> indices into pci_reset_fn_methods[], highest priority first, with the
> priority being determined when dev->reset_methods[] is updated. For
> example:
>
> const struct pci_reset_fn_method pci_reset_fn_methods[] = {
> { }, # 0
> { &pci_dev_specific_reset, .name = "device_specific" }, # 1
> { &pci_dev_acpi_reset, .name = "acpi" }, # 2
> { &pcie_reset_flr, .name = "flr" }, # 3
> { &pci_af_flr, .name = "af_flr" }, # 4
> { &pci_pm_reset, .name = "pm" }, # 5
> { &pci_reset_bus_function, .name = "bus" }, # 6
> };
>
> dev->reset_methods[] = [1, 2, 3, 4, 5, 6]
> means all reset methods are supported, in the default priority
> order
>
> dev->reset_methods[] = [1, 0, 0, 0, 0, 0]
> means only pci_dev_specific_reset is supported
>
> dev->reset_methods[] = [3, 5, 0, 0, 0, 0]
> means pcie_reset_flr and pci_pm_reset are supported, in that
> priority order
>
> Then we wouldn't need the nested loop and the return value would be
> easier to analyze:
>
> for (i = 0; i < PCI_RESET_METHODS_NUM && (m = dev->reset_methods[i]); i++) {
> rc = pci_reset_fn_methods[m].reset_fn(dev, 0);
> if (rc == 0)
> return 0;
> if (rc != -ENOTTY)
> return rc;
> }
> return -ENOTTY;
>
> pci_init_reset_methods() would be something like:
>
> n = 0;
> for (i = 1; i < PCI_RESET_METHODS_NUM; i++) {
> rc = pci_reset_fn_methods[i].reset_fn(dev, 1);
> if (!rc)
> dev->reset_methods[n++] = i;
> if (rc != -ENOTTY)
> return;
> }
>
I had similar idea initially but couldn't put it in words nicely
thanks for this. I'll update this.
[...]
Thanks,
Amey
