On Sun, Sep 02, 2018 at 12:16:41PM +0200, Lukas Wunner wrote:
> On Fri, Aug 31, 2018 at 03:26:33PM -0600, Keith Busch wrote:
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -139,13 +139,45 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
> > pcie_port_device_remove(dev);
> > }
> >
> > +static int detected_iter(struct device *device, void *data)
> > +{
> > + struct pci_dev *pdev = data;
> > + struct pcie_port_service_driver *driver;
> > +
> > + if (device->bus == &pcie_port_bus_type && device->driver) {
> > + driver = to_service_driver(device->driver);
> > + if (driver && driver->error_detected)
> > + driver->error_detected(pdev);
> > + }
> > + return 0;
> > +}
>
> You're passing a pci_dev to the ->error_detected callback and this
> forces the callback to laboriously find its own pcie port service device,
> as visible in patch [13/16], where pciehp_error_detected() contains:
>
> struct device *device = pcie_port_find_device(dev, PCIE_PORT_SERVICE_HP);
> if (!device)
> return;
> pcie_dev = to_pcie_device(device);
>
> This seems backwards, the callbacks should be passed a pcie_device
> instead of a pci_dev.
>
> Same for the ->slot_reset callback iterator added in this patch.
I agree that is better. I was only trying to stick with the existing
pattern, but I can change that as a prep patch preceeding adding these
error callbacks, no problem.
