Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
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, >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, >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
Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
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, >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, >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?
--
Andrew Donnellan OzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
On Wed, Sep 29, 2021 at 11:15:37PM +1000, Andrew Donnellan wrote:
> On 29/9/21 6:53 pm, Uwe Kleine-König wrote:>
> list_for_each_entry(afu_dev, >phb->bus->devices, bus_list) {
> > - if (afu_dev->driver && afu_dev->driver->err_handler &&
> > - afu_dev->driver->err_handler->resume)
> > - afu_dev->driver->err_handler->resume(afu_dev);
> > + struct pci_driver *afu_drv;
> > + if (afu_dev->dev.driver &&
> > + (afu_drv =
> > to_pci_driver(afu_dev->dev.driver))->err_handler &&
>
> I'm not a huge fan of assignments in if statements and if you send a v6 I'd
> prefer you break it up.
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, >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, >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.
(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).)
> Apart from that everything in the powerpc and cxl sections looks good to me.
Thanks for checking.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
On 29/9/21 6:53 pm, Uwe Kleine-König wrote:>
list_for_each_entry(afu_dev, >phb->bus->devices, bus_list) {
- if (afu_dev->driver && afu_dev->driver->err_handler &&
- afu_dev->driver->err_handler->resume)
- afu_dev->driver->err_handler->resume(afu_dev);
+ struct pci_driver *afu_drv;
+ if (afu_dev->dev.driver &&
+ (afu_drv = to_pci_driver(afu_dev->dev.driver))->err_handler
&&
I'm not a huge fan of assignments in if statements and if you send a v6
I'd prefer you break it up.
Apart from that everything in the powerpc and cxl sections looks good to me.
--
Andrew Donnellan OzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
