Page MenuHomeFreeBSD

bus: Introduce the bus interface get_device_path
AcceptedPublic

Authored by imp on Oct 30 2021, 12:52 AM.

Details

Reviewers
jhb
Summary

This returns the full path of a the child device requested. Since
there's different ways to recon the entire path, include a 'locator'
method. The default 'FreeBSD' method uses a filesystem-like path name
with each device to the root node separated by /. Other locators will be
UEFI, ACPI and fdt, though others are possible in the future. Make the
locator a string to allow maximum flexibility.

Sponsored by: Netflix

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 42786
Build 39674: arc lint + arc unit

Event Timeline

imp requested review of this revision.Oct 30 2021, 12:52 AM
imp created this revision.

Drop UEFI from this early patch, will add it when I add UEFI support
to pci.

jhb added inline comments.
sys/kern/subr_bus.c
4619

I suspect we should at least return rv? I wonder what it would break in practice if we failed-close by returning an error instead (EINVAL perhaps)

This revision is now accepted and ready to land.Nov 29 2021, 5:23 PM
sys/kern/subr_bus.c
4619

So I've looked ahead in the series and I do wonder if we want to support non-FreeBSD paths in this routine at all. My question about UEFI path inheritance in the UEFI review is probably relevant. I kind of find it surprising for child devices to inherit the parent UEFI path on non-UEFI-aware buses. I would rather expect that those child devices wouldn't really "exist" in UEFI, per se, but I could be wrong. Do you want the PCI bus devices to have a UEFI path for example, or just the bridges?

If we don't want that UEFI inheritance, then I think the final version of this function can be something like:

if (strcmp(locator, BUS_LOCATOR_FREEBSD) != 0)
    return (EINVAL);

parent = device_get_parent(bus);
if (parent != NULL) {
    rv = BUS_GET_DEVICE_PATH(parent, bus, locator, sb);
    if (rv != 0)
        return (rv);
}
sbuf_printf(sb, "/%s", device_get_nameunit(child));
return (0);

If we do want UEFI inheritance (but not ACPI clearly), then maybe that inheritance should be explicit (vs ACPI opting out):

int rv;
device_t parent;

parent = device_get_parent(bus);
if (parent != NULL) {
    rv = BUS_GET_DEVICE_PATH(parent, bus, locator, sb);
} else {       
    if (strcmp(locator, BUS_LOCATOR_FREEBSD) == 0)
        rv = 0;
    else
        rv = EINVAL;
}
if (rv != 0)
    return (rv);

if (strcmp(locator, BUS_LOCATOR_FREEBSD) == 0)
    sbuf_printf(sb, "/%s", device_get_nameunit(child));
return (0)

(I would maybe call rv error instead as I think error is used more consistently for this in new-bus?)

sys/kern/subr_bus.c
4619

For UEFI, we do want the inheritance. There's a number of differences between FreeBSD's device model and UEFI's device paths. Having the inheritance allows for arbitrary nesting seemlessly.
ACPI is the odd-duck out here in that it has a global name space that we pre-walk to get the ACPI handle, so we don't need to recover that path from the FreeBSD device tree.
I do think that getting to the root of the tree is not an EINVAL sense. We're done with recursion, so we should return 0 always in that case.
Maybe returning rv is the correct thing. I don't think either of your proposed alternate implementations actually add anything, though. They are more complex for no benefit. I don't think we'll have anything that walks the tree up, but wants us to signal an error if we get to the root. ACPI won't hit that. FDT won't hit that. They will both override this function.
I agree on rv vs error, ,though.

sys/kern/subr_bus.c
4619

Also, fwiw, I had tried to create some kind of 'mux' object where this sort of behavior could reside for the trip to the root. The dispatch was based on the locator type as well as the function method to call. Should we need it, I can dust that off, but it seems to be a lot of hacking to the roots of kobj to get only a slight gain in functionality here.

sys/kern/subr_bus.c
4619

Since FDT will be like ACPI, you will have to hack this function yet again for FDT to opt-out of inheritance. I think instead that UEFI should opt-in to inheritance as it will be the exception rather than the rule. I don't consider this an objection to committing though.

sys/kern/subr_bus.c
4619

OK. I'll reconsider this when I get FDT done, because OFW acts more like UEFI in its paths and I'll do the pair of them together.