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

[Jens Freimann <jfreimann@redhat.com>]

On Thu, Oct 24, 2019 at 10:52:36AM -0600, Alex Williamson wrote:
>On Thu, 24 Oct 2019 11:37:54 +0200
>Jens Freimann <jfreimann@redhat.com> 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