On Wed, Mar 24, 2021 at 05:11:52PM +0100, Mark Kettenis wrote:
> > Date: Wed, 24 Mar 2021 09:47:56 +0100
> > From: Theo Buehler <[email protected]>
> >
> > On Tue, Mar 23, 2021 at 08:29:29PM +0100, Mark Kettenis wrote:
> > > > Date: Tue, 23 Mar 2021 17:39:45 +0100
> > > > From: Theo Buehler <[email protected]>
> > > >
> > > > On Tue, Mar 23, 2021 at 05:28:37PM +0100, Mark Kettenis wrote:
> > > > > > Date: Tue, 23 Mar 2021 16:56:33 +0100
> > > > > > From: Theo Buehler <[email protected]>
> > > > > >
> > > > > > On Tue, Mar 23, 2021 at 04:13:53PM +0100, Mark Kettenis wrote:
> > > > > > > > Date: Tue, 23 Mar 2021 14:14:40 +0100
> > > > > > > > From: Theo Buehler <[email protected]>
> > > > > > >
> > > > > > > It would help if you could try and boot a kernel that adds some
> > > > > > > debug
> > > > > > > prints instead of calling aml_die(). Probably need to know the
> > > > > > > values
> > > > > > > of alen, bpos, blen, mode and flag for starters.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > alen 0x00, bpos 0x278, blen 0x10, mode 0x00, flag 0x605
> > > > > >
> > > > > > So: AML_FIELD_ACCESS(flag) == AML_FIELD_BUFFER_ACC, bpos & 0x03 == 0
> > > > > > and the aml_die("Invalid GenericSerialBus access") is hit because
> > > > > > blen
> > > > > > is twice as long as it should be according to the conditional
> > > > > > preceding
> > > > > > it:
> > > > > >
> > > > > > if (AML_FIELD_ACCESS(flag) != AML_FIELD_BUFFERACC ||
> > > > > > bpos & 0x3 || blen != 8)
> > > > >
> > > > > Right, we need to figure out what this actually means. The ACPI
> > > > > standard isn't exactly clear on this...
> > > > >
> > > > > > If I skip the aml_die("Invalid GenericSerialBus access"), it hits
> > > > > > the
> > > > > > next aml_die("Could not find GenericSerialBus controller"); because
> > > > > > node->i2c == NULL.
> > > > >
> > > > > If I'm reading the AML and FreeBSD dmesg correctly this is an i2c
> > > > > controller that attaches to PCI. I suspect that it is dwiic(4). D
> > > > > you see dwiic(4) attach?
> > > >
> > > > Two of them:
> > > >
> > > > dwiic0 at pci0 dev 21 function 0 "Intel 400 Series I2C" rev 0x00: apic
> > > > 2 int 22
> > > > iic0 at dwiic0
> > > > dwiic1 at pci0 dev 21 function 1 "Intel 400 Series I2C" rev 0x00: apic
> > > > 2 int 23
> > > > iic1 at dwiic1
> > > >
> > > > > If so, the problem is that we dont't call acpi_register_gsb() for
> > > > > dwiic(4) instances that attach to PCI. I'll see if I can come up with
> > > > > a diff for that.
> > > >
> > > > Thanks a lot!
> > >
> > > Can you try the diff below?
> >
> > I tried to install a release built with this. With miniroot69.img I
> > still hit the reboot loop described in my first mail:
> >
> > ...
> > wsdisplay0 at efifb0 mux 1: console (std, vt100 emulation), using wskbd0
> > aml_rwgen: unregistered RegionSpace 0x9
> > Could not convert 0 to 3
> >
> > panic: aml_die aml_convert: 2095
> > syncing disks...uvmfault(0xffffffff818e5d78, 0xbc8, 0, 1) -> e
> > fatal page fault in supervisor mode
> > trap type 6 code 0 rip ffffffff810e216b cs 8 rflags 10286 cr2 bc8 cpl 0 rsp
> > ffff80001f76f120
> > gsbase 0xffffffff818d0ff0 kgsbase 0x0
> > panic trap type 6, code=0, pc=ffffffff810e216b
> >
> > dump to dev 17,1 not possible
> > rebooting...
> >
> > I tried to move acpi_register_gsb() and the call to it in dwiic_pci.c
> > out of SMALL_KERNEL, but that didn't help.
> >
> > What info would be useful to debug this further?
>
> SMALL_KERNEL deliberately doesn't GenericSerialBus support as this
> requires the presence of a whole set of I2C drivers that we have no
> space for on the install media.
>
> This machine violates the ACPI spec, since the AML should check that
> the i2c bus is actually available before using it. But pointing this
> out to Dell is probably not going to help.
>
> Here is a diff that may work better. The GenericSerialBuffer stuff
> allows us to propagate the status of an I2C transfer back up the
> chain. So here is a diff that simply returns EIO if the controller
> can't be found.
The new blen and EIO logic isn't included in bsd.rd since aml_rwgsb() is
inside #ifndef SMALL_KERNEL. We instead fall back to aml_rwgen() in the
switch over ref1->v_opregion.iospace in aml_rwfield().
The diff below gives me a working bsd.rd. It pushes SMALL_KERNEL down a
bit so as to only guard iic_exec(), but perhaps it would be preferable
to signal the error from aml_rwfield() instead (I don't know how).
Index: dev/acpi/dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.261
diff -u -p -r1.261 dsdt.c
--- dev/acpi/dsdt.c 18 Mar 2021 00:17:26 -0000 1.261
+++ dev/acpi/dsdt.c 25 Mar 2021 06:44:48 -0000
@@ -2524,8 +2524,6 @@ aml_rwgpio(struct aml_value *conn, int b
}
}
-#ifndef SMALL_KERNEL
-
void
aml_rwgsb(struct aml_value *conn, int alen, int bpos, int blen,
struct aml_value *val, int mode, int flag)
@@ -2535,51 +2533,56 @@ aml_rwgsb(struct aml_value *conn, int al
i2c_tag_t tag;
i2c_op_t op;
i2c_addr_t addr;
- int cmdlen, buflen;
+ int cmdlen, buflen, acclen;
uint8_t cmd;
uint8_t *buf;
- int err;
+#ifndef SMALL_KERNEL
+ int pos;
+#endif
+ int err = 0;
if (conn->type != AML_OBJTYPE_BUFFER || conn->length < 5 ||
AML_CRSTYPE(crs) != LR_SERBUS || AML_CRSLEN(crs) > conn->length ||
crs->lr_i2cbus.revid != 1 || crs->lr_i2cbus.type != LR_SERBUS_I2C)
aml_die("Invalid GenericSerialBus");
if (AML_FIELD_ACCESS(flag) != AML_FIELD_BUFFERACC ||
- bpos & 0x3 || blen != 8)
+ bpos & 0x3 || (blen % 8) != 0)
aml_die("Invalid GenericSerialBus access");
node = aml_searchname(conn->node,
(char *)&crs->lr_i2cbus.vdata[crs->lr_i2cbus.tlength - 6]);
- if (node == NULL || node->i2c == NULL)
- aml_die("Could not find GenericSerialBus controller");
-
switch (((flag >> 6) & 0x3)) {
case 0: /* Normal */
switch (AML_FIELD_ATTR(flag)) {
case 0x02: /* AttribQuick */
cmdlen = 0;
- buflen = 0;
+ buflen = acclen = 0;
break;
case 0x04: /* AttribSendReceive */
cmdlen = 0;
- buflen = 1;
+ acclen = 1;
+ buflen = blen / 8;
break;
case 0x06: /* AttribByte */
cmdlen = 1;
- buflen = 1;
+ acclen = 1;
+ buflen = blen / 8;
break;
case 0x08: /* AttribWord */
cmdlen = 1;
- buflen = 2;
+ acclen = 2;
+ buflen = blen / 8;
break;
case 0x0b: /* AttribBytes */
cmdlen = 1;
- buflen = alen;
+ acclen = alen;
+ buflen = blen / 8;
break;
case 0x0e: /* AttribRawBytes */
cmdlen = 0;
- buflen = alen;
+ acclen = alen;
+ buflen = blen / 8;
break;
default:
aml_die("unsupported access type 0x%x", flag);
@@ -2588,11 +2591,11 @@ aml_rwgsb(struct aml_value *conn, int al
break;
case 1: /* AttribBytes */
cmdlen = 1;
- buflen = AML_FIELD_ATTR(flag);
+ acclen = buflen = AML_FIELD_ATTR(flag);
break;
case 2: /* AttribRawBytes */
cmdlen = 0;
- buflen = AML_FIELD_ATTR(flag);
+ acclen = buflen = AML_FIELD_ATTR(flag);
break;
default:
aml_die("unsupported access type 0x%x", flag);
@@ -2606,14 +2609,35 @@ aml_rwgsb(struct aml_value *conn, int al
op = I2C_OP_WRITE_WITH_STOP;
}
+ buf = val->v_buffer;
+
+ /*
+ * Return an error if we can't find the I2C controller that
+ * we're supposed to use for this request. This shouldn't
+ * happen as AML should only make use of GenericSerialBus
+ * address space after verifying that support for it has been
+ * registered.
+ */
+ if (node == NULL || node->i2c == NULL) {
+ buf[0] = EIO;
+ return;
+ }
+
tag = node->i2c;
addr = crs->lr_i2cbus._adr;
cmd = bpos >> 3;
- buf = val->v_buffer;
+#ifndef SMALL_KERNEL
iic_acquire_bus(tag, 0);
- err = iic_exec(tag, op, addr, &cmd, cmdlen, &buf[2], buflen, 0);
+ for (pos = 0; pos < buflen; pos += acclen) {
+ err = iic_exec(tag, op, addr, &cmd, cmdlen,
+ &buf[pos + 2], acclen, 0);
+ if (err)
+ break;
+ cmd++;
+ }
iic_release_bus(tag, 0);
+#endif
/*
* The ACPI specification doesn't tell us what the status
@@ -2624,8 +2648,6 @@ aml_rwgsb(struct aml_value *conn, int al
buf[0] = err;
}
-#endif
-
void
aml_rwindexfield(struct aml_value *fld, struct aml_value *val, int mode)
{
@@ -2727,13 +2749,11 @@ aml_rwfield(struct aml_value *fld, int b
aml_rwgpio(ref2, bpos, blen, val, mode,
fld->v_field.flags);
break;
-#ifndef SMALL_KERNEL
case ACPI_OPREG_GSB:
aml_rwgsb(ref2, fld->v_field.ref3,
fld->v_field.bitpos + bpos, blen,
val, mode, fld->v_field.flags);
break;
-#endif
default:
aml_rwgen(ref1, fld->v_field.bitpos + bpos, blen,
val, mode, fld->v_field.flags);
Index: dev/pci/dwiic_pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/dwiic_pci.c,v
retrieving revision 1.15
diff -u -p -r1.15 dwiic_pci.c
--- dev/pci/dwiic_pci.c 25 Dec 2020 21:48:27 -0000 1.15
+++ dev/pci/dwiic_pci.c 24 Mar 2021 18:48:46 -0000
@@ -225,7 +225,12 @@ dwiic_pci_attach(struct device *parent,
config_found((struct device *)sc, &sc->sc_iba, iicbus_print);
- return;
+#if NACPI > 0 && !defined(SMALL_KERNEL)
+ if (sc->sc_devnode) {
+ sc->sc_devnode->i2c = &sc->sc_i2c_tag;
+ acpi_register_gsb(acpi_softc, sc->sc_devnode);
+ }
+#endif
}
int
