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

Reply: Bjoern A. Zeeb: "Re: git: 99e6980fcf5e - main - device_get_property: add a HANDLE case"
In reply to: Bjoern A. Zeeb: "git: 99e6980fcf5e - main - device_get_property: add a HANDLE case"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Mon, 10 Oct 2022 23:21:32 UTC
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.

Jess

>    MFC after:      2 weeks
>    Reviewed by:    mw
>    Differential Revision: https://reviews.freebsd.org/D36793
> ---
> share/man/man9/device_get_property.9 |  5 +++-
> sys/dev/acpica/acpi.c                | 47 ++++++++++++++++++++++++++++++++++++
> sys/dev/fdt/simplebus.c              | 19 +++++++++++++--
> sys/kern/subr_bus.c                  |  1 +
> sys/sys/bus.h                        |  1 +
> 5 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/share/man/man9/device_get_property.9 b/share/man/man9/device_get_property.9
> index d925f5f224db..93c01f199477 100644
> --- a/share/man/man9/device_get_property.9
> +++ b/share/man/man9/device_get_property.9
> @@ -25,7 +25,7 @@
> .\"
> .\" $FreeBSD$
> .\"
> -.Dd February 18, 2022
> +.Dd September 29, 2022
> .Dt DEVICE_GET_PROPERTY 9
> .Os
> .Sh NAME
> @@ -54,6 +54,9 @@ Currently the following types are supported:
> The underlying property is a string of bytes.
> .It Dv DEVICE_PROP_ANY
> Wildcard property type.
> +.It Dv DEVICE_PROP_HANDLE
> +Following a reference the underlying property is a handle of the
> +respective bus.
> .It Dv DEVICE_PROP_UINT32
> The underlying property is an array of unsigned 32 bit integers.
> The
> diff --git a/sys/dev/acpica/acpi.c b/sys/dev/acpica/acpi.c
> index 941e2bd75aeb..a9d49a3c7f74 100644
> --- a/sys/dev/acpica/acpi.c
> +++ b/sys/dev/acpica/acpi.c
> @@ -1923,6 +1923,35 @@ acpi_find_dsd(struct acpi_device *ad)
> 	return (AE_NOT_FOUND);
> }
> 
> +static ssize_t
> +acpi_bus_get_prop_handle(const ACPI_OBJECT *hobj, void *propvalue, size_t size)
> +{
> +	ACPI_OBJECT *pobj;
> +	ACPI_HANDLE h;
> +
> +	if (hobj->Type != ACPI_TYPE_PACKAGE)
> +		goto err;
> +	if (hobj->Package.Count != 1)
> +		goto err;
> +
> +	pobj = &hobj->Package.Elements[0];
> +	if (pobj == NULL)
> +		goto err;
> +	if (pobj->Type != ACPI_TYPE_LOCAL_REFERENCE)
> +		goto err;
> +
> +	h = acpi_GetReference(NULL, pobj);
> +	if (h == NULL)
> +		goto err;
> +
> +	if (propvalue != NULL && size >= sizeof(ACPI_HANDLE))
> +		*(ACPI_HANDLE *)propvalue = h;
> +	return (sizeof(ACPI_HANDLE));
> +
> +err:
> +	return (-1);
> +}
> +
> static ssize_t
> acpi_bus_get_prop(device_t bus, device_t child, const char *propname,
>     void *propvalue, size_t size, device_property_type_t type)
> @@ -1941,6 +1970,8 @@ acpi_bus_get_prop(device_t bus, device_t child, const char *propname,
> 	case DEVICE_PROP_UINT32:
> 	case DEVICE_PROP_UINT64:
> 		break;
> +	case DEVICE_PROP_HANDLE:
> +		return (acpi_bus_get_prop_handle(obj, propvalue, size));
> 	default:
> 		return (-1);
> 	}
> @@ -1972,6 +2003,22 @@ acpi_bus_get_prop(device_t bus, device_t child, const char *propname,
> 			    MIN(size, obj->Buffer.Length));
> 		return (obj->Buffer.Length);
> 
> +	case ACPI_TYPE_PACKAGE:
> +		if (propvalue != NULL && size >= sizeof(ACPI_OBJECT *)) {
> +			*((ACPI_OBJECT **) propvalue) =
> +			    __DECONST(ACPI_OBJECT *, obj);
> +		}
> +		return (sizeof(ACPI_OBJECT *));
> +
> +	case ACPI_TYPE_LOCAL_REFERENCE:
> +		if (propvalue != NULL && size >= sizeof(ACPI_HANDLE)) {
> +			ACPI_HANDLE h;
> +
> +			h = acpi_GetReference(NULL,
> +			    __DECONST(ACPI_OBJECT *, obj));
> +			memcpy(propvalue, h, sizeof(ACPI_HANDLE));
> +		}
> +		return (sizeof(ACPI_HANDLE));
> 	default:
> 		return (0);
> 	}
> diff --git a/sys/dev/fdt/simplebus.c b/sys/dev/fdt/simplebus.c
> index ed472f87b57d..0b1277983da0 100644
> --- a/sys/dev/fdt/simplebus.c
> +++ b/sys/dev/fdt/simplebus.c
> @@ -357,7 +357,7 @@ static ssize_t
> simplebus_get_property(device_t bus, device_t child, const char *propname,
>     void *propvalue, size_t size, device_property_type_t type)
> {
> -	phandle_t node = ofw_bus_get_node(child);
> +	phandle_t node, xref;
> 	ssize_t ret, i;
> 	uint32_t *buffer;
> 	uint64_t val;
> @@ -367,11 +367,13 @@ simplebus_get_property(device_t bus, device_t child, const char *propname,
> 	case DEVICE_PROP_BUFFER:
> 	case DEVICE_PROP_UINT32:
> 	case DEVICE_PROP_UINT64:
> +	case DEVICE_PROP_HANDLE:
> 		break;
> 	default:
> 		return (-1);
> 	}
> 
> +	node = ofw_bus_get_node(child);
> 	if (propvalue == NULL || size == 0)
> 		return (OF_getproplen(node, propname));
> 
> @@ -402,7 +404,20 @@ simplebus_get_property(device_t bus, device_t child, const char *propname,
> 			((uint64_t *)buffer)[i / 2] = val;
> 		}
> 		return (ret);
> -	 }
> +	}
> +
> +	if (type == DEVICE_PROP_HANDLE) {
> +		if (size < sizeof(node))
> +			return (-1);
> +		ret = OF_getencprop(node, propname, &xref, sizeof(xref));
> +		if (ret <= 0)
> +			return (ret);
> +
> +		node = OF_node_from_xref(xref);
> +		if (propvalue != NULL)
> +			*(uint32_t *)propvalue = node;
> +		return (ret);
> +	}
> 
> 	return (OF_getprop(node, propname, propvalue, size));
> }
> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c
> index 041e77259313..2365d83ef91e 100644
> --- a/sys/kern/subr_bus.c
> +++ b/sys/kern/subr_bus.c
> @@ -2226,6 +2226,7 @@ device_get_property(device_t dev, const char *prop, void *val, size_t sz,
> 	switch (type) {
> 	case DEVICE_PROP_ANY:
> 	case DEVICE_PROP_BUFFER:
> +	case DEVICE_PROP_HANDLE:	/* Size checks done in implementation. */
> 		break;
> 	case DEVICE_PROP_UINT32:
> 		if (sz % 4 != 0)
> diff --git a/sys/sys/bus.h b/sys/sys/bus.h
> index 26d100aba222..5dead4dd382e 100644
> --- a/sys/sys/bus.h
> +++ b/sys/sys/bus.h
> @@ -72,6 +72,7 @@ typedef enum device_property_type {
> 	DEVICE_PROP_BUFFER = 1,
> 	DEVICE_PROP_UINT32 = 2,
> 	DEVICE_PROP_UINT64 = 3,
> +	DEVICE_PROP_HANDLE = 4,
> } device_property_type_t;
> 
> /**