Hi,
it's good to see some NTB code getting into mainline! I have a few comments
though.
On Tue, 02 Oct 2012 21:26:16 -0000, Jon Mason <jon.mason@xxxxxxxxx>
wrote:
[...]
>+/**
>+ * ntb_write_local_spad() - write to the secondary scratchpad register
>+ * @ndev: pointer to ntb_device instance
>+ * @idx: index to the scratchpad register, 0 based
>+ * @val: the data value to put into the register
>+ *
>+ * This function allows writing of a 32bit value to the indexed scratchpad
>+ * register. The register resides on the secondary (external) side.
>+ *
>+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>+ */
[...]
>+/**
>+ * ntb_write_remote_spad() - write to the secondary scratchpad register
>+ * @ndev: pointer to ntb_device instance
>+ * @idx: index to the scratchpad register, 0 based
>+ * @val: the data value to put into the register
>+ *
>+ * This function allows writing of a 32bit value to the indexed scratchpad
>+ * register. The register resides on the secondary (external) side.
>+ *
>+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>+ */
Those comments look suspiciously similar. I think one of the functions
does write to primary scratchpad?
[...]
>+/**
>+ * ntb_read_local_spad() - read from the primary scratchpad register
>+ * @ndev: pointer to ntb_device instance
>+ * @idx: index to scratchpad register, 0 based
>+ * @val: pointer to 32bit integer for storing the register value
>+ *
>+ * This function allows reading of the 32bit scratchpad register on
>+ * the primary (internal) side.
>+ *
>+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>+ */
[...]
>+/**
>+ * ntb_read_remote_spad() - read from the primary scratchpad register
>+ * @ndev: pointer to ntb_device instance
>+ * @idx: index to scratchpad register, 0 based
>+ * @val: pointer to 32bit integer for storing the register value
>+ *
>+ * This function allows reading of the 32bit scratchpad register on
>+ * the primary (internal) side.
>+ *
>+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>+ */
Same here.
[...]
>+static int ntb_setup_msix(struct ntb_device *ndev)
>+{
>+ struct pci_dev *pdev = ndev->pdev;
>+ struct msix_entry *msix;
>+ int msix_entries;
>+ int rc, i, pos;
>+ u16 val;
>+
>+ pos = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
>+ if (!pos) {
>+ rc = -EIO;
>+ goto err;
>+ }
>+
>+ rc = pci_read_config_word(pdev, pos + PCI_MSIX_FLAGS, &val);
>+ if (rc)
>+ goto err;
>+
>+ msix_entries = msix_table_size(val);
>+ if (msix_entries > ndev->limits.msix_cnt) {
>+ rc = -EINVAL;
>+ goto err;
>+ }
>+
>+ ndev->msix_entries = kmalloc(sizeof(struct msix_entry) * msix_entries,
>+ GFP_KERNEL);
>+ if (!ndev->msix_entries) {
>+ rc = -ENOMEM;
>+ goto err;
>+ }
>+
>+ for (i = 0; i < msix_entries; i++)
>+ ndev->msix_entries[i].entry = i;
>+
>+ rc = pci_enable_msix(pdev, ndev->msix_entries, msix_entries);
>+ if (rc < 0)
>+ goto err1;
>+ if (rc > 0) {
rc > 0 doesn't mean that vectors were allocated. Have a look at the
example in Documentation/PCI/MSI-HOWTO.txt.
>+ /* On SNB, the link interrupt is always tied to 4th vector. If
>+ * we can't get all 4, then we can't use MSI-X.
>+ */
>+ if (ndev->hw_type != BWD_HW) {
>+ rc = -EIO;
>+ goto err1;
>+ }
This looks fragile, what if msix_table_size(val) was < 4?
>+
>+ dev_warn(&pdev->dev,
>+ "Only %d MSI-X vectors. Limiting the number of queues to that number.\n",
>+ rc);
>+ msix_entries = rc;
>+ }
>+
>+ for (i = 0; i < msix_entries; i++) {
>+ msix = &ndev->msix_entries[i];
>+ WARN_ON(!msix->vector);
>+
>+ /* Use the last MSI-X vector for Link status */
>+ if (ndev->hw_type == BWD_HW) {
>+ rc = request_irq(msix->vector, bwd_callback_msix_irq, 0,
>+ "ntb-callback-msix", &ndev->db_cb[i]);
>+ if (rc)
>+ goto err2;
>+ } else {
>+ if (i == msix_entries - 1) {
>+ rc = request_irq(msix->vector,
>+ xeon_event_msix_irq, 0,
>+ "ntb-event-msix", ndev);
>+ if (rc)
>+ goto err2;
>+ } else {
>+ rc = request_irq(msix->vector,
>+ xeon_callback_msix_irq, 0,
>+ "ntb-callback-msix",
>+ &ndev->db_cb[i]);
>+ if (rc)
>+ goto err2;
>+ }
>+ }
>+ }
>+
>+ ndev->num_msix = msix_entries;
>+ if (ndev->hw_type == BWD_HW)
>+ ndev->max_cbs = msix_entries;
>+ else
>+ ndev->max_cbs = msix_entries - 1;
>+
>+ return 0;
>+
>+err2:
>+ while (--i >= 0) {
>+ msix = &ndev->msix_entries[i];
>+ if (ndev->hw_type != BWD_HW && i == ndev->num_msix - 1)
>+ free_irq(msix->vector, ndev);
>+ else
>+ free_irq(msix->vector, &ndev->db_cb[i]);
>+ }
>+ pci_disable_msix(pdev);
>+err1:
>+ kfree(ndev->msix_entries);
>+ dev_err(&pdev->dev, "Error allocating MSI-X interrupt\n");
>+err:
>+ ndev->num_msix = 0;
>+ return rc;
>+}
Thanks for your work,
-- Kuba
--
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
