From 17f2e4c7f6eb41fc9994922518a6b7d6e46ef1e3 Mon Sep 17 00:00:00 2001 From: jkim Date: Fri, 16 Nov 2007 20:49:34 +0000 Subject: Make VPD register access more robust: - Implement timing out of VPD register access.[1] - Fix an off-by-one error of freeing malloc'd space when checksum is invalid. - Fix style(9) bugs, i.e., sizeof cannot be followed by space. - Retire now obsolete 'hw.pci.enable_vpd' tunable. Submitted by: cokane (initial revision)[1] Reviewed by: marius (intermediate revision) Silence from: jhb, jmg, rwatson Tested by: cokane, jkim MFC after: 3 days --- sys/dev/pci/pci.c | 190 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 121 insertions(+), 69 deletions(-) (limited to 'sys') diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c index e110b3a..d7343d2 100644 --- a/sys/dev/pci/pci.c +++ b/sys/dev/pci/pci.c @@ -94,10 +94,10 @@ static int pci_modevent(module_t mod, int what, void *arg); static void pci_hdrtypedata(device_t pcib, int b, int s, int f, pcicfgregs *cfg); static void pci_read_extcap(device_t pcib, pcicfgregs *cfg); -static uint32_t pci_read_vpd_reg(device_t pcib, pcicfgregs *cfg, - int reg); +static int pci_read_vpd_reg(device_t pcib, pcicfgregs *cfg, + int reg, uint32_t *data); #if 0 -static void pci_write_vpd_reg(device_t pcib, pcicfgregs *cfg, +static int pci_write_vpd_reg(device_t pcib, pcicfgregs *cfg, int reg, uint32_t data); #endif static void pci_read_vpd(device_t pcib, pcicfgregs *cfg); @@ -254,11 +254,6 @@ SYSCTL_INT(_hw_pci, OID_AUTO, do_power_resume, CTLFLAG_RW, &pci_do_power_resume, 1, "Transition from D3 -> D0 on resume."); -static int pci_do_vpd = 1; -TUNABLE_INT("hw.pci.enable_vpd", &pci_do_vpd); -SYSCTL_INT(_hw_pci, OID_AUTO, enable_vpd, CTLFLAG_RW, &pci_do_vpd, 1, - "Enable support for VPD."); - static int pci_do_msi = 1; TUNABLE_INT("hw.pci.enable_msi", &pci_do_msi); SYSCTL_INT(_hw_pci, OID_AUTO, enable_msi, CTLFLAG_RW, &pci_do_msi, 1, @@ -638,34 +633,50 @@ pci_read_extcap(device_t pcib, pcicfgregs *cfg) /* * PCI Vital Product Data */ -static uint32_t -pci_read_vpd_reg(device_t pcib, pcicfgregs *cfg, int reg) + +#define PCI_VPD_TIMEOUT 1000000 + +static int +pci_read_vpd_reg(device_t pcib, pcicfgregs *cfg, int reg, uint32_t *data) { + int count = PCI_VPD_TIMEOUT; KASSERT((reg & 3) == 0, ("VPD register must by 4 byte aligned")); WREG(cfg->vpd.vpd_reg + PCIR_VPD_ADDR, reg, 2); - while ((REG(cfg->vpd.vpd_reg + PCIR_VPD_ADDR, 2) & 0x8000) != 0x8000) + + while ((REG(cfg->vpd.vpd_reg + PCIR_VPD_ADDR, 2) & 0x8000) != 0x8000) { + if (--count < 0) + return (ENXIO); DELAY(1); /* limit looping */ + } + *data = (REG(cfg->vpd.vpd_reg + PCIR_VPD_DATA, 4)); - return (REG(cfg->vpd.vpd_reg + PCIR_VPD_DATA, 4)); + return (0); } #if 0 -static void +static int pci_write_vpd_reg(device_t pcib, pcicfgregs *cfg, int reg, uint32_t data) { + int count = PCI_VPD_TIMEOUT; + KASSERT((reg & 3) == 0, ("VPD register must by 4 byte aligned")); WREG(cfg->vpd.vpd_reg + PCIR_VPD_DATA, data, 4); WREG(cfg->vpd.vpd_reg + PCIR_VPD_ADDR, reg | 0x8000, 2); - while ((REG(cfg->vpd.vpd_reg + PCIR_VPD_ADDR, 2) & 0x8000) == 0x8000) + while ((REG(cfg->vpd.vpd_reg + PCIR_VPD_ADDR, 2) & 0x8000) == 0x8000) { + if (--count < 0) + return (ENXIO); DELAY(1); /* limit looping */ + } - return; + return (0); } #endif +#undef PCI_VPD_TIMEOUT + struct vpd_readstate { device_t pcib; pcicfgregs *cfg; @@ -675,14 +686,16 @@ struct vpd_readstate { uint8_t cksum; }; -static uint8_t -vpd_nextbyte(struct vpd_readstate *vrs) +static int +vpd_nextbyte(struct vpd_readstate *vrs, uint8_t *data) { + uint32_t reg; uint8_t byte; if (vrs->bytesinval == 0) { - vrs->val = le32toh(pci_read_vpd_reg(vrs->pcib, vrs->cfg, - vrs->off)); + if (pci_read_vpd_reg(vrs->pcib, vrs->cfg, vrs->off, ®)) + return (ENXIO); + vrs->val = le32toh(reg); vrs->off += 4; byte = vrs->val & 0xff; vrs->bytesinval = 3; @@ -693,7 +706,8 @@ vpd_nextbyte(struct vpd_readstate *vrs) } vrs->cksum += byte; - return (byte); + *data = byte; + return (0); } static void @@ -703,17 +717,12 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) int state; int name; int remain; - int end; int i; - uint8_t byte; int alloc, off; /* alloc/off for RO/W arrays */ int cksumvalid; int dflen; - - if (!pci_do_vpd) { - cfg->vpd.vpd_cached = 1; - return; - } + uint8_t byte; + uint8_t byte2; /* init vpd reader */ vrs.bytesinval = 0; @@ -726,10 +735,12 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) name = remain = i = 0; /* shut up stupid gcc */ alloc = off = 0; /* shut up stupid gcc */ dflen = 0; /* shut up stupid gcc */ - end = 0; cksumvalid = -1; - for (; !end;) { - byte = vpd_nextbyte(&vrs); + while (state >= 0) { + if (vpd_nextbyte(&vrs, &byte)) { + state = -2; + break; + } #if 0 printf("vpd: val: %#x, off: %d, bytesinval: %d, byte: %#hhx, " \ "state: %d, remain: %d, name: %#x, i: %d\n", vrs.val, @@ -738,12 +749,20 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) switch (state) { case 0: /* item name */ if (byte & 0x80) { - remain = vpd_nextbyte(&vrs); - remain |= vpd_nextbyte(&vrs) << 8; + if (vpd_nextbyte(&vrs, &byte2)) { + state = -2; + break; + } + remain = byte2; + if (vpd_nextbyte(&vrs, &byte2)) { + state = -2; + break; + } + remain |= byte2 << 8; if (remain > (0x7f*4 - vrs.off)) { - end = 1; + state = -1; printf( - "pci%d:%d:%d:%d: invalid vpd data, remain %#x\n", + "pci%d:%d:%d:%d: invalid VPD data, remain %#x\n", cfg->domain, cfg->bus, cfg->slot, cfg->func, remain); } @@ -760,28 +779,27 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) state = 1; break; case 0xf: /* End */ - end = 1; state = -1; break; case 0x10: /* VPD-R */ alloc = 8; off = 0; cfg->vpd.vpd_ros = malloc(alloc * - sizeof *cfg->vpd.vpd_ros, M_DEVBUF, - M_WAITOK); + sizeof(*cfg->vpd.vpd_ros), M_DEVBUF, + M_WAITOK | M_ZERO); state = 2; break; case 0x11: /* VPD-W */ alloc = 8; off = 0; cfg->vpd.vpd_w = malloc(alloc * - sizeof *cfg->vpd.vpd_w, M_DEVBUF, - M_WAITOK); + sizeof(*cfg->vpd.vpd_w), M_DEVBUF, + M_WAITOK | M_ZERO); state = 5; break; default: /* Invalid data, abort */ - end = 1; - continue; + state = -1; + break; } break; @@ -797,12 +815,20 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) case 2: /* VPD-R Keyword Header */ if (off == alloc) { cfg->vpd.vpd_ros = reallocf(cfg->vpd.vpd_ros, - (alloc *= 2) * sizeof *cfg->vpd.vpd_ros, - M_DEVBUF, M_WAITOK); + (alloc *= 2) * sizeof(*cfg->vpd.vpd_ros), + M_DEVBUF, M_WAITOK | M_ZERO); } cfg->vpd.vpd_ros[off].keyword[0] = byte; - cfg->vpd.vpd_ros[off].keyword[1] = vpd_nextbyte(&vrs); - dflen = vpd_nextbyte(&vrs); + if (vpd_nextbyte(&vrs, &byte2)) { + state = -2; + break; + } + cfg->vpd.vpd_ros[off].keyword[1] = byte2; + if (vpd_nextbyte(&vrs, &byte2)) { + state = -2; + break; + } + dflen = byte2; if (dflen == 0 && strncmp(cfg->vpd.vpd_ros[off].keyword, "RV", 2) == 0) { @@ -815,17 +841,17 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) cfg->domain, cfg->bus, cfg->slot, cfg->func, dflen); cksumvalid = 0; - end = 1; + state = -1; break; } else if (dflen == 0) { cfg->vpd.vpd_ros[off].value = malloc(1 * - sizeof *cfg->vpd.vpd_ros[off].value, + sizeof(*cfg->vpd.vpd_ros[off].value), M_DEVBUF, M_WAITOK); cfg->vpd.vpd_ros[off].value[0] = '\x00'; } else cfg->vpd.vpd_ros[off].value = malloc( (dflen + 1) * - sizeof *cfg->vpd.vpd_ros[off].value, + sizeof(*cfg->vpd.vpd_ros[off].value), M_DEVBUF, M_WAITOK); remain -= 3; i = 0; @@ -845,12 +871,14 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) if (vrs.cksum == 0) cksumvalid = 1; else { - printf( + if (bootverbose) + printf( "pci%d:%d:%d:%d: bad VPD cksum, remain %hhu\n", - cfg->domain, cfg->bus, cfg->slot, - cfg->func, vrs.cksum); + cfg->domain, cfg->bus, + cfg->slot, cfg->func, + vrs.cksum); cksumvalid = 0; - end = 1; + state = -1; break; } } @@ -862,8 +890,8 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) if (dflen == 0 && remain == 0) { cfg->vpd.vpd_rocnt = off; cfg->vpd.vpd_ros = reallocf(cfg->vpd.vpd_ros, - off * sizeof *cfg->vpd.vpd_ros, - M_DEVBUF, M_WAITOK); + off * sizeof(*cfg->vpd.vpd_ros), + M_DEVBUF, M_WAITOK | M_ZERO); state = 0; } else if (dflen == 0) state = 2; @@ -878,15 +906,23 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) case 5: /* VPD-W Keyword Header */ if (off == alloc) { cfg->vpd.vpd_w = reallocf(cfg->vpd.vpd_w, - (alloc *= 2) * sizeof *cfg->vpd.vpd_w, - M_DEVBUF, M_WAITOK); + (alloc *= 2) * sizeof(*cfg->vpd.vpd_w), + M_DEVBUF, M_WAITOK | M_ZERO); } cfg->vpd.vpd_w[off].keyword[0] = byte; - cfg->vpd.vpd_w[off].keyword[1] = vpd_nextbyte(&vrs); - cfg->vpd.vpd_w[off].len = dflen = vpd_nextbyte(&vrs); + if (vpd_nextbyte(&vrs, &byte2)) { + state = -2; + break; + } + cfg->vpd.vpd_w[off].keyword[1] = byte2; + if (vpd_nextbyte(&vrs, &byte2)) { + state = -2; + break; + } + cfg->vpd.vpd_w[off].len = dflen = byte2; cfg->vpd.vpd_w[off].start = vrs.off - vrs.bytesinval; cfg->vpd.vpd_w[off].value = malloc((dflen + 1) * - sizeof *cfg->vpd.vpd_w[off].value, + sizeof(*cfg->vpd.vpd_w[off].value), M_DEVBUF, M_WAITOK); remain -= 3; i = 0; @@ -909,8 +945,8 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) if (dflen == 0 && remain == 0) { cfg->vpd.vpd_wcnt = off; cfg->vpd.vpd_w = reallocf(cfg->vpd.vpd_w, - off * sizeof *cfg->vpd.vpd_w, - M_DEVBUF, M_WAITOK); + off * sizeof(*cfg->vpd.vpd_w), + M_DEVBUF, M_WAITOK | M_ZERO); state = 0; } else if (dflen == 0) state = 5; @@ -920,18 +956,34 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg) printf("pci%d:%d:%d:%d: invalid state: %d\n", cfg->domain, cfg->bus, cfg->slot, cfg->func, state); - end = 1; + state = -1; break; } } - if (cksumvalid == 0) { + if (cksumvalid == 0 || state < -1) { /* read-only data bad, clean up */ - for (; off; off--) - free(cfg->vpd.vpd_ros[off].value, M_DEVBUF); - - free(cfg->vpd.vpd_ros, M_DEVBUF); - cfg->vpd.vpd_ros = NULL; + if (cfg->vpd.vpd_ros != NULL) { + for (off = 0; cfg->vpd.vpd_ros[off].value; off++) + free(cfg->vpd.vpd_ros[off].value, M_DEVBUF); + free(cfg->vpd.vpd_ros, M_DEVBUF); + cfg->vpd.vpd_ros = NULL; + } + } + if (state < -1) { + /* I/O error, clean up */ + printf("pci%d:%d:%d:%d: failed to read VPD data.\n", + cfg->domain, cfg->bus, cfg->slot, cfg->func); + if (cfg->vpd.vpd_ident != NULL) { + free(cfg->vpd.vpd_ident, M_DEVBUF); + cfg->vpd.vpd_ident = NULL; + } + if (cfg->vpd.vpd_w != NULL) { + for (off = 0; cfg->vpd.vpd_w[off].value; off++) + free(cfg->vpd.vpd_w[off].value, M_DEVBUF); + free(cfg->vpd.vpd_w, M_DEVBUF); + cfg->vpd.vpd_w = NULL; + } } cfg->vpd.vpd_cached = 1; #undef REG @@ -968,7 +1020,7 @@ pci_get_vpd_readonly_method(device_t dev, device_t child, const char *kw, for (i = 0; i < cfg->vpd.vpd_rocnt; i++) if (memcmp(kw, cfg->vpd.vpd_ros[i].keyword, - sizeof cfg->vpd.vpd_ros[i].keyword) == 0) { + sizeof(cfg->vpd.vpd_ros[i].keyword)) == 0) { *vptr = cfg->vpd.vpd_ros[i].value; } -- cgit v1.1