Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver

["Paraschiv, Andra-Irina" <andraprs@amazon.com>]

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.