On Sat, Mar 12, 2016 at 07:57:16PM +0100, Mark Kettenis wrote:
> Simply ignoring the SMBALERT_STS bit, like your latest diff does, is a
> bit dangerous. It seems that on later generations of the controller
> there is a SMBALERT_DIS bit in the SLV_CMD register that determines
> whether SMBALERT_STS actually causes an interrupt or not. I suppose
> this bit is set on your board. But it may be cleared on other
> hardware. And on such hardware, simply ignoring the SMBALERT_STS will
> trigger an interrupt storm. Not sure if clearing the SMBALERT_STS bit
> is enough to prevent such a storm, but I think we should at least try
> to do that, like you did in one of the earlier diffs.
This diff is printing SLV_CMD (Slave Command I/O Register, Offset 11h)
from ichiic_intr in addition to interrupt status:
Index: ichreg.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/ichreg.h,v
retrieving revision 1.7
diff -u -p -r1.7 ichreg.h
--- ichreg.h 18 Dec 2005 12:09:04 -0000 1.7
+++ ichreg.h 12 Mar 2016 19:05:38 -0000
@@ -113,6 +113,7 @@
#define ICH_SMB_SCMD_INTREN (1 << 0) /* enable interrupts on HN */
#define ICH_SMB_SCMD_WKEN (1 << 1) /* wake on HN */
#define ICH_SMB_SCMD_SMBALDS (1 << 2) /* disable SMBALERT# intr */
+#define ICH_SMB_SCMD_BITS "\020\001INTREN\002WKEN\003SMBALDS"
#define ICH_SMB_NDADDR 0x14 /* notify device address */
#define ICH_SMB_NDADDR_ADDR(x) ((x) >> 1) /* 7-bit address */
#define ICH_SMB_NDLOW 0x16 /* notify data low byte */
Index: ichiic.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/ichiic.c,v
retrieving revision 1.37
diff -u -p -r1.37 ichiic.c
--- ichiic.c 7 Dec 2015 02:56:36 -0000 1.37
+++ ichiic.c 12 Mar 2016 19:33:04 -0000
@@ -346,8 +346,11 @@ ichiic_intr(void *arg)
/* Interrupt was not for us */
return (0);
- DPRINTF(("%s: intr st 0x%b\n", sc->sc_dev.dv_xname, st,
- ICH_SMB_HS_BITS));
+ printf("%s: intr st 0x%b\n", sc->sc_dev.dv_xname, st,
+ ICH_SMB_HS_BITS);
+ printf("%s: slave cmd 0x%b\n", sc->sc_dev.dv_xname,
+ bus_space_read_1(sc->sc_iot, sc->sc_ioh, ICH_SMB_SCMD),
+ ICH_SMB_SCMD_BITS);
/* Clear status bits */
bus_space_write_1(sc->sc_iot, sc->sc_ioh, ICH_SMB_HS, st);
With the .03 (buggy) BIOS, I see:
ichiic0: intr st 0x20<SMBAL>
ichiic0: slave cmd 0x0
ichiic0: intr st 0x60<SMBAL,INUSE>
ichiic0: slave cmd 0x0
ichiic0: intr st 0x20<SMBAL>
ichiic0: slave cmd 0x0
ichiic0: intr st 0x60<SMBAL,INUSE>
ichiic0: slave cmd 0x0
With the .08 (good) BIOS, I see:
ichiic0: intr st 0x42<INTR,INUSE>
ichiic0: slave cmd 0x0
ichiic0: intr st 0x42<INTR,INUSE>
ichiic0: slave cmd 0x0
ichiic0: intr st 0x42<INTR,INUSE>
ichiic0: slave cmd 0x0
ichiic0: intr st 0x42<INTR,INUSE>
ichiic0: slave cmd 0x0
So the SMBALERT_DIS bit is always cleared on this board.
I don't think we can infer from any of this how other hardware will behave.
I guess you would prefer this version for commit, then?
Index: ichiic.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/ichiic.c,v
retrieving revision 1.37
diff -u -p -r1.37 ichiic.c
--- ichiic.c 7 Dec 2015 02:56:36 -0000 1.37
+++ ichiic.c 12 Mar 2016 19:45:25 -0000
@@ -340,17 +340,19 @@ ichiic_intr(void *arg)
/* Read status */
st = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ICH_SMB_HS);
+
+ /* Clear status bits */
+ bus_space_write_1(sc->sc_iot, sc->sc_ioh, ICH_SMB_HS, st);
+
+ /* XXX Ignore SMBALERT# for now */
if ((st & ICH_SMB_HS_BUSY) != 0 || (st & (ICH_SMB_HS_INTR |
ICH_SMB_HS_DEVERR | ICH_SMB_HS_BUSERR | ICH_SMB_HS_FAILED |
- ICH_SMB_HS_SMBAL | ICH_SMB_HS_BDONE)) == 0)
+ ICH_SMB_HS_BDONE)) == 0)
/* Interrupt was not for us */
return (0);
DPRINTF(("%s: intr st 0x%b\n", sc->sc_dev.dv_xname, st,
ICH_SMB_HS_BITS));
-
- /* Clear status bits */
- bus_space_write_1(sc->sc_iot, sc->sc_ioh, ICH_SMB_HS, st);
/* Check for errors */
if (st & (ICH_SMB_HS_DEVERR | ICH_SMB_HS_BUSERR | ICH_SMB_HS_FAILED)) {
