[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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, &afu->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, &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.

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

Attachment: signature.asc
Description: PGP signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.