Re: [PATCH 10/16] PCI/portdrv: Provide pci error callbacks

[Keith Busch <keith.busch@intel.com>]

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.