Messages in this thread | | | Subject | Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver | From | "Paraschiv, Andra-Irina" <> | Date | Mon, 25 May 2020 13:54:10 +0300 |
| |
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.
| |