Re: git: 99e6980fcf5e - main - device_get_property: add a HANDLE case

On 11 Oct 2022, at 13:10, Bjoern A. Zeeb <bz@FreeBSD.org> wrote:
> 
> On Tue, 11 Oct 2022, Jessica Clarke wrote:
> 
>> On 9 Oct 2022, at 22:53, Bjoern A. Zeeb <bz@FreeBSD.org> wrote:
>>> 
>>> The branch main has been updated by bz:
>>> 
>>> URL: https://cgit.FreeBSD.org/src/commit/?id=99e6980fcf5e12654c3e89b97b774de807d740a4
>>> 
>>> commit 99e6980fcf5e12654c3e89b97b774de807d740a4
>>> Author: Bjoern A. Zeeb <bz@FreeBSD.org>
>>> AuthorDate: 2022-09-29 12:41:58 +0000
>>> Commit: Bjoern A. Zeeb <bz@FreeBSD.org>
>>> CommitDate: 2022-10-09 21:51:25 +0000
>>> 
>>> device_get_property: add a HANDLE case
>>> 
>>> This will resolve a reference and return the appropriate handle, a node
>>> on the simplebus or an ACPI_HANDLE for ACPI. For now we do not try to
>>> further abstract the return type.
>> 
>> How’s this supposed to be used though? phandle_t is a uint32_t and
>> ACPI_HANDLE is a void *, which are not the same size (aside from
>> ILP32), let alone the same type. The existing interface lets you not
>> care about the bus because the type corresponds to what you ask for
>> (string or fixed-width integer), but this totally breaks that. This
>> really should be abstracting the type as well, presumably to a
>> uintptr_t.
> 
> While that is all true, basic types are always easier to handle :)
> 
> All known use cases so far have entirely different code paths in ACPI and
> FDT as they are "described" differently and there would be more work to be
> done to harmonize these cases.

Not once you get in to the DSD world, where you have device tree bindings
inside ACPI and want to be able to use one code path for both.

> Making it a uintptr_t wouldn't solve that problem and that later code has to
> deal with the appropriate type still and call non-bus-bus-specific functions
> (e.g., acpi_get_device vs. OF_device_from_xref) with the appropriate type and
> I'd almost vote for extending this to return a device instead rather
> than a handle, but that may be highly dependent on the use case I am
> aware of.

Using a device wouldn’t let you get at intermediary nodes that have no
device. You do want a way to turn the handle into a device in a
bus-agnostic way, though.

> In earlier code I used the DEVICE_PROP_ANY with the bus-sized encoded
> handle. Now we get at least proper size checks on each bus, and avoid
> putting ACPI-internal code into drivers and avoid code duplication there
> and we have a way to track the use-cases.
> 
> mw@ had initially asked for this to be broken out of another review and said
> he's working on more code (I haven't seen) which will hopefully benefit from
> this and I assume once we get there and some more bus-abstractions may
> be dealt with, we may be able to refine this.

My concern is this is just not quite the right shape for the use cases
I’m imagining, but tweaking it slightly (using uintptr_t, or
integer-in-a-void *), would allow for that whilst being completely
compatible with whatever you’re trying to achieve here. My problem is
I’ve been looking at trying to do more complete DSD things to support
PRP0001 bindings and this isn’t taking into account the existence of
that.

Jess

> Hence the "For now ..." in the commit message. I'll highly likely defer
> the MFC for longer than specified for those reasons but I like to have
> the email to not forget about it.
> 
> /bz
> 
> -- 
> Bjoern A. Zeeb r15:7