PCIe HotPlug support

[jmg]

Summary

Initial support for PCIe Hot Plug support. There are two parts.

First is making the pci device adding more of a proper subclass, so that the PCI framework can add a pci device, and the various implementations (like ACPI) can
allocate/create the device w/ the necessary info.

Second is supporting hot plug. There is a kernel option on this part that will disable this functionality. This will initially be disabled, but we should look at enabling it by default to get the most testing.

This does not have the final state machine for controlling hot plug, that still needs some work.

[adrian]

sys/dev/pci/pcie_hp.c

448

Hm, so I know it requires MSI, but is it possible to also have legacy IRQ bits working too? I ask only because some of the wireless chipsets (eg *cough* earlier QCA pieces) support MSI but it's buggy, so the driver just plainly doesn't use MSI. (eg all the AR92xx PCIe pieces.)

[imp]

First pass: everything that isn't the actual hotplug stuff. Mostly dealing with consistency with the rest of the system.

sys/conf/files

2133

that's a funky option name. pcie_hp or pciehp would be better. pci_ehp doesn't make sense to my brain.

sys/conf/options

179

and fix it here too. This is a PCIE option...

sys/dev/pci/pci.c

3555

I'd be very hesitant to remove this DELAY. I think it is really needed for some legacy hardware.

3614

This style change looks independent of the rest of the work.

3840–3848

This cleanup looks independent of the rest of the work.

5407

This isn't necessarily a good test.

There are many devices that don't expect you to read config space for function > 0 since they are single function devices, as defined in the header for function 0. While most are well behaved, an interesting number are not.

sys/dev/pci/pcie_hp.c

122

history has shown that this isn't a good interface. Pass the length in. I see you take care to try to not overflow, but it isn't best practices...

[jmg]

address some comments.

sys/conf/files

2133

Yeh, I agree.. I was trying to stay in the PCI_ "namespace".. I'm fine w/ renaming to PCIE_HP or PCIEHP.

sys/conf/options

179

Should I just drop the e then and call it PCI_HP? the PCI_IOV is for PCIe SR-IOV and it doesn't have the E.

sys/dev/pci/pci.c

3555

I talked w/ gavin (iirc) about this, and this was only required for a special AMD8111 chip that was never put into production (I believe pre production) that FreeBSD did devel on.

See:
https://svnweb.freebsd.org/base/head/sys/dev/pci/pci.c?revision=151645&view=markup

3614

ok, missed that w/ my recent white space clean up last night.. I'll commit this separately shortly.

3840–3848

Standard problem of cleaning up code while looking at related code, and trying to pull changes apart.

Shall I just commit this fix then?

5407

Well, pci_child_present is called in two contexts, either a) a device was present and went away (and in this case I believe it's well defined) (so, should be fine) or b) we are probing for devices, but we always check 0, 0 first, before going on. If you follow the code, this code should be virtually identical (here we check all 32 bits instead of just the vendor).. See pci_read_device for the original check.

sys/dev/pci/pcie_hp.c

122

as this code is static and used in only one place, I'd prefer not to. I would rather inline this code in pci_slot_cap_print than pass in a length here.

448

This is only to handle the interrupt for the PCIe bridge chip providing Hot Plug support... Devices below it should work fine on legacy interrupts just as they work fine below any other PCIe bridge.

If you have an example of them not working, please let me know.

[imp]

just commenting on the comments...
Still haven't had the time to concentrate on pcie_hp.c

sys/conf/options

179

Sure, I like that better. PCI_HP it is...

sys/dev/pci/pci.c

3555

OK. The pre-production status of this part wasn't clear from reading the commit log.

3840–3848

Yes. Reduce the diffs. Easier to review, and easier to bisect changes if one of these cleanups winds up causing trouble.

5407

Document this with a comment then.

[jhb]

There are other PCI bus drivers that probably need an add_child method. Maybe just ofw_pci.c? Grep for pci_driver to see who is using it.

sys/dev/pci/pci.c

3508

You could keep this to avoid the code duplication in pci_add_children(). It could do something like this:

device_t
pci_identify_function(device_t pcib, device_t dev, int domain, int busno, int slot, int func)
{

     if (pci_child_present(pcib, busno, slot, func) == -1 &&
         pci_find_dbsf(domain, busno, slot, func) == NULL) {
             return (PCI_ADD_CHILD(dev, slot, func));
     return (NULL);
}

Then to handle ARI you would just do:

child = pci_identify_function(...);
if (child != NULL && pci_enable_ari)
   ....

3553

I would leave the REG() here to reduce the noise in the diff.

3635

style(9) puts ()'s around return values, so 'return (pci_add_child_size(...))' FWIW.

(Also, throughout the patch).

5407

You should only check the vendor. That is what the PCI spec defines (as it were) in that it says that a vendor ID of 0xFFFFh is always invalid. It makes no guarantees about the device ID.

sys/dev/pci/pcie_hp.c

265

Is this only for debugging? That's fine for now, but perhaps put it under an #ifdef? You should probably teach pciconf -lc to output these details, but they probably belong there rather than in the kernel outside of a temporary #ifdef.

283

This will reprobe any devices that were already there. Ideally you would only probe new devices to avoid console spam, etc.

380

These comments are mostly sentences, please just format them as such (leading capital letter and punctuation).

411

This comment seems outdated perhaps? It doesn't seem like this is doing that anywhere. NEW_PCIB is probably doing all of this for you. You could perhaps provide a hint to reserve more bus numbers by default if you wish.

481

For the case of only 1 resource, I think the spec table probably obfuscates more than helps.

sys/dev/pci/pcivar.h

157

Is the idea of putting these here rather than in pcicfg_hp because the same message can be overloaded for both hotplug and other things like AER?

[jhb]

I had another thought in the shower this morning. Storing resources, etc. in the dinfo is not the right place to do this. The MSI interrupt belongs to the pcib(4) device. This state should be stored in the pcib(4) driver as part of its softc. Pretty much all of pcicfg_hp should move there, and the setup should be done from pcib_attach(), not from the PCI bus layer. This also makes it much more feasible to ensure there are adequate resources reserved for the bridge since you will be doing it in the right place when the bridge is doing the initial probe of its windows.

[gavin]

sys/dev/pci/pci.c

3555

Nope, not me. I know nothing about this line's history other than from commit logs.

sys/dev/pci/pcie_hp.c

166

If dinfo->cfg.hp.hp_slotcap was set in pci_hotplug_init() rather than _setup it could be used here rather than reading it from the HW each time.

313

Use dinfo->cfg.hp.hp_slotcap here

373

I'd rather see this spelled out as the individual bits or'd together. As it is, PCIEM_SLOT_STA_EMASK is just a magic number that means nothing in isolation in the header file.

538

Save this in dinfo->cfg.hp.hp_slotcap here (rather than in pci_hotplug_setup() as it is now) then all other uses of PCIER_SLOT_CAP can just use this cached copy.