On Thu, Mar 25, 2021 at 08:00:57AM +0100, Theo Buehler wrote:
> 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).
Sorry, I meant "signal the error from aml_rwgen()".
>
> 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
