Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: [patch] Re: acpi: panic in aml_convert() on 2025 Framework Laptop 13
To:
Sven M. Hallberg <pesco@khjk.org>
Cc:
tech@openbsd.org, bugs@openbsd.org
Date:
Sat, 07 Jun 2025 18:49:17 +0200

Download raw body.

Thread
> From: Sven M. Hallberg <pesco@khjk.org>
> Date: Sat, 07 Jun 2025 01:16:12 +0200

Hi Sven,

Thanks for the well-written bug report and this fix.

The description of the comparison operators isn't very clear as it
suggests that only the 2nd source operand will be converted.  ut it
also doesn't say that the 1st source operand must be interger, buffer
or string.  Only that it must evaluate to one of those types.

So your interpretation makes sense.  And the patch looks good.  I have
one minor nit.  I'm not a big fan on assignments within if statements
so I propose the diff below instead.

ok?

> With the attached patch, the machine boots. :)
> 
> Installer works. X works. I'll send a separate email to dmesg@ with more
> details.
> 
> 
> Sven M. Hallberg on Fri, Jun 06 2025:
> > 	Attempting to boot on the recent Framework Laptop 13 with AMD Ryzen AI
> > 	300 series processor leads to a panic with the following message:
> >
> > 		Could not convert 1 to 5
> >
> > 		panic: aml_die aml_convert:2094
> >
> > 	The source is in /sys/dev/acpi/dsdt.c (line 2094). Some investigation
> > 	shows that aml_compare() is trying to convert an AML_OBJTYPE_INTEGER (1)
> > 	to AML_OBJTYPE_FIELDUNIT (5) while handling an AMLOP_LEQUAL in
> > 	aml_parse(). This conversion case is not implemented.
> >
> > 	The implementation of AMLOP_CONCAT in aml_concat() has the same issue
> > 	and implicit conversions might be missing in other cases (see
> > 	explanations below). The arithmetic operators, for instance, seem to
> > 	require their arguments to be integers without a call to aml_convert().
> >
> >       [...]
> >
> > 	For type Field Unit, Table 19.6 lists all possible conversions.
> > 	Technically, any missing cases should be added to aml_convert().
> > 	To fix this particular case, at least the conversion from Field Unit
> > 	to Integer must be implemented.
> >
> > 	All the arithmetic comparison operators accept integer, string, or
> > 	buffer arguments. The aml_compare() function should try to convert the
> > 	first operand to these types in order, before the existing call to
> > 	aml_convert() for the second operand. Similar for aml_concat().
> 
> 

Index: dev/acpi/dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
diff -u -p -r1.274 dsdt.c
--- dev/acpi/dsdt.c	22 Mar 2025 18:14:37 -0000	1.274
+++ dev/acpi/dsdt.c	7 Jun 2025 16:46:49 -0000
@@ -1753,6 +1753,7 @@ struct aml_scope *aml_pushscope(struct a
 struct aml_scope *aml_popscope(struct aml_scope *);
 
 void		aml_showstack(struct aml_scope *);
+struct aml_value *aml_tryconv(struct aml_value *, int, int);
 struct aml_value *aml_convert(struct aml_value *, int, int);
 
 int		aml_matchtest(int64_t, int64_t, int);
@@ -1766,6 +1767,8 @@ int		aml_ccrlen(int, union acpi_resource
 
 void		aml_store(struct aml_scope *, struct aml_value *, int64_t,
     struct aml_value *);
+void		aml_rwfield(struct aml_value *, int, int, struct aml_value *,
+    int);
 
 /*
  * Reference Count functions
@@ -2022,7 +2025,7 @@ aml_hextoint(const char *str)
 }
 
 struct aml_value *
-aml_convert(struct aml_value *a, int ctype, int clen)
+aml_tryconv(struct aml_value *a, int ctype, int clen)
 {
 	struct aml_value *c = NULL;
 
@@ -2045,6 +2048,11 @@ aml_convert(struct aml_value *a, int cty
 			c = aml_allocvalue(AML_OBJTYPE_BUFFER, a->length,
 			    a->v_string);
 			break;
+		case AML_OBJTYPE_BUFFERFIELD:
+		case AML_OBJTYPE_FIELDUNIT:
+			c = aml_allocvalue(AML_OBJTYPE_BUFFER, 0, NULL);
+			aml_rwfield(a, 0, a->v_field.bitlen, c, ACPI_IOREAD);
+			break;
 		}
 		break;
 	case AML_OBJTYPE_INTEGER:
@@ -2062,6 +2070,13 @@ aml_convert(struct aml_value *a, int cty
 		case AML_OBJTYPE_UNINITIALIZED:
 			c = aml_allocvalue(AML_OBJTYPE_INTEGER, 0, NULL);
 			break;
+		case AML_OBJTYPE_BUFFERFIELD:
+		case AML_OBJTYPE_FIELDUNIT:
+			if (a->v_field.bitlen > aml_intlen)
+				break;
+			c = aml_allocvalue(AML_OBJTYPE_INTEGER, 0, NULL);
+			aml_rwfield(a, 0, a->v_field.bitlen, c, ACPI_IOREAD);
+			break;
 		}
 		break;
 	case AML_OBJTYPE_STRING:
@@ -2087,6 +2102,15 @@ aml_convert(struct aml_value *a, int cty
 		}
 		break;
 	}
+	return c;
+}
+
+struct aml_value *
+aml_convert(struct aml_value *a, int ctype, int clen)
+{
+	struct aml_value *c;
+
+	c = aml_tryconv(a, ctype, clen);
 	if (c == NULL) {
 #ifndef SMALL_KERNEL
 		aml_showvalue(a);
@@ -2099,8 +2123,26 @@ aml_convert(struct aml_value *a, int cty
 int
 aml_compare(struct aml_value *a1, struct aml_value *a2, int opcode)
 {
+	struct aml_value *cv;		/* value after conversion */
 	int rc = 0;
 
+	/*
+	 * Convert A1 to integer, string, or buffer.
+	 *
+	 * The possible conversions listed in Table 19.6 of the ACPI spec
+	 * imply that unless we already got one of the three supported types,
+	 * the conversion must be from field unit or buffer field. In both
+	 * cases, the rules (Table 19.7) state that we should convert to
+	 * integer if possible with buffer as a fallback.
+	 */
+	if (a1->type != AML_OBJTYPE_INTEGER && a1->type != AML_OBJTYPE_STRING
+	    && a1->type != AML_OBJTYPE_BUFFER) {
+		cv = aml_tryconv(a1, AML_OBJTYPE_INTEGER, -1);
+		if (cv == NULL)
+			cv = aml_convert(a1, AML_OBJTYPE_BUFFER, -1);
+		a1 = cv;
+	}
+
 	/* Convert A2 to type of A1 */
 	a2 = aml_convert(a2, a1->type, -1);
 	if (a1->type == AML_OBJTYPE_INTEGER)
@@ -2127,6 +2169,14 @@ aml_concat(struct aml_value *a1, struct 
 {
 	struct aml_value *c = NULL;
 
+	/*
+	 * Make A1 an integer, string, or buffer. Unless we already got one
+	 * of these three types, convert to string.
+	 */
+	if (a1->type != AML_OBJTYPE_INTEGER && a1->type != AML_OBJTYPE_STRING
+	    && a1->type != AML_OBJTYPE_BUFFER)
+		a1 = aml_convert(a1, AML_OBJTYPE_STRING, -1);
+
 	/* Convert arg2 to type of arg1 */
 	a2 = aml_convert(a2, a1->type, -1);
 	switch (a1->type) {
@@ -2292,7 +2342,6 @@ void aml_rwgen(struct aml_value *, int, 
 void aml_rwgpio(struct aml_value *, int, int, struct aml_value *, int, int);
 void aml_rwgsb(struct aml_value *, int, int, int, struct aml_value *, int, int);
 void aml_rwindexfield(struct aml_value *, struct aml_value *val, int);
-void aml_rwfield(struct aml_value *, int, int, struct aml_value *, int);
 
 /* Get PCI address for opregion objects */
 int