Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver

[Frederic Barrat <fbarrat@xxxxxxxxxxxxx>]

On 29/09/2021 17:44, Andrew Donnellan wrote:
> On 29/9/21 11:43 pm, Uwe Kleine-König wrote:> I'm not a huge fan either, 
> I used it to keep the control flow as is and
> > without introducing several calls to to_pci_driver.
> >
> > The whole code looks as follows:
> >
> >     list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
> >         struct pci_driver *afu_drv;
> >         if (afu_dev->dev.driver &&
> >             (afu_drv = 
> > to_pci_driver(afu_dev->dev.driver))->err_handler &&
> >             afu_drv->err_handler->resume)
> >             afu_drv->err_handler->resume(afu_dev);
> >     }
> >
> > Without assignment in the if it could look as follows:
> >
> >     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->err_handler && afu_drv->err_handler->resume)
> >             afu_drv->err_handler->resume(afu_dev);
> >     }
> >
> > Fine for me.
> This looks fine.
>
> As an aside while writing my email I discovered the existence of 
> container_of_safe(), a version of container_of() that handles the null 
> and err ptr cases... if to_pci_driver() used that, the null check in the 
> caller could be moved until after the to_pci_driver() call which would 
> be neater.
> But then, grep tells me that container_of_safe() is used precisely zero 
> times in the entire tree. Interesting.
> > (Sidenote: What happens if the device is unbound directly after the
> > check for afu_dev->dev.driver? This is a problem the old code had, too
> > (assuming it is a real problem, didn't check deeply).)
> Looking at any of the cxl PCI error handling paths brings back 
> nightmares from a few years ago... Fred: I wonder if we need to add a 
> lock here?
Yes, it's indeed a potential issue, there's nothing to prevent the afu 
driver to unbind during that window. Sigh..
  Fred