Page MenuHomeFreeBSD

Expand device_get_property API
ClosedPublic

Authored by mindal_semihalf.com on Dec 15 2021, 10:41 AM.

Details

Summary

Create a new enum so that a caller can specify the expected type of property they want to read.
This way the bus logic can use that information to process the underlying data.
For example in DT all integer properties are stored in BE format.
In order to get constant results across different platforms we need to convert its endianness to match the host.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Tested with ACPI and DT on CN913x-DB (sdhci_xenon).

This revision is now accepted and ready to land.Dec 19 2021, 10:05 AM
sys/sys/bus.h
72

Is the integer data always 32-bit? I think it should be named DEVICE_PROP_INT as it may be used to read a 64-bit integer used as a physical address.

sys/sys/bus.h
72

Yes, my intention here is to put emphasis on the 32bit size of underlying property.
On OFW based systems DEVICE_PROP_UINT32 triggers usage of be32toh, which assumes that the underlying property is a uint32_t array.

ACPI stores integers(ACPI_TYPE_INTEGER) as uint64_t.
Consider the following scenario: (without this patch applied)

uint32_t temp;
(...)
device_get_property(..., &temp, sizeof(temp));

This will work just fine with ofw, but will fail on ACPI systems as the buffer is not big enough to store the property.
As a workaround in this case I'm doing an explicit conversion to uint32_t.
Now since the caller specified DEVICE_PROP_UINT32 I believe that the cast to uint32_t should not be a surprise.

I've also thought about adding some helpers, e.g

ssize_t device_get_property_u32(device_t dev, const char *prop, uint32_t *val)

Now I'm not sure if that won't pollute the API too much.
Any thoughts?

sys/sys/bus.h
72

Yes, my intention here is to put emphasis on the 32bit size of underlying property.
On OFW based systems DEVICE_PROP_UINT32 triggers usage of be32toh, which assumes that the underlying property is a uint32_t array.

ACPI stores integers(ACPI_TYPE_INTEGER) as uint64_t.
Consider the following scenario: (without this patch applied)

uint32_t temp;
(...)
device_get_property(..., &temp, sizeof(temp));

This will work just fine with ofw, but will fail on ACPI systems as the buffer is not big enough to store the property.
As a workaround in this case I'm doing an explicit conversion to uint32_t.
Now since the caller specified DEVICE_PROP_UINT32 I believe that the cast to uint32_t should not be a surprise.

How about keeping DEVICE_PROP_UINT32 for explicit casting and adding DEVICE_PROP_INT for the cases where the maxium supported type will be returned (ofw - 32-bit, acpi - 64-bit)? The binding is somewhat an ABI and it's user's responsibility to parse this with caution.

I've also thought about adding some helpers, e.g

ssize_t device_get_property_u32(device_t dev, const char *prop, uint32_t *val)

Now I'm not sure if that won't pollute the API too much.
Any thoughts?

Type passed as parameter works better IMO. Linux has a callback-per-type (overall > 10) and I do not fancy following this direction.

sys/sys/bus.h
72

Hi @andrew Do you have any thoughts after our previous considerations?

Add DEVICE_PROP_UINT64 to handle 8 byte integer properties.
The DEVICE_PROP_UINT32 won't work for ACPI, since it will cast the underlying property to uint32.

This revision now requires review to proceed.Jan 5 2022, 1:22 PM
mw requested changes to this revision.Jan 5 2022, 3:17 PM
mw added inline comments.
sys/dev/fdt/simplebus.c
384

s/uint32s/uint32 variables/

386

s/|/||/

This revision now requires changes to proceed.Jan 5 2022, 3:17 PM
share/man/man9/BUS_GET_PROPERTY.9
39

There should be a description of the type argument in the man page.

share/man/man9/device_get_property.9
39

And in this manpage.

sys/dev/acpica/acpi.c
148

I would keep the signature as it was (with the new argument) for consistancy.

1962–1966

What should happen if we ask for an integer type, but the data is a string or vice versa?

sys/dev/fdt/simplebus.c
386

Is this correct on little endian in the DEVICE_PROP_UINT64 case? Won't the two 32 bit halves be backwards?

sys/dev/fdt/simplebus.h
73 ↗(On Diff #100979)

Do any subclasses use this directly? If not we could move it to simplebus.c and make static.

mindal_semihalf.com marked an inline comment as done.
mindal_semihalf.com added a reviewer: manpages.

Update according to @andrew review.
Sorry for taking this long.

sys/dev/acpica/acpi.c
1962–1966

Initially I wanted to be as lax as possible with type checking in order no to break poorly written firmware.
With that being said I guess that declaring a string property as integer is a little bit too much.
I've added checks only for string<->integer case. Imho the "buffer" type should be treated as a wildcard of sorts.

sys/dev/fdt/simplebus.c
386

Whoops, you're right.
I've added the conversion and tested it by reading a simple 64bit property.

sys/dev/fdt/simplebus.h
73 ↗(On Diff #100979)

All buses that expose OFW will need to sue it, see D33437.
On the side note other places, such as ofw_pci.c will have to be modified to support device_get_property too.

sys/dev/fdt/simplebus.h
73 ↗(On Diff #100979)

Couldn't we have a default implementation that calls the parent device_get_property?

sys/dev/fdt/simplebus.h
73 ↗(On Diff #100979)

Good idea. I'll look into it.

Other than that, the manual page spelling and grammar LGTM. Can't say anything about consistency with the implementation.

share/man/man9/BUS_GET_PROPERTY.9
44

"a child's" maybe?

46

What I think you mean, reworded for clarity and slightly simpler language.

58

"a multiple of"? Do you mean "larger than"?

61
68

Also, not sure what you mean by "implementation". Is that the macro/function code?

70

Since you're touching this page.

share/man/man9/device_get_property.9
47
53

Or maybe "...is a string of bytes", depending on what you mean. (Would be consistent with the other types.)

60
65
mindal_semihalf.com marked 8 inline comments as done.

Update man pages

share/man/man9/BUS_GET_PROPERTY.9
58

Yes, multiple of is the right term.
For example for a uint32_t type(4 bytes) size = 4*n.

68

It's something similar to polymorphism in c++. We have multiple implementations of BUS_GET_PROPERTY depending on which bus a device resides on. This man page is mostly meant to describe the API, rather then any particular one.

share/man/man9/BUS_GET_PROPERTY.9
58

I would add "size" somewhere for clarity, but this being the kernel, I don't know the audience well enough to insist on it.

68

In that case, "shall" in the spots I suggested changing it is correct even if stilted, except the one that says "shall to", which should be "shall" instead.

share/man/man9/device_get_property.9
60

Or "shall" if you don't want to use "must". "shall to" is ungrammatical.

65

Same as above. "shall", not "shall to".

Manual page language looks good to me now. (Sorry for the "shall to" request-and-revert.)

I'll let others check consistency between manual page and source code.

I keep meaning to give it a try for my local application; after all I think I triggered this? I cannot even remember. If you have a few more days I'll try hard; otherwise please don't wait for me.

In D33457#774705, @bz wrote:

I keep meaning to give it a try for my local application; after all I think I triggered this? I cannot even remember. If you have a few more days I'll try hard; otherwise please don't wait for me.

Sure, I can wait.
IIRC it was triggered by problems you uncovered with the previous version of this API in D33170.
If you want to test it, don't forget to apply D34031 too.

No manual page changes I could see since, so my earlier LGTM about that still applies with its disclaimers.

@bz ping

On it. If things go well I should know in 2-3 hours. Sorry for the delay.

I tested has_prop fine; And get_prop with DEVICE_PROP_BUFFER was also working with FDT it seems. Haven't tried ACPI yet. But feel free to go ahead. Scrolling through it seems fine modulo the comments which probably deserve a mention in the commit message (type size change).

share/man/man9/BUS_GET_PROPERTY.9
28

Please update before commit.

share/man/man9/device_get_property.9
28

Please update before commit.

sys/dev/mmc/mmc_helpers.c
92

Is this size change an explicit bugfix as well?

sys/dev/sdhci/sdhci_xenon.c
474

Same as above. I'd probably mention this somewhere.

sys/dev/mmc/mmc_helpers.c
92

Well, kind of.
The "bus-width" property is stored as ACPI_TYPE_INTEGER(uint64_t) on ACPI systems.
Without this patch get_property would fail to read ACPI_TYPE_INTEGER if the buffer was smaller than 8 bytes.
With the addition of DEVICE_PROP_UINT32 the ACPI logic was changed to cast the property to uint32_t in that case.
I'll explain this in the commit msg.

There are minor style issues (they will be fixed when commiting), but otherwise LGTM.

The patch was tested on MacchiatoBin and CN913x-DB with DT and ACPI.

This revision is now accepted and ready to land.Mar 10 2022, 11:28 AM
This revision was automatically updated to reflect the committed changes.