On Sat, Feb 14, 2009 at 12:56:44AM +0800, Andi Kleen wrote: > Yu Zhao <yu.zhao@xxxxxxxxx> writes: > > + > > + > > +static int sriov_init(struct pci_dev *dev, int pos) > > +{ > > + int i; > > + int rc; > > + int nres; > > + u32 pgsz; > > + u16 ctrl, total, offset, stride; > > + struct pci_sriov *iov; > > + struct resource *res; > > + struct pci_dev *pdev; > > + > > + if (dev->pcie_type != PCI_EXP_TYPE_RC_END && > > + dev->pcie_type != PCI_EXP_TYPE_ENDPOINT) > > + return -ENODEV; > > + > > It would be a good idea to put a might_sleep() here just in > case the msleep happens below and drivers call it incorrectly. Yes, will do. > > + pci_read_config_word(dev, pos + PCI_SRIOV_CTRL, &ctrl); > > + if (ctrl & PCI_SRIOV_CTRL_VFE) { > > + pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, 0); > > + msleep(100); > > That's really long. Hopefully that's really needed. It's needed according to SR-IOV spec, however, these lines clear the VF Enable bit if the BIOS or something else has set it. So it doesn't always run into this. > > + > > + pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl); > > + pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total); > > + pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset); > > + pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride); > > + if (!offset || (total > 1 && !stride)) > > + return -EIO; > > + > > + pci_read_config_dword(dev, pos + PCI_SRIOV_SUP_PGSIZE, &pgsz); > > + i = PAGE_SHIFT > 12 ? PAGE_SHIFT - 12 : 0; > > + pgsz &= ~((1 << i) - 1); > > + if (!pgsz) > > + return -EIO; > > All the error paths don't seem to undo the config space writes. > How will the devices behave with half initialized context? Since the VF Enable bit is cleared before the initialization, setting others SR-IOV registers won't change state of the device. So it should be OK even without undo these writes as long as the VF Enable bit is not set. Thanks, Yu -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html