Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 12:15:58PM -0800, Philip Guenther wrote: > On Mon, Jan 24, 2022 at 11:41 AM Mark Kettenis > wrote: > > > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > > From: Anton Lindqvist > > > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > > Currently we attach ACPI devices that are present in a machine. > > > > However, in some cases ACPI devices can be present, but not enabled. > > > > Attaching a device driver to devices that are not enabled is not a > > > > good idea since reading and writing from/to its registers will fail > > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > > serial port that is misdetected and hangs the kernel when it is > > > > actually opened. > > > > > > > > The diff below makes sure we only enable devices that are actually > > > > enabled. This may cause some devices to disappear in OpenBSD. > > > > However those devices should have been unusable anyway, so that isn't > > > > an issue. > > > > > > > > ok? > > > > > > According to the ACPI specification[1]: > > > > > > > A device can only decode its hardware resources if both bits 0 and 1 > > are > > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > > [1] cleared), then the device must not decode its resources. > > > > Just before that it says: > > > > If bit [0] is cleared, then bit 1 must also be cleared (in other > > words, a device that is not present cannot be enabled). > > > > > Should we therefore check for presence of both STA_PRESENT and > > > STA_ENABLED? > > > > So according to the ACPI specification we don't need to do that. > > Should we do it just to be safe? > > > > Unless you're taking money bets about this being the one thing in the ACPI > spec that some vendor won't screw up, doing both seems "can't be worse; can > be better". > > Philip A bit late to the party here, but we should all remember the HP bios with the FACS table version of "1" and not 1. (0x31 and not 0x01). So yes, someone will indeed screw it up if it's possible to screw it up.
Re: Properly check if ACPI devices are enabled
> Date: Tue, 25 Jan 2022 19:44:23 +0100 (CET) > From: Mark Kettenis > > > Date: Tue, 25 Jan 2022 07:01:41 +0100 > > From: Anton Lindqvist > > > > On Mon, Jan 24, 2022 at 08:40:36PM +0100, Mark Kettenis wrote: > > > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > > > From: Anton Lindqvist > > > > > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > > > Currently we attach ACPI devices that are present in a machine. > > > > > However, in some cases ACPI devices can be present, but not enabled. > > > > > Attaching a device driver to devices that are not enabled is not a > > > > > good idea since reading and writing from/to its registers will fail > > > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > > > serial port that is misdetected and hangs the kernel when it is > > > > > actually opened. > > > > > > > > > > The diff below makes sure we only enable devices that are actually > > > > > enabled. This may cause some devices to disappear in OpenBSD. > > > > > However those devices should have been unusable anyway, so that isn't > > > > > an issue. > > > > > > > > > > ok? > > > > > > > > According to the ACPI specification[1]: > > > > > > > > > A device can only decode its hardware resources if both bits 0 and 1 > > > > > are > > > > > set. If the device is not present (bit [0] cleared) or not enabled > > > > > (bit > > > > > [1] cleared), then the device must not decode its resources. > > > > > > Just before that it says: > > > > > > If bit [0] is cleared, then bit 1 must also be cleared (in other > > > words, a device that is not present cannot be enabled). > > > > > > > Should we therefore check for presence of both STA_PRESENT and > > > > STA_ENABLED? > > > > > > So according to the ACPI specification we don't need to do that. > > > Should we do it just to be safe? > > > > I would still vote for it. > > Since several people pointed this out, here is a diff that checks both > bits. > > ok? No. As a fellow developer pointed out, this is wrong. Correct diff below. Index: dev/acpi/acpi.c === RCS file: /cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.405 diff -u -p -r1.405 acpi.c --- dev/acpi/acpi.c 12 Jan 2022 11:18:30 - 1.405 +++ dev/acpi/acpi.c 25 Jan 2022 18:59:52 - @@ -3321,7 +3321,7 @@ acpi_foundhid(struct aml_node *node, voi return (0); sta = acpi_getsta(sc, node->parent); - if ((sta & STA_PRESENT) == 0) + if ((sta & (STA_PRESENT | STA_ENABLED)) != (STA_PRESENT | STA_ENABLED)) return (0); if (aml_evalinteger(sc, node->parent, "_CCA", 0, NULL, &cca))
Re: Properly check if ACPI devices are enabled
> Date: Tue, 25 Jan 2022 07:01:41 +0100 > From: Anton Lindqvist > > On Mon, Jan 24, 2022 at 08:40:36PM +0100, Mark Kettenis wrote: > > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > > From: Anton Lindqvist > > > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > > Currently we attach ACPI devices that are present in a machine. > > > > However, in some cases ACPI devices can be present, but not enabled. > > > > Attaching a device driver to devices that are not enabled is not a > > > > good idea since reading and writing from/to its registers will fail > > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > > serial port that is misdetected and hangs the kernel when it is > > > > actually opened. > > > > > > > > The diff below makes sure we only enable devices that are actually > > > > enabled. This may cause some devices to disappear in OpenBSD. > > > > However those devices should have been unusable anyway, so that isn't > > > > an issue. > > > > > > > > ok? > > > > > > According to the ACPI specification[1]: > > > > > > > A device can only decode its hardware resources if both bits 0 and 1 are > > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > > [1] cleared), then the device must not decode its resources. > > > > Just before that it says: > > > > If bit [0] is cleared, then bit 1 must also be cleared (in other > > words, a device that is not present cannot be enabled). > > > > > Should we therefore check for presence of both STA_PRESENT and > > > STA_ENABLED? > > > > So according to the ACPI specification we don't need to do that. > > Should we do it just to be safe? > > I would still vote for it. Since several people pointed this out, here is a diff that checks both bits. ok? Index: dev/acpi/acpi.c === RCS file: /cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.405 diff -u -p -r1.405 acpi.c --- dev/acpi/acpi.c 12 Jan 2022 11:18:30 - 1.405 +++ dev/acpi/acpi.c 25 Jan 2022 18:43:25 - @@ -3321,7 +3321,7 @@ acpi_foundhid(struct aml_node *node, voi return (0); sta = acpi_getsta(sc, node->parent); - if ((sta & STA_PRESENT) == 0) + if ((sta & (STA_PRESENT | STA_ENABLED)) == (STA_PRESENT | STA_ENABLED)) return (0); if (aml_evalinteger(sc, node->parent, "_CCA", 0, NULL, &cca))
Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 08:40:36PM +0100, Mark Kettenis wrote: > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > From: Anton Lindqvist > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > Currently we attach ACPI devices that are present in a machine. > > > However, in some cases ACPI devices can be present, but not enabled. > > > Attaching a device driver to devices that are not enabled is not a > > > good idea since reading and writing from/to its registers will fail > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > serial port that is misdetected and hangs the kernel when it is > > > actually opened. > > > > > > The diff below makes sure we only enable devices that are actually > > > enabled. This may cause some devices to disappear in OpenBSD. > > > However those devices should have been unusable anyway, so that isn't > > > an issue. > > > > > > ok? > > > > According to the ACPI specification[1]: > > > > > A device can only decode its hardware resources if both bits 0 and 1 are > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > [1] cleared), then the device must not decode its resources. > > Just before that it says: > > If bit [0] is cleared, then bit 1 must also be cleared (in other > words, a device that is not present cannot be enabled). > > > Should we therefore check for presence of both STA_PRESENT and > > STA_ENABLED? > > So according to the ACPI specification we don't need to do that. > Should we do it just to be safe? I would still vote for it.
Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 11:41 AM Mark Kettenis wrote: > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > From: Anton Lindqvist > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > Currently we attach ACPI devices that are present in a machine. > > > However, in some cases ACPI devices can be present, but not enabled. > > > Attaching a device driver to devices that are not enabled is not a > > > good idea since reading and writing from/to its registers will fail > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > serial port that is misdetected and hangs the kernel when it is > > > actually opened. > > > > > > The diff below makes sure we only enable devices that are actually > > > enabled. This may cause some devices to disappear in OpenBSD. > > > However those devices should have been unusable anyway, so that isn't > > > an issue. > > > > > > ok? > > > > According to the ACPI specification[1]: > > > > > A device can only decode its hardware resources if both bits 0 and 1 > are > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > [1] cleared), then the device must not decode its resources. > > Just before that it says: > > If bit [0] is cleared, then bit 1 must also be cleared (in other > words, a device that is not present cannot be enabled). > > > Should we therefore check for presence of both STA_PRESENT and > > STA_ENABLED? > > So according to the ACPI specification we don't need to do that. > Should we do it just to be safe? > Unless you're taking money bets about this being the one thing in the ACPI spec that some vendor won't screw up, doing both seems "can't be worse; can be better". Philip
Re: Properly check if ACPI devices are enabled
> Date: Mon, 24 Jan 2022 20:19:46 +0100 > From: Anton Lindqvist > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > Currently we attach ACPI devices that are present in a machine. > > However, in some cases ACPI devices can be present, but not enabled. > > Attaching a device driver to devices that are not enabled is not a > > good idea since reading and writing from/to its registers will fail > > and the driver will malfunction in interesting ways. Such as a com(4) > > serial port that is misdetected and hangs the kernel when it is > > actually opened. > > > > The diff below makes sure we only enable devices that are actually > > enabled. This may cause some devices to disappear in OpenBSD. > > However those devices should have been unusable anyway, so that isn't > > an issue. > > > > ok? > > According to the ACPI specification[1]: > > > A device can only decode its hardware resources if both bits 0 and 1 are > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > [1] cleared), then the device must not decode its resources. Just before that it says: If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled). > Should we therefore check for presence of both STA_PRESENT and > STA_ENABLED? So according to the ACPI specification we don't need to do that. Should we do it just to be safe? > [1] > https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#device-insertion-removal-and-status-objects
Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > Currently we attach ACPI devices that are present in a machine. > However, in some cases ACPI devices can be present, but not enabled. > Attaching a device driver to devices that are not enabled is not a > good idea since reading and writing from/to its registers will fail > and the driver will malfunction in interesting ways. Such as a com(4) > serial port that is misdetected and hangs the kernel when it is > actually opened. > > The diff below makes sure we only enable devices that are actually > enabled. This may cause some devices to disappear in OpenBSD. > However those devices should have been unusable anyway, so that isn't > an issue. > > ok? According to the ACPI specification[1]: > A device can only decode its hardware resources if both bits 0 and 1 are > set. If the device is not present (bit [0] cleared) or not enabled (bit > [1] cleared), then the device must not decode its resources. Should we therefore check for presence of both STA_PRESENT and STA_ENABLED? [1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#device-insertion-removal-and-status-objects
Properly check if ACPI devices are enabled
Currently we attach ACPI devices that are present in a machine. However, in some cases ACPI devices can be present, but not enabled. Attaching a device driver to devices that are not enabled is not a good idea since reading and writing from/to its registers will fail and the driver will malfunction in interesting ways. Such as a com(4) serial port that is misdetected and hangs the kernel when it is actually opened. The diff below makes sure we only enable devices that are actually enabled. This may cause some devices to disappear in OpenBSD. However those devices should have been unusable anyway, so that isn't an issue. ok? Index: dev/acpi/acpi.c === RCS file: /cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.405 diff -u -p -r1.405 acpi.c --- dev/acpi/acpi.c 12 Jan 2022 11:18:30 - 1.405 +++ dev/acpi/acpi.c 23 Jan 2022 17:13:03 - @@ -3321,7 +3321,7 @@ acpi_foundhid(struct aml_node *node, voi return (0); sta = acpi_getsta(sc, node->parent); - if ((sta & STA_PRESENT) == 0) + if ((sta & STA_ENABLED) == 0) return (0); if (aml_evalinteger(sc, node->parent, "_CCA", 0, NULL, &cca))
