nvme: Fix broken ahci attachment

[imp]

Summary

ahci was broken when I added the check for the pci programming
interface. Only do that on PCI bus attached devices.

Sponsored by: Netflix

[adrian]

sys/dev/nvme/nvme_sim.c

327

is this really the only way we can potentially call a parent/bus method conditionally? eep

[imp]

sys/dev/nvme/nvme_sim.c

327

I believe so, but ask @jhb to double check.

[jhb]

sys/dev/nvme/nvme_sim.c

327

Well, we typically get the devclass by name and compare that instead, but yes.

Note that we do this an awful lot though, so I do wonder if it might not be worth at least having a helper function for this (bool is_pci_device(device_t dev)) that various drivers can call.

Currently in the tree:

> git grep 'devclass_find("pci")' sys/ | wc -l
      30

The other more OOP-y way to handle this btw, would be to have a new nvme_if.m method that is implemented by nvmeX instead of the children, something like:

nvme_if.m:

CODE {
      static bool
      default_is_storage_device(device_t dev)
      {
          return (true);
      }
}

...

METHOD bool is_storage_device {
        device_t dev;
} default default_is_storage_device;

nvme_pci.c:

static bool
nvme_pci_is_storage_device(device_t dev)
{
    /*
      * NVMHCI 1.0 interfaces are the only devices that
      * have namespaces with LBA ranges.
      */
     return (pci_get_progif(dev) == PCIP_STORAGE_NVM_ENTERPRISE_NVMHCI_1_0);
}

...

     DEVMETHOD(nvme_is_storage_device, nvme_pci_is_storage_device);

Then in the code above what you write is:

static int
nvme_sim_probe(device_t dev)
{
    if (nvme_use_nvd)
       return (ENXIO);

    if (!NVME_IS_STORAGE_DEVICE(device_get_parent(dev))
       return (ENXIO):

    device_set_desc(...);
    return (BUS_PROBE_DEFAULT);
}

Overall, I think I prefer the more OOP-y approach.

[imp]

sys/dev/nvme/nvme_sim.c

327

OK. I'll code that up. But it does leave unanswered if there's a good way to know when ivar accessors can be called... ahci uses ivars in a way that makes this impossible, though (and is what leads to the crash.

I'll see if I can do this in a reasonable amount of time for 15.1R. If not, what do you think about this as a quick fix with the longer approach as a fallback if my day gets away from me?

[imp]

sys/dev/nvme/nvme_sim.c

327

ah, but in this case it's already there... Woot! Though it's primary purpose is parent to child callback, not child-to-parent but we don't seem to have a strong preference for unidirectional interfaces

[jhb]

In terms of accessors in general: I recently added a new "has" wrapper method to __BUS_ACCESSOR, but it only works if your IVAR is using a "global" number and not the private range. All of the PCI IVARs are in the private range.

sys/dev/nvme/nvme_sim.c

327

Yes, I think this is ok. I have used a single foo_if.m for methods that are on both "sides" of a logical interface before, e.g. t4_if.m defines methods on "sibling" PCI devices that call back and forth into each other. I think grouping bidirectional methods into a single interface is ok (and was intentionally suggesting it).

[adrian]

Tested on my intel comet lake desktop w/ nvme attached via an ahci sata/raid integrated, worked just fine.

honestly i think this is fine to land to unbrick head and stable/15, and we should take a side quest to clean up things.