Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Jun 2021 22:52:42 +0530
From:   Amey Narkhede <ameynarkhede03@gmail.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     alex.williamson@redhat.com,
        Raphael Norwitz <raphael.norwitz@nutanix.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        kw@linux.com, Shanker Donthineni <sdonthineni@nvidia.com>,
        Sinan Kaya <okaya@kernel.org>, Len Brown <lenb@kernel.org>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>
Subject: Re: [PATCH v7 2/8] PCI: Add new array for keeping track of ordering
 of reset methods
Message-ID: <20210618172242.vs3qwimjpcicb4m4@archlinux>
References: <20210608054857.18963-3-ameynarkhede03@gmail.com>
 <20210617231305.GA3139128@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210617231305.GA3139128@bjorn-Precision-5520>
Precedence: bulk

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@redhat.com>
> > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > Co-developed-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > ---
> >  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