Re: [PATCH v1 04/15] nitro_enclaves: Init PCI device driver

[Liran Alon <liran.alon@oracle.com>]

On 21/04/2020 21:41, Andra Paraschiv wrote:
> +
> +/**
> + * ne_setup_msix - Setup MSI-X vectors for the PCI device.
> + *
> + * @pdev: PCI device to setup the MSI-X for.
> + * @ne_pci_dev: PCI device private data structure.
> + *
> + * @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)
> +{
> +	int nr_vecs = 0;
> +	int rc = -EINVAL;
> +
> +	BUG_ON(!ne_pci_dev);
This kind of defensive programming does not align with Linux coding 
convention.
I think these BUG_ON() conditions should be removed.
> +
> +	nr_vecs = pci_msix_vec_count(pdev);
> +	if (nr_vecs < 0) {
> +		rc = nr_vecs;
> +
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in getting vec count [rc=%d]\n",
> +				    rc);
> +
> +		return rc;
> +	}
> +
> +	rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
> +	if (rc < 0) {
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in alloc MSI-X vecs [rc=%d]\n",
> +				    rc);
> +
> +		goto err_alloc_irq_vecs;
You should just replace this with "return rc;" as no cleanup is required 
here.
> +	}
> +
> +	return 0;
> +
> +err_alloc_irq_vecs:
> +	return rc;
> +}
> +
> +/**
> + * ne_pci_dev_enable - Select PCI device version and enable it.
> + *
> + * @pdev: PCI device to select version for and then enable.
> + * @ne_pci_dev: PCI device private data structure.
> + *
> + * @returns: 0 on success, negative return value on failure.
> + */
> +static int ne_pci_dev_enable(struct pci_dev *pdev,
> +			     struct ne_pci_dev *ne_pci_dev)
> +{
> +	u8 dev_enable_reply = 0;
> +	u16 dev_version_reply = 0;
> +
> +	BUG_ON(!pdev);
> +	BUG_ON(!ne_pci_dev);
> +	BUG_ON(!ne_pci_dev->iomem_base);
Same.
> +
> +	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,
> +				    "Failure in pci dev version cmd\n");
> +
> +		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,
> +				    "Failure in pci dev enable cmd\n");
> +
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ne_pci_dev_disable - Disable PCI device.
> + *
> + * @pdev: PCI device to disable.
> + * @ne_pci_dev: PCI device private data structure.
> + *
> + * @returns: 0 on success, negative return value on failure.
> + */
> +static int ne_pci_dev_disable(struct pci_dev *pdev,
> +			      struct ne_pci_dev *ne_pci_dev)
> +{
> +	u8 dev_disable_reply = 0;
> +
> +	BUG_ON(!pdev);
> +	BUG_ON(!ne_pci_dev);
> +	BUG_ON(!ne_pci_dev->iomem_base);
Same.
> +
> +	iowrite8(NE_ENABLE_OFF, ne_pci_dev->iomem_base + NE_ENABLE);
> +
> +	/*
> +	 * TODO: 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.
> +	 */
> +	dev_disable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
> +	if (dev_disable_reply != NE_ENABLE_OFF) {
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in pci dev disable cmd\n");
> +
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ne_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct ne_pci_dev *ne_pci_dev = NULL;
> +	int rc = -EINVAL;
Unnecessary variable initialization.
ne_pci_dev and rc are initialized below always before they are used.
> +
> +	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,
> +				    "Failure in pci dev enable [rc=%d]\n", rc);
> +
Why is this dev_err_ratelimited() instead of dev_err()?
Same for the rest of error printing in this probe() method and other 
places in this patch.
> +		goto err_pci_enable_dev;
I find it confusing that the error labels are named based on the 
failure-case they are used,
instead of the action they do (i.e. Unwind previous successful operation 
that requires unwinding).
This doesn't seem to match Linux kernel coding convention.
It also created an unnecessary 2 labels pointing to the same place in 
cleanup code.
> +	}
> +
> +	rc = pci_request_regions_exclusive(pdev, "ne_pci_dev");
> +	if (rc < 0) {
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in pci request regions [rc=%d]\n",
> +				    rc);
> +
> +		goto err_req_regions;
> +	}
> +
> +	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,
> +				    "Failure in pci bar mapping [rc=%d]\n", rc);
> +
> +		goto err_iomap;
> +	}
> +
> +	rc = ne_setup_msix(pdev, ne_pci_dev);
> +	if (rc < 0) {
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in pci dev msix setup [rc=%d]\n",
> +				    rc);
> +
> +		goto err_setup_msix;
> +	}
> +
> +	rc = ne_pci_dev_disable(pdev, ne_pci_dev);
> +	if (rc < 0) {
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in ne_pci_dev disable [rc=%d]\n",
> +				    rc);
> +
> +		goto err_ne_pci_dev_disable;
> +	}
It seems weird that we need to disable the device before enabling it on 
the probe() method.
Why can't we just enable the device without disabling it?
> +
> +	rc = ne_pci_dev_enable(pdev, ne_pci_dev);
> +	if (rc < 0) {
> +		dev_err_ratelimited(&pdev->dev,
> +				    "Failure in ne_pci_dev enable [rc=%d]\n",
> +				    rc);
> +
> +		goto err_ne_pci_dev_enable;
> +	}
> +
> +	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);
> +
> +	pci_set_drvdata(pdev, ne_pci_dev);
If you would have pci_set_drvdata() as one of the first operations in 
ne_probe(), then you could have avoided
passing both struct pci_dev  and struct ne_pci_dev parameters to 
ne_setup_msix(), ne_pci_dev_enable() and ne_pci_dev_disable().
Which would have been a bit more elegant.
> +
> +	return 0;
> +
> +err_ne_pci_dev_enable:
> +err_ne_pci_dev_disable:
> +	pci_free_irq_vectors(pdev);
> +err_setup_msix:
> +	pci_iounmap(pdev, ne_pci_dev->iomem_base);
> +err_iomap:
> +	pci_release_regions(pdev);
> +err_req_regions:
> +	pci_disable_device(pdev);
> +err_pci_enable_dev:
> +	kzfree(ne_pci_dev);
An empty new-line is appropriate here.
To separate the return statement from the cleanup logic.
> +	return rc;
> +}
> +
> +static void ne_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;
Why is this condition necessary?
The ne_remove() function should be called only in case ne_probe() succeeded.
In that case, both ne_pci_dev and ne_pci_dev->iomem_base should be non-NULL.
> +
> +	ne_pci_dev_disable(pdev, ne_pci_dev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +
> +	pci_free_irq_vectors(pdev);
> +
> +	pci_iounmap(pdev, ne_pci_dev->iomem_base);
> +
> +	kzfree(ne_pci_dev);
> +
> +	pci_release_regions(pdev);
> +
> +	pci_disable_device(pdev);
You should aspire to keep ne_remove() order of operations to be the 
reverse order of operations done in ne_probe().
Which would also nicely match the order of operations done in ne_probe() 
cleanup.
i.e. The following order:

pci_set_drvdata();
ne_pci_dev_disable();
pci_free_irq_vectors();
pci_iounmap();
pci_release_regions();
pci_disable_device()
kzfree();

-Liran