Better handling of nexus and errors for DEV_GET_PATH

[kib]

Summary

commit d59afd47fd1ee3d536d33e0407524ee89b71a765
Author: Konstantin Belousov <[e-mail]>
Date:   Thu Oct 6 22:46:30 2022 +0300

    device_get_path(): handle case when dev is root
    
    PR:     266862
    Based on submission by: takawata

commit f356d7d48fbbd94658c2f7c4d7f53d6fcfabdf59
Author: Konstantin Belousov <[e-mail]>
Date:   Fri Oct 7 04:25:37 2022 +0300

    device_get_path(): do not drop the error from BUS_GET_DEVICE_PATH()
    
    Later it would silently converted to ENOMEM always, because any error
    was reported as NULL return path.

[jhb]

sys/kern/subr_bus.c

5316

This could probably use M_NOWAIT since the sbuf_new() above didn't use SB_NOWAIT so uses M_WAITOK internally?

[imp]

I think this is a good cleanup. We might be better off returning an error for when we have no parent (at the root) and I think we might need to add SB_NOWAIT, unless we can now sleep when probing / attaching driverrs.

sys/kern/subr_bus.c

5314

Could we just return an EINVAL error for root instead?

5316

Likely the above sbuf_new should use SB_NOWAIT and we should return a ENOMEM error there.
This function is called after boot in a sleepable context.
But during boot we can use it to determine if a wired entry matches one of the locators, and we can't sleep then (though in practice, there's a lot of places we are sloppy)

[jhb]

sys/kern/subr_bus.c

5316

Then I wonder if what we don't want to do instead, is change this API to pass down the sbuf pointer directly like we do now for child and location string methods. This lets the caller decide what kind of allocation policy it wants. It also avoids having to do a malloc() + copy in most cases since the caller can use sbuf_data() directly.

[imp]

sys/kern/subr_bus.c

5316

Then I wonder if what we don't want to do instead, is change this API to pass down the sbuf pointer directly like we do now for child and location string methods. This lets the caller decide what kind of allocation policy it wants. It also avoids having to do a malloc() + copy in most cases since the caller can use sbuf_data() directly.

Oh! I love that idea... For ioctl context, we'd do one thing and in the matching / wiring code another. And I think it would also save at least one malloc. We already have it BUS_GET_DEVICE_PATH which is used here to get the data, and having the callers provide the sb would solve this issue.

I can also code that up as well, if this is getting too far afield for kib.

[takawata]

sys/kern/subr_bus.c

5314

My original patch do just return with error, because the interface does not assume bus device handle the bus device itself. This code will produce "/root0" with "FreeBSD" locator, I think. I think this have to produce "/" instead, if it produce non-error output.

[kib]

sys/kern/subr_bus.c

5314

I do not like returning EINVAL, because why? nexus is the device that should be not exceptional for regular API.

Returning "/" would be inconsistent. Note that the bus method is not accumulating, it is supposed to return the full name.

[kib]

Make device_get_path() take sbuf pointer.

[takawata]

sys/kern/subr_bus.c

5314

% devctl getpath FreeBSD acpi_wmi0
/nexus0/acpi0/acpi_wmi0
devctl getpath FreeBSD acpi0
/nexus0/acpi0
 % devctl getpath FreeBSD  nexus0
/nexus0

What's next?

[jhb]

sys/kern/subr_bus.c

5314

I do think the FreeBSD path for root0 should be either "" or "/".

5317–5321

I think this is equivalent to:

error = sbuf_error(sb);
if (error == 0 && sbuf_len(sb) <= 1)
    error = EIO;

[imp]

sys/kern/subr_bus.c

5314

I think it should be "", since we append /$name...

5317–5321

I think so too...

[takawata]

sys/kern/subr_bus.c

5314

Returning empty string for all locator may simplify the issue.

[kib]

Return "/" for root0.
Simplify error calculation for sbuf.