Expand device_get_property API

[kd]

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.

[mw]

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

[andrew]

sys/sys/bus.h

73

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.

[kd]

sys/sys/bus.h

73

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?

[mw]

sys/sys/bus.h

73

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.

[mw]

sys/sys/bus.h

73

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

[kd]

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.

[mw]

sys/dev/fdt/simplebus.c

385

s/uint32s/uint32 variables/

387

s/|/||/

[andrew]

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

40

And in this manpage.

sys/dev/acpica/acpi.c

148

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

1856–1860

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

sys/dev/fdt/simplebus.c

387

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.

[kd]

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

sys/dev/acpica/acpi.c

1856–1860

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

387

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.

[andrew]

sys/dev/fdt/simplebus.h

73 ↗

(On Diff #100979)

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

[kd]

sys/dev/fdt/simplebus.h

73 ↗

(On Diff #100979)

Good idea. I'll look into it.

[kd]

Rebase on D34031

sys/dev/fdt/simplebus.h

73 ↗

(On Diff #100979)

@andrew take a look at D34031

[pauamma_gundo.com]

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.

59

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

62

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

48

54

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

61

66

[kd]

Update man pages

share/man/man9/BUS_GET_PROPERTY.9

59

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.

[pauamma_gundo.com]

share/man/man9/BUS_GET_PROPERTY.9

59

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

61

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

66

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

[pauamma_gundo.com]

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.

[kd]

Rebase on D34031

[kd]

@andrew Do you have any more thoughts on this?

[bz]

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.

[kd]

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.

[pauamma_gundo.com]

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

[kd]

@bz ping

[bz]

In D33457#776563, @mindal_semihalf.com wrote:

@bz ping

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

[bz]

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.

[kd]

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.

[mw]

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.