Re: git: a9880bfe1181 - main - acpi_ged: New driver to ACPI generic event device
- In reply to: Takanori Watanabe : "git: a9880bfe1181 - main - acpi_ged: New driver to ACPI generic event device"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 24 Oct 2022 15:35:55 UTC
On 24 Oct 2022, at 11:02, Takanori Watanabe <takawata@FreeBSD.org> wrote:
>
> The branch main has been updated by takawata:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=a9880bfe1181b7a32d026339bae113f24300e5e1
>
> commit a9880bfe1181b7a32d026339bae113f24300e5e1
> Author: Takanori Watanabe <takawata@FreeBSD.org>
> AuthorDate: 2022-10-18 05:41:53 +0000
> Commit: Takanori Watanabe <takawata@FreeBSD.org>
> CommitDate: 2022-10-24 09:57:36 +0000
>
> acpi_ged: New driver to ACPI generic event device
>
> New driver to ACPI generic event device, defined in ACPI spec.
> Some ACPI power button may not work without this.
>
> In qemu arm64 with "virt" machine, with ACPI firmware,
> enable devd check devd message by
> and invoke following command in qemu monitor
> (qemu) system_powerdown
> and make sure some power button input event appear.
> (setting sysctl hw.acpi.power_button_state=S5 is not work,
> because ACPI tree does not have \_S5 object.)
>
> Reviewed by: andrew, hrs
> Differential Revision: https://reviews.freebsd.org/D37032
> ---
> share/man/man4/acpi_ged.4 | 64 +++++++++
> sys/arm64/conf/std.virt | 1 +
> sys/conf/files | 1 +
> sys/dev/acpica/acpi_ged.c | 267 +++++++++++++++++++++++++++++++++++++
> sys/modules/acpi/Makefile | 2 +-
> sys/modules/acpi/acpi_ged/Makefile | 11 ++
> 6 files changed, 345 insertions(+), 1 deletion(-)
>
> diff --git a/share/man/man4/acpi_ged.4 b/share/man/man4/acpi_ged.4
> new file mode 100644
> index 000000000000..dd62e172ae8d
> --- /dev/null
> +++ b/share/man/man4/acpi_ged.4
> @@ -0,0 +1,64 @@
> +.\" Copyright (c) 2022 Takanori Watanabe
> +.\"
> +.\" Redistribution and use in source and binary forms, with or without
> +.\" modification, are permitted provided that the following conditions
> +.\" are met:
> +.\" 1. Redistributions of source code must retain the above copyright
> +.\" notice, this list of conditions and the following disclaimer.
> +.\" 2. Redistributions in binary form must reproduce the above copyright
> +.\" notice, this list of conditions and the following disclaimer in the
> +.\" documentation and/or other materials provided with the distribution.
> +.\"
> +.\" THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> +.\" ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +.\" IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> +.\" ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> +.\" FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> +.\" DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> +.\" OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> +.\" HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> +.\" LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> +.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +.\" SUCH DAMAGE.
> +.\"
> +.\" $FreeBSD$
> +.\"
> +.Dd October 18, 2022
> +.Dt ACPI_GED 4
> +.Os
> +.Sh NAME
> +.Nm acpi_ged
> +.Nd "ACPI Generic Event Device"
> +.Sh SYNOPSIS
> +To compile this driver into the kernel,
> +place the following line in your
> +kernel configuration file:
> +.Bd -ragged -offset indent
> +.Cd "device acpi_ged"
> +.Ed
> +.Pp
> +Alternatively, to load the driver as a
> +module at boot time, place the following line in
> +.Xr loader.conf 5 :
> +.Bd -literal -offset indent
> +acpi_ged_load="YES"
> +.Ed
> +.Sh DESCRIPTION
> +The
> +.Nm
> +driver provides support for generic events interface.
> +This handles interrupts and evaluates the specific ACPI method.
> +This may generate optionally ACPI notify for another device.
> +.Sh SEE ALSO
> +.Xr acpi 4
> +.Sh HISTORY
> +The
> +.Nm
> +device driver first appeared in
> +.Fx 14.0 .
> +.Sh AUTHORS
> +.An -nosplit
> +The
> +.Nm
> +driver was written by
> +.An Takanori Watanabe Aq Mt takawata@FreeBSD.org
> diff --git a/sys/arm64/conf/std.virt b/sys/arm64/conf/std.virt
> index 507e1321f3d1..1b7d7ad8ab0a 100644
> --- a/sys/arm64/conf/std.virt
> +++ b/sys/arm64/conf/std.virt
> @@ -24,3 +24,4 @@ device vtnet # VirtIO Ethernet device
>
> options FDT
> device acpi
> +device acpi_ged
> diff --git a/sys/conf/files b/sys/conf/files
> index 39e6d4aaf0a8..563054bbac3b 100644
> --- a/sys/conf/files
> +++ b/sys/conf/files
> @@ -806,6 +806,7 @@ dev/acpica/acpi_button.c optional acpi
> dev/acpica/acpi_cmbat.c optional acpi
> dev/acpica/acpi_cpu.c optional acpi
> dev/acpica/acpi_ec.c optional acpi
> +dev/acpica/acpi_ged.c optional acpi_ged acpi
> dev/acpica/acpi_isab.c optional acpi isa
> dev/acpica/acpi_lid.c optional acpi
> dev/acpica/acpi_package.c optional acpi
> diff --git a/sys/dev/acpica/acpi_ged.c b/sys/dev/acpica/acpi_ged.c
> new file mode 100644
> index 000000000000..9459ccc3525b
> --- /dev/null
> +++ b/sys/dev/acpica/acpi_ged.c
> @@ -0,0 +1,267 @@
> +/*-
> + * Copyright (c) 2022 Takanori Watanabe
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD$");
> +
> +#include "opt_acpi.h"
> +
> +#include <sys/param.h>
> +#include <sys/bus.h>
> +#include <sys/kernel.h>
> +#include <sys/malloc.h>
> +#include <sys/module.h>
> +#include <sys/rman.h>
> +
> +#include <contrib/dev/acpica/include/acpi.h>
> +#include <contrib/dev/acpica/include/accommon.h>
> +#include <dev/acpica/acpivar.h>
> +
> +/* Hooks for the ACPI CA debugging infrastructure */
> +#define _COMPONENT ACPI_GED
> +ACPI_MODULE_NAME("GED")
> +
> +static MALLOC_DEFINE(M_ACPIGED, "acpiged", "ACPI Generic event data");
> +
> +struct acpi_ged_event {
> + device_t dev;
> + struct resource *r;
> + int rid;
> + void *cookie;
> + ACPI_HANDLE ah;
> + ACPI_OBJECT_LIST args;
> + ACPI_OBJECT arg1;
> +};
> +
> +struct acpi_ged_softc {
> + int numevts;
> + struct acpi_ged_event *evts;
> +};
> +
> +static int acpi_ged_probe(device_t dev);
> +static int acpi_ged_attach(device_t dev);
> +static int acpi_ged_detach(device_t dev);
> +
> +static char *ged_ids[] = { "ACPI0013", NULL };
> +
> +static device_method_t acpi_ged_methods[] = {
> + /* Device interface */
> + DEVMETHOD(device_probe, acpi_ged_probe),
> + DEVMETHOD(device_attach, acpi_ged_attach),
> + DEVMETHOD(device_detach, acpi_ged_detach),
> + DEVMETHOD_END
> +};
> +
> +static driver_t acpi_ged_driver = {
> + "acpi_ged",
> + acpi_ged_methods,
> + sizeof(struct acpi_ged_softc),
> +};
> +
> +DRIVER_MODULE(acpi_ged, acpi, acpi_ged_driver, 0, 0);
> +MODULE_DEPEND(acpi_ged, acpi, 1, 1, 1);
> +
> +static void
> +acpi_ged_evt(void *arg)
> +{
> + struct acpi_ged_event *evt = arg;
> +
> + AcpiEvaluateObject(evt->ah, NULL, &evt->args, NULL);
> +}
> +
> +static void
> +acpi_ged_intr(void *arg)
> +{
> + AcpiOsExecute(OSL_GPE_HANDLER, acpi_ged_evt, arg);
> +}
> +static int
Missing newline
> +acpi_ged_probe(device_t dev)
> +{
> + int rv;
> +
> + if (acpi_disabled("ged"))
> + return (ENXIO);
> + rv = ACPI_ID_PROBE(device_get_parent(dev), dev, ged_ids, NULL);
> + if (rv > 0)
> + return (ENXIO);
Why not return rv? Or just mirror most other uses and guard
device_set_desc with rv <= 0.
> +
> + device_set_desc(dev, "Generic Event Device");
> + return (rv);
> +}
> +
> +/*this should be in acpi_resource.*/
Spaces around the comment, and why not just put it there?
> +static int
> +acpi_get_trigger(ACPI_RESOURCE *res)
> +{
> + int trig;
> +
> + switch (res->Type) {
> + case ACPI_RESOURCE_TYPE_IRQ:
> + KASSERT(res->Data.Irq.InterruptCount == 1,
> + ("%s: multiple interrupts", __func__));
> + trig = res->Data.Irq.Triggering;
> + break;
> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> + KASSERT(res->Data.ExtendedIrq.InterruptCount == 1,
> + ("%s: multiple interrupts", __func__));
> + trig = res->Data.ExtendedIrq.Triggering;
> + break;
> + default:
> + panic("%s: bad resource type %u", __func__, res->Type);
> + }
> +
> + return (trig == ACPI_EDGE_SENSITIVE)
> + ? INTR_TRIGGER_EDGE : INTR_TRIGGER_LEVEL;
Parentheses should be around the whole expression not the condition.
> +}
> +
> +static int
> +acpi_ged_attach(device_t dev)
> +{
> + struct acpi_ged_softc *sc = device_get_softc(dev);
> + struct resource_list *rl;
> + struct resource_list_entry *rle;
> + ACPI_RESOURCE ares;
> + ACPI_HANDLE evt_method;
> + int i;
> + int rawirq, trig;
> + char name[] = "_Xnn";
> +
> + ACPI_FUNCTION_TRACE((char *)(uintptr_t) __func__);
> +
> + if (ACPI_FAILURE(AcpiGetHandle(acpi_get_handle(dev), "_EVT",
> + &evt_method))) {
> + device_printf(dev, "_EVT not found\n");
> + evt_method = NULL;
> + }
> +
> + rl = BUS_GET_RESOURCE_LIST(device_get_parent(dev), dev);
> + STAILQ_FOREACH(rle, rl, link) {
> + if (rle->type == SYS_RES_IRQ) {
> + sc->numevts++;
> + }
> + }
> + sc->evts = mallocarray(sc->numevts, sizeof(*sc->evts), M_ACPIGED,
> + M_WAITOK | M_ZERO);
> + for (i = 0; i < sc->numevts; i++) {
> + sc->evts[i].dev = dev;
> + sc->evts[i].rid = i;
> + sc->evts[i].r = bus_alloc_resource_any(dev, SYS_RES_IRQ,
> + &sc->evts[i].rid, RF_ACTIVE | RF_SHAREABLE);
> + if (sc->evts[i].r == NULL) {
> + device_printf(dev, "Cannot alloc %dth irq\n", i);
Not correct for i being 1 or 2 (or 21, 22, etc). Use “irq %d” instead?
> + continue;
> + }
> +#ifdef INTRNG
> + {
> + struct intr_map_data_acpi *ima;
> + ima = rman_get_virtual(sc->evts[i].r);
> + if (ima == NULL) {
> + device_printf(dev, "map not found"
> + " non-intrng?\n");
This existing seems dubious, how would it be hit?
> + rawirq = rman_get_start(sc->evts[i].r);
> + trig = INTR_TRIGGER_LEVEL;
> + if (ACPI_SUCCESS(acpi_lookup_irq_resource
> + (dev, sc->evts[i].rid,
> + sc->evts[i].r, &ares))) {
> + trig = acpi_get_trigger(&ares);
> + }
> + } else if (ima->hdr.type == INTR_MAP_DATA_ACPI) {
> + device_printf(dev, "Raw IRQ %d\n", ima->irq);
This is a debug print.
> + rawirq = ima->irq;
> + trig = ima->trig;
> + } else {
> + device_printf(dev, "Not supported intr"
> + " type%d\n", ima->hdr.type);
Space after type?
> + continue;
> + }
> + }
> +#else
> + rawirq = rman_get_start(sc->evt[i].r);
> + trig = INTR_TRIGGER_LEVEL;
> + if (ACPI_SUCCESS(acpi_lookup_irq_resource
> + (dev, sc->evts[i].rid,
> + sc->evts[i].r, &ares))) {
> + trig = acpi_get_trigger(&ares);
> + }
> +#endif
> + if (rawirq < 0x100) {
> + sprintf(name, "_%c%02X",
> + ((trig == INTR_TRIGGER_EDGE) ? 'E' : 'L'),
Condition parentheses are redundant.
> + rawirq);
> + if (ACPI_SUCCESS(AcpiGetHandle
> + (acpi_get_handle(dev),
> + name, &sc->evts[i].ah))) {
> + sc->evts[i].args.Count = 0; /* ensure */
These “ensure” comments seem meaningless?
> + } else {
> + sc->evts[i].ah = NULL; /* ensure */
> + }
> + }
> +
> + if (sc->evts[i].ah == NULL) {
> + if (evt_method != NULL) {
> + sc->evts[i].ah = evt_method;
> + sc->evts[i].arg1.Type = ACPI_TYPE_INTEGER;
> + sc->evts[i].arg1.Integer.Value = rawirq;
> + sc->evts[i].args.Count = 1;
> + sc->evts[i].args.Pointer = &sc->evts[i].arg1;
> + } else{
Space before {
> + device_printf
> + (dev,
> + "Cannot find handler method %d\n",
> + i);
I don’t think this conforms to style(9).
> + continue;
> + }
> + }
> +
> + if (bus_setup_intr(dev, sc->evts[i].r,
> + INTR_TYPE_MISC | INTR_MPSAFE, NULL, acpi_ged_intr,
> + &sc->evts[i], &sc->evts[i].cookie) != 0) {
> + device_printf(dev, "Failed to setup intr %d\n", i);
> + }
> + }
> +
> + return_VALUE(0);
Existing uses of return_VALUE put a space like return.
> +}
> +
> +static int
> +acpi_ged_detach(device_t dev)
> +{
> + struct acpi_ged_softc *sc = device_get_softc(dev);
> + int i;
> +
> + for (i = 0; i < sc->numevts; i++) {
> + if (sc->evts[i].cookie) {
!= NULL
> + bus_teardown_intr(dev, sc->evts[i].r,
> + sc->evts[i].cookie);
> + }
> + if (sc->evts[i].r) {
!= NULL
Jess
> + bus_release_resource(dev, SYS_RES_IRQ, sc->evts[i].rid,
> + sc->evts[i].r);
> + }
> + }
> + free(sc->evts, M_ACPIGED);
> +
> + return (0);
> +}
> diff --git a/sys/modules/acpi/Makefile b/sys/modules/acpi/Makefile
> index 552c4d2cbba0..71e083fef700 100644
> --- a/sys/modules/acpi/Makefile
> +++ b/sys/modules/acpi/Makefile
> @@ -1,7 +1,7 @@
> # $FreeBSD$
>
> SUBDIR= acpi_asus acpi_asus_wmi acpi_dock acpi_fujitsu acpi_hp \
> - acpi_ibm acpi_panasonic acpi_sony acpi_toshiba \
> + acpi_ged acpi_ibm acpi_panasonic acpi_sony acpi_toshiba \
> acpi_video acpi_wmi aibs
>
> .include <bsd.subdir.mk>
> diff --git a/sys/modules/acpi/acpi_ged/Makefile b/sys/modules/acpi/acpi_ged/Makefile
> new file mode 100644
> index 000000000000..a937249357f4
> --- /dev/null
> +++ b/sys/modules/acpi/acpi_ged/Makefile
> @@ -0,0 +1,11 @@
> +# $FreeBSD$
> +
> +.PATH: ${SRCTOP}/sys/dev/acpica
> +.if ${TARGET_ARCH} == aarch64
> +CFLAGS += -DINTRNG
> +.endif
> +KMOD= acpi_ged
> +SRCS= acpi_ged.c
> +SRCS+= opt_acpi.h opt_evdev.h acpi_if.h bus_if.h device_if.h
> +
> +.include <bsd.kmod.mk>