> Date: Mon, 29 Mar 2021 21:55:46 +0200
> From: Theo Buehler <t...@theobuehler.org>
> Cc: bugs@openbsd.org
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> 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 <t...@theobuehler.org>
> > >
> > > 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 <t...@theobuehler.org>
> > > > >
> > > > > 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 <t...@theobuehler.org>
> > > > > > >
> > > > > > > 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 <t...@theobuehler.org>
> > > > > > > > >
> > > > > > > > > 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 <t...@theobuehler.org>
> > > > > > > > > >
> > > > > > > > > > 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?
Good point. I'll adjust that before I commit this.
> > 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
>
