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

2021-10-15 Thread Andy Shevchenko 
On Fri, Oct 15, 2021 at 7:46 PM Bjorn Helgaas  wrote:
> On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:

...

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

Thanks for elaboration. I have got it.

...

> 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

Side remark: default without break or return is error prone (okay, to
some extent). Perhaps adding the return statement there will make
things robust and clean.

-- 
With Best Regards,
Andy Shevchenko

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

2021-10-15 Thread Bjorn Helgaas 
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:
> > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> > > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas  wrote:
> > > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

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

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

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

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

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

2021-10-13 Thread Andy Shevchenko 
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  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

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

2021-10-13 Thread Bjorn Helgaas 
On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas  wrote:
> > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> 
> > I split some of the bigger patches apart so they only touched one
> > driver or subsystem at a time.  I also updated to_pci_driver() so it
> > returns NULL when given NULL, which makes some of the validations
> > quite a bit simpler, especially in the PM code in pci-driver.c.
> 
> 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.  Here's one example without the NULL
check:

  @@ -493,12 +493,15 @@ static void pci_device_remove(struct device *dev)
   static void pci_device_shutdown(struct device *dev)
   {
  struct pci_dev *pci_dev = to_pci_dev(dev);
  -   struct pci_driver *drv = pci_dev->driver;

  pm_runtime_resume(dev);

  -   if (drv && drv->shutdown)
  -   drv->shutdown(pci_dev);
  +   if (pci_dev->dev.driver) {
  +   struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
  +
  +   if (drv->shutdown)
  +   drv->shutdown(pci_dev);
  +   }

  static void pci_device_shutdown(struct device *dev)
  {
struct pci_dev *pci_dev = to_pci_dev(dev);

pm_runtime_resume(dev);

if (pci_dev->dev.driver) {
  struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);

  if (drv->shutdown)
drv->shutdown(pci_dev);
}

and here's the same thing with the NULL check:

  @@ -493,7 +493,7 @@ static void pci_device_remove(struct device *dev)
   static void pci_device_shutdown(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);

  static void pci_device_shutdown(struct device *dev)
  {
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *drv = to_pci_driver(dev->driver);

pm_runtime_resume(dev);

if (drv && drv->shutdown)
  drv->shutdown(pci_dev);

> >  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
> > short device)
> >  {
> > +   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > const struct pci_device_id *id;
> >
> > if (pdev->vendor == vendor && pdev->device == device)
> > return true;
> 
> > +   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.  The current patch is:

  @@ -80,7 +80,7 @@ static struct resource video_rom_resource = {
*/
   static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
short device)
   {
  -   struct pci_driver *drv = pdev->driver;
  +   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
  const struct pci_device_id *id;

  if (pdev->vendor == vendor && pdev->device == device)

> > device_lock(&vf_dev->dev);
> > -   if (vf_dev->dev.driver) {
> > +   if (to_pci_driver(vf_dev->dev.driver)) {
> 
> Hmm...

Yeah, it could be either of:

  if (to_pci_driver(vf_dev->dev.driver))
  if (vf_dev->dev.driver)

I went back and forth on that and went with to_pci_driver() on the
theory that we were testing the pci_driver * before and the patch is
more of a mechanical change and easier to review if we test the
pci_driver * after.

> > +   if (!pci_dev->state_saved && pci_dev->current_state != 
> > PCI_D0
> 
> > +   && pci_dev->current_state != PCI_UNKNOWN) {
> 
> Can we keep && on the previous line?

I think this is in pci_legacy_suspend(), and I didn't touch that line.
It shows up in the interdiff because without the NULL check in
to_pci_driver(), we had to indent this code another level.  With the
NULL check, we don't need that extra indentation.

> > +   pci_WARN_ONCE(pci_dev, pci_dev->current_state != 
> > prev,
> > + "PCI PM: Device state not saved by 
> > %pS\n",
> > + drv->suspend);
> > }
> 
> ...
> 
> > +   return drv && drv->resume ?
> > +   drv->resume(pci_dev) : 
> > pci_pm_reenable_device(pci_dev);
> 
> One line?

I don't think I touched that line.

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

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

2021-10-13 Thread Uwe Kleine-König 
Hello,

On Wed, Oct 13, 2021 at 05:54:28AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> > > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > > index d997c9c3ebb5..7eb3706cf42d 100644
> > > --- a/drivers/misc/cxl/guest.c
> > > +++ b/drivers/misc/cxl/guest.c
> > > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> > >   pci_channel_state_t state)
> > >  {
> > >   struct pci_dev *afu_dev;
> > > + struct pci_driver *afu_drv;
> > > + struct pci_error_handlers *err_handler;
> > 
> > These two could be moved into the for loop (where afu_drv was with my
> > patch already). This is also possible in a few other drivers.
> 
> That's true, they could.  I tried to follow the prevailing style in
> the file.  At least in cxl, I didn't see any other cases of
> declarations being in the minimal scope like that.

I don't care much, do whatever you consider nice. I'm happy you liked
the cleanup and that you took it.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

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

2021-10-13 Thread Bjorn Helgaas 
On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote:
> On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > this is v6 of the quest to drop the "driver" member from struct pci_dev
> > > which tracks the same data (apart from a constant offset) as dev.driver.
> > 
> > I like this a lot and applied it to pci/driver for v5.16, thanks!
> > 
> > I split some of the bigger patches apart so they only touched one
> > driver or subsystem at a time.  I also updated to_pci_driver() so it
> > returns NULL when given NULL, which makes some of the validations
> > quite a bit simpler, especially in the PM code in pci-driver.c.
> 
> OK.
> 
> > Full interdiff from this v6 series:
> > 
> > diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> > index deaaef6efe34..36e84d904260 100644
> > --- a/arch/x86/kernel/probe_roms.c
> > +++ b/arch/x86/kernel/probe_roms.c
> > @@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
> >   */
> >  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
> > short device)
> >  {
> > +   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > const struct pci_device_id *id;
> >  
> > if (pdev->vendor == vendor && pdev->device == device)
> > return true;
> >  
> > -   if (pdev->dev.driver) {
> > -   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > -   for (id = drv->id_table; id && id->vendor; id++)
> > -   if (id->vendor == vendor && id->device == device)
> > -   break;
> > -   }
> > +   for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > +   if (id->vendor == vendor && id->device == device)
> > +   break;
> >  
> > return id && id->vendor;
> >  }
> > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > index d997c9c3ebb5..7eb3706cf42d 100644
> > --- a/drivers/misc/cxl/guest.c
> > +++ b/drivers/misc/cxl/guest.c
> > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> > pci_channel_state_t state)
> >  {
> > struct pci_dev *afu_dev;
> > +   struct pci_driver *afu_drv;
> > +   struct pci_error_handlers *err_handler;
> 
> These two could be moved into the for loop (where afu_drv was with my
> patch already). This is also possible in a few other drivers.

That's true, they could.  I tried to follow the prevailing style in
the file.  At least in cxl, I didn't see any other cases of
declarations being in the minimal scope like that.

Bjorn

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

2021-10-13 Thread Andy Shevchenko 
On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas  wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time.  I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.

It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.

Below are some comments as well.

...

>  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
> short device)
>  {
> +   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> const struct pci_device_id *id;
>
> if (pdev->vendor == vendor && pdev->device == device)
> return true;

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

>  }

...

> +   afu_result = err_handler->error_detected(afu_dev,
> +state);

One line?

...

> device_lock(&vf_dev->dev);
> -   if (vf_dev->dev.driver) {
> +   if (to_pci_driver(vf_dev->dev.driver)) {

Hmm...

...

> +   if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0

> +   && pci_dev->current_state != PCI_UNKNOWN) {

Can we keep && on the previous line?

> +   pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> + "PCI PM: Device state not saved by 
> %pS\n",
> + drv->suspend);
> }

...

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

One line?

...

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

...

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

Ditto.

...

> device_lock(&dev->dev);
> +   pdrv = to_pci_driver(dev->dev.driver);
> if (!pci_dev_set_io_state(dev, state) ||
> -   !dev->dev.driver ||
> -   !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||

> +   !pdrv ||
> +   !pdrv->err_handler ||

One line now?

> !pdrv->err_handler->error_detected) {

Or this and the previous line?

...

> +   pdrv = to_pci_driver(dev->dev.driver);
> +   if (!pdrv ||
> +   !pdrv->err_handler ||
> !pdrv->err_handler->mmio_enabled)
> goto out;

Ditto.

...

> +   pdrv = to_pci_driver(dev->dev.driver);
> +   if (!pdrv ||
> +   !pdrv->err_handler ||
> !pdrv->err_handler->slot_reset)
> goto out;

Ditto.

...

> if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
> -   !dev->dev.driver ||
> -   !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> +   !pdrv ||
> +   !pdrv->err_handler ||
> !pdrv->err_handler->resume)
> goto out;

Ditto.

> -   result = PCI_ERS_RESULT_NONE;
>
> pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> if (!pcidev || !pcidev->dev.driver) {
> dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n");
> pci_dev_put(pcidev);
> -   return result;
> +   return PCI_ERS_RESULT_NONE;
> }
> pdrv = to_pci_driver(pcidev->dev.driver);

What about splitting the conditional to two with clear error message
in each and use pci_err() in the second one?

...

> default:
> dev_err(&pdev->xdev->dev,
> -   "bad request in aer recovery "
> -   "operation!\n");
> +   "bad request in AER recovery operation!\n");

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

-- 
With Best Regards,
Andy Shevchenko

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

2021-10-13 Thread Uwe Kleine-König 
On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this is v6 of the quest to drop the "driver" member from struct pci_dev
> > which tracks the same data (apart from a constant offset) as dev.driver.
> 
> I like this a lot and applied it to pci/driver for v5.16, thanks!
> 
> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time.  I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.

OK.

> Full interdiff from this v6 series:
> 
> diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> index deaaef6efe34..36e84d904260 100644
> --- a/arch/x86/kernel/probe_roms.c
> +++ b/arch/x86/kernel/probe_roms.c
> @@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
>   */
>  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
> short device)
>  {
> + struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
>   const struct pci_device_id *id;
>  
>   if (pdev->vendor == vendor && pdev->device == device)
>   return true;
>  
> - if (pdev->dev.driver) {
> - struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> - for (id = drv->id_table; id && id->vendor; id++)
> - if (id->vendor == vendor && id->device == device)
> - break;
> - }
> + for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> + if (id->vendor == vendor && id->device == device)
> + break;
>  
>   return id && id->vendor;
>  }
> diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> index d997c9c3ebb5..7eb3706cf42d 100644
> --- a/drivers/misc/cxl/guest.c
> +++ b/drivers/misc/cxl/guest.c
> @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
>   pci_channel_state_t state)
>  {
>   struct pci_dev *afu_dev;
> + struct pci_driver *afu_drv;
> + struct pci_error_handlers *err_handler;

These two could be moved into the for loop (where afu_drv was with my
patch already). This is also possible in a few other drivers.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

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

2021-10-12 Thread Bjorn Helgaas 
On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this is v6 of the quest to drop the "driver" member from struct pci_dev
> which tracks the same data (apart from a constant offset) as dev.driver.

I like this a lot and applied it to pci/driver for v5.16, thanks!

I split some of the bigger patches apart so they only touched one
driver or subsystem at a time.  I also updated to_pci_driver() so it
returns NULL when given NULL, which makes some of the validations
quite a bit simpler, especially in the PM code in pci-driver.c.

Full interdiff from this v6 series:

diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
index deaaef6efe34..36e84d904260 100644
--- a/arch/x86/kernel/probe_roms.c
+++ b/arch/x86/kernel/probe_roms.c
@@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
  */
 static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
short device)
 {
+   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
const struct pci_device_id *id;
 
if (pdev->vendor == vendor && pdev->device == device)
return true;
 
-   if (pdev->dev.driver) {
-   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
-   for (id = drv->id_table; id && id->vendor; id++)
-   if (id->vendor == vendor && id->device == device)
-   break;
-   }
+   for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
+   if (id->vendor == vendor && id->device == device)
+   break;
 
return id && id->vendor;
 }
diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index d997c9c3ebb5..7eb3706cf42d 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c
@@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
pci_channel_state_t state)
 {
struct pci_dev *afu_dev;
+   struct pci_driver *afu_drv;
+   struct pci_error_handlers *err_handler;
 
if (afu->phb == NULL)
return;
 
list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
-   struct pci_driver *afu_drv;
-
-   if (!afu_dev->dev.driver)
-   continue;
-
afu_drv = to_pci_driver(afu_dev->dev.driver);
+   if (!afu_drv)
+   continue;
 
+   err_handler = afu_drv->err_handler;
switch (bus_error_event) {
case CXL_ERROR_DETECTED_EVENT:
afu_dev->error_state = state;
 
-   if (afu_drv->err_handler &&
-   afu_drv->err_handler->error_detected)
-   afu_drv->err_handler->error_detected(afu_dev, 
state);
-   break;
+   if (err_handler &&
+   err_handler->error_detected)
+   err_handler->error_detected(afu_dev, state);
+   break;
case CXL_SLOT_RESET_EVENT:
afu_dev->error_state = state;
 
-   if (afu_drv->err_handler &&
-   afu_drv->err_handler->slot_reset)
-   afu_drv->err_handler->slot_reset(afu_dev);
-   break;
+   if (err_handler &&
+   err_handler->slot_reset)
+   err_handler->slot_reset(afu_dev);
+   break;
case CXL_RESUME_EVENT:
-   if (afu_drv->err_handler &&
-   afu_drv->err_handler->resume)
-   afu_drv->err_handler->resume(afu_dev);
-   break;
+   if (err_handler &&
+   err_handler->resume)
+   err_handler->resume(afu_dev);
+   break;
}
}
 }
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 7e7545d01e27..08bd81854101 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1795,6 +1795,8 @@ static pci_ers_result_t cxl_vphb_error_detected(struct 
cxl_afu *afu,
pci_channel_state_t state)
 {
struct pci_dev *afu_dev;
+   struct pci_driver *afu_drv;
+   struct pci_error_handlers *err_handler;
pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;
 
@@ -1805,16 +1807,16 @@ static pci_ers_result_t cxl_vphb_error_detected(struct 
cxl_afu *afu,
return result;
 
list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
-   struct pci_driver *afu_drv;
-   if (!afu_dev->dev.driver)
-   continue;
-
afu_drv = to_pci_driver(afu_dev->dev.driv

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

2021-10-04 Thread Uwe Kleine-König 
Hello,

this is v6 of the quest to drop the "driver" member from struct pci_dev
which tracks the same data (apart from a constant offset) as dev.driver.

Changes since v5:
 - Some Acks added
 - Some fixes in "PCI: Replace pci_dev::driver usage by
   pci_dev::dev.driver" to properly handle that
   to_pci_driver(X) is wrong if X is NULL.
   This should fix the problem reported by Ido Schimmel.

Full range diff below.

This patch stack survived an allmodconfig build on arm64, m68k, powerpc,
riscv, s390, sparc64 and x86_64 on top of v5.15-rc3.

Best regards
Uwe

Uwe Kleine-König (11):
  PCI: Simplify pci_device_remove()
  PCI: Drop useless check from pci_device_probe()
  xen/pci: Drop some checks that are always true
  bcma: simplify reference to the driver's name
  powerpc/eeh: Don't use driver member of struct pci_dev and further
cleanups
  ssb: Simplify determination of driver name
  PCI: Replace pci_dev::driver usage that gets the driver name
  scsi: message: fusion: Remove unused parameter of mpt_pci driver's
probe()
  crypto: qat - simplify adf_enable_aer()
  PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
  PCI: Drop duplicated tracking of a pci_dev's bound driver

 arch/powerpc/include/asm/ppc-pci.h|  5 -
 arch/powerpc/kernel/eeh.c |  8 ++
 arch/powerpc/kernel/eeh_driver.c  | 10 +-
 arch/x86/events/intel/uncore.c|  2 +-
 arch/x86/kernel/probe_roms.c  | 10 +-
 drivers/bcma/host_pci.c   |  6 +-
 drivers/crypto/hisilicon/qm.c |  2 +-
 drivers/crypto/qat/qat_4xxx/adf_drv.c |  7 +-
 drivers/crypto/qat/qat_c3xxx/adf_drv.c|  7 +-
 drivers/crypto/qat/qat_c62x/adf_drv.c |  7 +-
 drivers/crypto/qat/qat_common/adf_aer.c   | 10 +-
 .../crypto/qat/qat_common/adf_common_drv.h|  3 +-
 drivers/crypto/qat/qat_dh895xcc/adf_drv.c |  7 +-
 drivers/message/fusion/mptbase.c  |  7 +-
 drivers/message/fusion/mptbase.h  |  2 +-
 drivers/message/fusion/mptctl.c   |  4 +-
 drivers/message/fusion/mptlan.c   |  2 +-
 drivers/misc/cxl/guest.c  | 24 +++--
 drivers/misc/cxl/pci.c| 30 +++---
 .../ethernet/hisilicon/hns3/hns3_ethtool.c|  2 +-
 .../ethernet/marvell/prestera/prestera_pci.c  |  2 +-
 drivers/net/ethernet/mellanox/mlxsw/pci.c |  2 +-
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  |  3 +-
 drivers/pci/iov.c | 33 +--
 drivers/pci/pci-driver.c  | 96 ++-
 drivers/pci/pci.c |  4 +-
 drivers/pci/pcie/err.c| 36 +++
 drivers/pci/xen-pcifront.c| 63 ++--
 drivers/ssb/pcihost_wrapper.c |  6 +-
 drivers/usb/host/xhci-pci.c   |  2 +-
 include/linux/pci.h   |  1 -
 31 files changed, 208 insertions(+), 195 deletions(-)

Range-diff against v5:
 -:   >  1:  c2b53ab26a6b PCI: Simplify pci_device_remove()
 -:   >  2:  2c733e1d5186 PCI: Drop useless check from 
pci_device_probe()
 -:   >  3:  547ca5a7aa16 xen/pci: Drop some checks that are always 
true
 -:   >  4:  40eb07353844 bcma: simplify reference to the driver's 
name
 -:   >  5:  bab59c1dff6d powerpc/eeh: Don't use driver member of 
struct pci_dev and further cleanups
 1:  abd70de9782d !  6:  92f4d61bbac3 ssb: Simplify determination of driver name
@@ Commit message
 This has the upside of not requiring the driver member of struct 
pci_dev
 which is about to be removed and being simpler.
 
+Acked-by: Michael Büsch 
 Signed-off-by: Uwe Kleine-König 
 
  ## drivers/ssb/pcihost_wrapper.c ##
 2:  735845bd26b9 !  7:  6303f03ab2aa PCI: Replace pci_dev::driver usage that 
gets the driver name
@@ Commit message
 driver name by dev_driver_string() which implicitly makes use of struct
 pci_dev::dev->driver.
 
+Acked-by: Simon Horman  (for NFP)
 Signed-off-by: Uwe Kleine-König 
 
  ## drivers/crypto/hisilicon/qm.c ##
 3:  1e58019165b9 =  8:  658a6c00ec96 scsi: message: fusion: Remove unused 
parameter of mpt_pci driver's probe()
 4:  dea72a470141 =  9:  aceaf5321603 crypto: qat - simplify adf_enable_aer()
 5:  b4165dda38ea ! 10:  80648d85 PCI: Replace pci_dev::driver usage by 
pci_dev::dev.driver
@@ arch/x86/kernel/probe_roms.c: static struct resource video_rom_resource 
= {
  static bool match_id(struct pci_dev *pdev, unsigned short vendor, 
unsigned short device)
  {
 -  struct pci_driver *drv = pdev->driver;
-+  struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
const struct pci_device_id *id;
  
if (pdev->vendor == vendor && pdev->device == device)
+   return true;
+ 
+-  for (id = drv ? drv->id_table : NU