Re: [PATCH v5 02/11] pci: add option for net failover

[Jens Freimann]

On Thu, Oct 24, 2019 at 10:52:36AM -0600, Alex Williamson wrote:
> On Thu, 24 Oct 2019 11:37:54 +0200
> Jens Freimann <address@hidden> wrote:
[...]
> >
> > >While reviewing, I realized that we shouldn't have this check for below 
> > reasons.
> > >
> > >1. It is user's responsibility to pass networking device.
> > >But its ok to check the class, if PCI Device is passed.
> > >So class comparison should be inside the pci_check().
> >
> > I'm not sure I understand this point, could you please elaborate?
> > You're suggesting to move the check for the class into the check for
> > pci_is_express?
>
> Seems like the suggestion is that net_failover_pair_id should be an
> option on the base class of PCIDevice (DeviceState?) and only if it's a
> PCI device would we check the class code.  But there are dependencies
> at the hotplug controller, which I think is why this is currently
> specific to PCI.

Yes, It doesn't support acpi, shpc,... hotplug as of now. It
shouldn't be hard to add but I'd like to do it as a follow-on series.

> However, it's an interesting point about pci_is_express().  This test
> is really just meant to check whether the hotplug controller supports
> this feature, which is only implemented in pciehp via this series.
> There's a bit of a mismatch though that pcie_is_express() checks
> whether the device is express, not whether the bus it sits on is
> express.  I think we really want the latter, so maybe this should be:
>
> pci_bus_is_express(pci_get_bus(dev)
>
> For example this feature should work if I plug an e1000 (not e1000e)
> into an express slot, but not if I plug an e1000e into a conventional
> slot.

I'll try this and test. 

Thanks!

regards,
Jens