On Mon, Mar 29, 2021 at 09:25:04PM +0200, Mark Kettenis wrote:
> > Date: Thu, 25 Mar 2021 08:00:56 +0100
> > From: Theo Buehler <[email protected]>
> >
> > 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).
>
> Thanks! I think it is cleaner to provide a separate dummy
> implementation for SMALL_KERNEL. That way it is easier to see that
> the implementation will always fail if SMALL_KERNEL is defined.
This makes a lot of sense and is indeed cleaner.
> Also it turns out that ACPI allows code to do GenericSerialBus access
> without proper check. In that case "writes are ignored and reads will
> return indeterminate data".
Doesn't the "This shouldn't happen" comment need a small update to
reflect that?
> Anyway, the essentials of this diff have been in snaps for a bit. So
> it is time to ask for oks.
>
> ok?
I'm definitely in favor of this going in. I'm biased...
The dummy implementation reads fine to me. I won't be able to test it
for a couple of days since a bulk build is running on the machine. I
will report back as soon as I can. However, I don't see the point in
waiting any longer, so fwiw:
ok tb
>
>
> 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 29 Mar 2021 19:19:25 -0000
> @@ -2535,51 +2535,53 @@ 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;
> + int pos, err;
>
> 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 +2590,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,13 +2608,32 @@ 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;
>
> 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);
>
> /*
> @@ -2624,6 +2645,58 @@ aml_rwgsb(struct aml_value *conn, int al
> buf[0] = err;
> }
>
> +#else
> +
> +/*
> + * We don't support GenericSerialBus in RAMDISK kernels. Provide a
> + * dummy implementation that returns a non-zero error status.
> + */
> +
> +void
> +aml_rwgsb(struct aml_value *conn, int alen, int bpos, int blen,
> + struct aml_value *val, int mode, int flag)
> +{
> + int buflen;
> + uint8_t *buf;
> +
> + if (AML_FIELD_ACCESS(flag) != AML_FIELD_BUFFERACC ||
> + bpos & 0x3 || (blen % 8) != 0)
> + aml_die("Invalid GenericSerialBus access");
> +
> + switch (((flag >> 6) & 0x3)) {
> + case 0: /* Normal */
> + switch (AML_FIELD_ATTR(flag)) {
> + case 0x02: /* AttribQuick */
> + buflen = 0;
> + break;
> + case 0x04: /* AttribSendReceive */
> + case 0x06: /* AttribByte */
> + case 0x08: /* AttribWord */
> + case 0x0b: /* AttribBytes */
> + case 0x0e: /* AttribRawBytes */
> + buflen = blen / 8;
> + break;
> + default:
> + aml_die("unsupported access type 0x%x", flag);
> + break;
> + }
> + break;
> + case 1: /* AttribBytes */
> + case 2: /* AttribRawBytes */
> + buflen = AML_FIELD_ATTR(flag);
> + break;
> + default:
> + aml_die("unsupported access type 0x%x", flag);
> + break;
> + }
> +
> + if (mode == ACPI_IOREAD)
> + _aml_setvalue(val, AML_OBJTYPE_BUFFER, buflen + 2, NULL);
> +
> + buf = val->v_buffer;
> + buf[0] = EIO;
> +}
> +
> #endif
>
> void
> @@ -2727,13 +2800,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 29 Mar 2021 19:19:25 -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
