Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver
From: Paraschiv, Andra-Irina
Date: Mon May 25 2020 - 06:54:40 EST
On 22/05/2020 10:04, Greg KH wrote:
On Fri, May 22, 2020 at 09:29:32AM +0300, Andra Paraschiv wrote:
+/**
+ * ne_setup_msix - Setup MSI-X vectors for the PCI device.
+ *
+ * @pdev: PCI device to setup the MSI-X for.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_setup_msix(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = NULL;
+ int nr_vecs = 0;
+ int rc = -EINVAL;
+
+ if (WARN_ON(!pdev))
+ return -EINVAL;
How can this ever happen? If it can not, don't test for it. If it can,
don't warn for it as that will crash systems that do panic-on-warn, just
test and return an error.
It shouldn't happen, only used this and subsequent occurrences in the
other functions for sanity checks.
I removed the WARN_ONs from the patch series and updated the static
calls checks.
+
+ ne_pci_dev = pci_get_drvdata(pdev);
+ if (WARN_ON(!ne_pci_dev))
+ return -EINVAL;
Same here, don't use WARN_ON if at all possible.
Done.
+
+ nr_vecs = pci_msix_vec_count(pdev);
+ if (nr_vecs < 0) {
+ rc = nr_vecs;
+
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in getting vec count [rc=%d]\n",
+ rc);
+
Why ratelimited, can this happen over and over and over?
That's correct, not in this case. I updated the dev_error / pr_error
calls to include ratelimited only in the call paths of the ioctl commands.
+ return rc;
+ }
+
+ rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
+ if (rc < 0) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in alloc MSI-X vecs [rc=%d]\n",
+ rc);
Same here.
Updated to dev_err().
+
+ return rc;
+ }
+
+ return 0;
+}
+
+/**
+ * ne_teardown_msix - Teardown MSI-X vectors for the PCI device.
+ *
+ * @pdev: PCI device to teardown the MSI-X for.
+ */
+static void ne_teardown_msix(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = NULL;
=NULL not needed.
Updated to init via pci_get_drvdata() directly.
+
+ if (WARN_ON(!pdev))
+ return;
Again, you control the callers, how can this ever be true?
Sanity check, should never happen.
+
+ ne_pci_dev = pci_get_drvdata(pdev);
+ if (WARN_ON(!ne_pci_dev))
+ return;
Again, same thing. I'm just going to let you fix up all instances of
this pattern from now on and not call it out again.
Yep, I updated all the occurrences.
+
+ pci_free_irq_vectors(pdev);
+}
+
+/**
+ * ne_pci_dev_enable - Select PCI device version and enable it.
+ *
+ * @pdev: PCI device to select version for and then enable.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_pci_dev_enable(struct pci_dev *pdev)
+{
+ u8 dev_enable_reply = 0;
+ u16 dev_version_reply = 0;
+ struct ne_pci_dev *ne_pci_dev = NULL;
+
+ if (WARN_ON(!pdev))
+ return -EINVAL;
+
+ ne_pci_dev = pci_get_drvdata(pdev);
+ if (WARN_ON(!ne_pci_dev) || WARN_ON(!ne_pci_dev->iomem_base))
+ return -EINVAL;
+
+ iowrite16(NE_VERSION_MAX, ne_pci_dev->iomem_base + NE_VERSION);
+
+ dev_version_reply = ioread16(ne_pci_dev->iomem_base + NE_VERSION);
+ if (dev_version_reply != NE_VERSION_MAX) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci dev version cmd\n");
Same here, why all the ratelimited stuff? Should just be dev_err(),
right?
True, I modified to dev_err().
+
+ return -EIO;
+ }
+
+ iowrite8(NE_ENABLE_ON, ne_pci_dev->iomem_base + NE_ENABLE);
+
+ dev_enable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
+ if (dev_enable_reply != NE_ENABLE_ON) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci dev enable cmd\n");
+
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * ne_pci_dev_disable - Disable PCI device.
+ *
+ * @pdev: PCI device to disable.
+ */
+static void ne_pci_dev_disable(struct pci_dev *pdev)
+{
+ u8 dev_disable_reply = 0;
+ struct ne_pci_dev *ne_pci_dev = NULL;
+ const unsigned int sleep_time = 10; // 10 ms
+ unsigned int sleep_time_count = 0;
+
+ if (WARN_ON(!pdev))
+ return;
+
+ ne_pci_dev = pci_get_drvdata(pdev);
+ if (WARN_ON(!ne_pci_dev) || WARN_ON(!ne_pci_dev->iomem_base))
+ return;
+
+ iowrite8(NE_ENABLE_OFF, ne_pci_dev->iomem_base + NE_ENABLE);
+
+ /*
+ * Check for NE_ENABLE_OFF in a loop, to handle cases when the device
+ * state is not immediately set to disabled and going through a
+ * transitory state of disabling.
+ */
+ while (sleep_time_count < DEFAULT_TIMEOUT_MSECS) {
+ dev_disable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
+ if (dev_disable_reply == NE_ENABLE_OFF)
+ return;
+
+ msleep_interruptible(sleep_time);
+ sleep_time_count += sleep_time;
+ }
+
+ dev_disable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
+ if (dev_disable_reply != NE_ENABLE_OFF)
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci dev disable cmd\n");
+}
+
+static int ne_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ struct ne_pci_dev *ne_pci_dev = NULL;
+ int rc = -EINVAL;
+
+ ne_pci_dev = kzalloc(sizeof(*ne_pci_dev), GFP_KERNEL);
+ if (!ne_pci_dev)
+ return -ENOMEM;
+
+ rc = pci_enable_device(pdev);
+ if (rc < 0) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci dev enable [rc=%d]\n", rc);
+
+ goto free_ne_pci_dev;
+ }
+
+ rc = pci_request_regions_exclusive(pdev, "ne_pci_dev");
+ if (rc < 0) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci request regions [rc=%d]\n",
+ rc);
+
+ goto disable_pci_dev;
+ }
+
+ ne_pci_dev->iomem_base = pci_iomap(pdev, PCI_BAR_NE, 0);
+ if (!ne_pci_dev->iomem_base) {
+ rc = -ENOMEM;
+
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci iomap [rc=%d]\n", rc);
+
+ goto release_pci_regions;
+ }
+
+ pci_set_drvdata(pdev, ne_pci_dev);
+
+ rc = ne_setup_msix(pdev);
+ if (rc < 0) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in pci dev msix setup [rc=%d]\n",
+ rc);
+
+ goto iounmap_pci_bar;
+ }
+
+ ne_pci_dev_disable(pdev);
+
+ rc = ne_pci_dev_enable(pdev);
+ if (rc < 0) {
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in ne_pci_dev enable [rc=%d]\n",
+ rc);
+
+ goto teardown_msix;
+ }
+
+ atomic_set(&ne_pci_dev->cmd_reply_avail, 0);
+ init_waitqueue_head(&ne_pci_dev->cmd_reply_wait_q);
+ INIT_LIST_HEAD(&ne_pci_dev->enclaves_list);
+ mutex_init(&ne_pci_dev->enclaves_list_mutex);
+ mutex_init(&ne_pci_dev->pci_dev_mutex);
+
+ return 0;
+
+teardown_msix:
+ ne_teardown_msix(pdev);
+iounmap_pci_bar:
+ pci_set_drvdata(pdev, NULL);
+ pci_iounmap(pdev, ne_pci_dev->iomem_base);
+release_pci_regions:
+ pci_release_regions(pdev);
+disable_pci_dev:
+ pci_disable_device(pdev);
+free_ne_pci_dev:
+ kzfree(ne_pci_dev);
+
+ return rc;
+}
+
+static void ne_pci_remove(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
+
+ if (!ne_pci_dev || !ne_pci_dev->iomem_base)
+ return;
+
+ ne_pci_dev_disable(pdev);
+
+ ne_teardown_msix(pdev);
+
+ pci_set_drvdata(pdev, NULL);
+
+ pci_iounmap(pdev, ne_pci_dev->iomem_base);
+
+ pci_release_regions(pdev);
+
+ pci_disable_device(pdev);
+
+ kzfree(ne_pci_dev);
Why kzfree()? It's a pci device structure? What "special" info was in
it?
It's a data structure that includes metadata for enclaves bookkeeping
and NE PCI dev access e.g. iomem of the NE PCI dev, PCI dev cmd reply
waitqueue. I updated to kfree().
Thanks for the overall review.
Andra
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.