On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:

On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote:

quoted

On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:

quoted

On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas [off-list ref] wrote:

quoted

On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

quoted

quoted

quoted

+       return drv && drv->resume ?
+                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);

One line?

I don't think I touched that line.

Then why they are both in + section?

They're both in the + section of the interdiff because Uwe's v6 patch
looks like this:

  static int pci_legacy_resume(struct device *dev)
  {
          struct pci_dev *pci_dev = to_pci_dev(dev);
  -       return drv && drv->resume ?
  -                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
  +       if (pci_dev->dev.driver) {
  +               struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
  +
  +               if (drv->resume)
  +                       return drv->resume(pci_dev);
  +       }
  +
  +       return pci_pm_reenable_device(pci_dev);

and my revision looks like this:

   static int pci_legacy_resume(struct device *dev)
   {
	  struct pci_dev *pci_dev = to_pci_dev(dev);
  -       struct pci_driver *drv = pci_dev->driver;
  +       struct pci_driver *drv = to_pci_driver(dev->driver);

so compared to Uwe's v6, I restored that section to the original code.
My goal here was to make the patch as simple and easy to review as
possible.

quoted

quoted

quoted

+       struct pci_driver *drv = to_pci_driver(dev->dev.driver);
        const struct pci_error_handlers *err_handler =
-                       dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
+                       drv ? drv->err_handler : NULL;

Isn't dev->driver == to_pci_driver(dev->dev.driver)?

Yes, I think so, but not sure what you're getting at here, can you
elaborate?

Getting pointer from another pointer seems waste of resources, why we
can't simply

	struct pci_driver *drv = dev->driver;

I think this is in pci_dev_save_and_disable(), and "dev" here is a
struct pci_dev *.  We're removing the dev->driver member.  Let me know
if I'm still missing something.

quoted

quoted

quoted

-                               "bad request in aer recovery "
-                               "operation!\n");
+                               "bad request in AER recovery operation!\n");

quoted

quoted

Stray change? Or is it in a separate patch in your tree?

Could be skipped.  The string now fits on one line so I combined it to
make it more greppable.

This is inconsistency in your changes, in one case you are objecting of
doing something close to the changed lines, in the other you are doing
unrelated change.

You're right, this didn't make much sense in that patch.  I moved the
line join to the previous patch, which unindented this section and
made space for this to fit on one line.  Here's the revised commit:

  https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287