Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

...

> > It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.
> 
> It is a little unusual.  I only found three of 77 that are NULL-aware:
> 
>   to_moxtet_driver()
>   to_siox_driver()
>   to_spi_driver()
> 
> It seems worthwhile to me because it makes the patch and the resulting
> code significantly cleaner.

I'm not objecting the change, just a remark.

...

> > > +       for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > > +               if (id->vendor == vendor && id->device == device)
> > 
> > > +                       break;
> > 
> > return true;
> > 
> > >         return id && id->vendor;
> > 
> > return false;
> 
> Good cleanup for a follow-up patch, but doesn't seem directly related
> to the objective here.

True. Maybe you can bake one while not forgotten?

...

> > > +       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?

...

> > > +       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;

?

...

> > 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.

-- 
With Best Regards,
Andy Shevchenko