LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Liran Alon <liran.alon@oracle.com>
To: Andra Paraschiv <andraprs@amazon.com>, linux-kernel@vger.kernel.org
Cc: Anthony Liguori <aliguori@amazon.com>,
	Benjamin Herrenschmidt <benh@amazon.com>,
	Colm MacCarthaigh <colmmacc@amazon.com>,
	Bjoern Doebel <doebel@amazon.de>,
	David Woodhouse <dwmw@amazon.co.uk>,
	Frank van der Linden <fllinden@amazon.com>,
	Alexander Graf <graf@amazon.de>,
	Martin Pohlack <mpohlack@amazon.de>, Matt Wilson <msw@amazon.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Balbir Singh <sblbir@amazon.com>,
	Stewart Smith <trawets@amazon.com>,
	Uwe Dannowski <uwed@amazon.de>,
	kvm@vger.kernel.org, ne-devel-upstream@amazon.com
Subject: Re: [PATCH v1 04/15] nitro_enclaves: Init PCI device driver
Date: Sat, 25 Apr 2020 17:25:48 +0300
Message-ID: <dbb58c31-f388-cd03-ff66-e77b027a7ba3@oracle.com> (raw)
In-Reply-To: <20200421184150.68011-5-andraprs@amazon.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


  reply index

Thread overview: 77+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-21 18:41 [PATCH v1 00/15] Add support for Nitro Enclaves Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 01/15] nitro_enclaves: Add ioctl interface definition Andra Paraschiv
2020-04-21 18:47   ` Randy Dunlap
2020-04-21 21:45     ` Paolo Bonzini
2020-04-22 15:49       ` Paraschiv, Andra-Irina
2020-04-21 18:41 ` [PATCH v1 02/15] nitro_enclaves: Define the PCI device interface Andra Paraschiv
2020-04-21 21:22   ` Paolo Bonzini
2020-04-23 13:37     ` Paraschiv, Andra-Irina
2020-04-24 15:10       ` Paraschiv, Andra-Irina
2020-04-21 18:41 ` [PATCH v1 03/15] nitro_enclaves: Define enclave info for internal bookkeeping Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 04/15] nitro_enclaves: Init PCI device driver Andra Paraschiv
2020-04-25 14:25   ` Liran Alon [this message]
2020-04-29 16:31     ` Paraschiv, Andra-Irina
2020-04-21 18:41 ` [PATCH v1 05/15] nitro_enclaves: Handle PCI device command requests Andra Paraschiv
2020-04-25 14:52   ` Liran Alon
2020-04-29 17:00     ` Paraschiv, Andra-Irina
2020-04-21 18:41 ` [PATCH v1 06/15] nitro_enclaves: Handle out-of-band PCI device events Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 07/15] nitro_enclaves: Init misc device providing the ioctl interface Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 08/15] nitro_enclaves: Add logic for enclave vm creation Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 09/15] nitro_enclaves: Add logic for enclave vcpu creation Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 10/15] nitro_enclaves: Add logic for enclave memory region set Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 11/15] nitro_enclaves: Add logic for enclave start Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 12/15] nitro_enclaves: Add logic for enclave termination Andra Paraschiv
2020-04-21 18:41 ` [PATCH v1 13/15] nitro_enclaves: Add Kconfig for the Nitro Enclaves driver Andra Paraschiv
2020-04-21 18:50   ` Randy Dunlap
2020-04-22 14:35     ` Paraschiv, Andra-Irina
2020-04-21 18:41 ` [PATCH v1 14/15] nitro_enclaves: Add Makefile " Andra Paraschiv
2020-04-23  8:12   ` kbuild test robot
2020-04-24 17:00     ` Paraschiv, Andra-Irina
2020-04-23  8:43   ` kbuild test robot
2020-04-24 15:27     ` Paraschiv, Andra-Irina
2020-04-21 18:41 ` [PATCH v1 15/15] MAINTAINERS: Add entry " Andra Paraschiv
2020-04-21 21:46 ` [PATCH v1 00/15] Add support for Nitro Enclaves Paolo Bonzini
2020-04-23 13:19   ` Paraschiv, Andra-Irina
2020-04-23 13:42     ` Paolo Bonzini
2020-04-23 17:42       ` Paraschiv, Andra-Irina
2020-04-23 17:51         ` Paolo Bonzini
2020-04-23 20:56           ` Alexander Graf
2020-04-23 21:18             ` Paolo Bonzini
2020-04-24 12:56               ` Alexander Graf
2020-04-24 16:27                 ` Paolo Bonzini
2020-04-24 19:11                   ` Alexander Graf
2020-04-25 16:05                     ` Paolo Bonzini
2020-04-27  9:15                       ` Paraschiv, Andra-Irina
2020-04-27  9:22                       ` Paraschiv, Andra-Irina
2020-04-27  9:46                         ` Paolo Bonzini
2020-04-27 10:00                           ` Paraschiv, Andra-Irina
2020-04-28 15:07                       ` Alexander Graf
2020-04-29 13:20                         ` Paolo Bonzini
2020-04-30 13:59                           ` Paraschiv, Andra-Irina
2020-04-30 10:34                         ` Paolo Bonzini
2020-04-30 11:21                           ` Alexander Graf
2020-04-30 11:38                             ` Paolo Bonzini
2020-04-30 11:47                               ` Alexander Graf
2020-04-30 11:58                                 ` Paolo Bonzini
2020-04-30 12:19                                   ` Alexander Graf
2020-05-07 17:44       ` Pavel Machek
2020-05-08  7:00         ` Paraschiv, Andra-Irina
2020-05-09 19:21           ` Pavel Machek
2020-05-10 11:02             ` Herrenschmidt, Benjamin
2020-05-11 10:49               ` Paraschiv, Andra-Irina
2020-05-11 13:49               ` Stefan Hajnoczi
2020-04-24  3:04     ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2020-04-24  8:19       ` Paraschiv, Andra-Irina
2020-04-24  9:54         ` Paraschiv, Andra-Irina
2020-04-26  1:55           ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2020-04-27 18:39             ` Paraschiv, Andra-Irina
2020-04-24  9:59     ` Tian, Kevin
2020-04-24 13:59       ` Paraschiv, Andra-Irina
2020-04-26  8:16         ` Tian, Kevin
2020-04-27 19:05           ` Paraschiv, Andra-Irina
     [not found]         ` <CAKXe6SLonLQLAOY9Q_2AzTeg4uJxiknsAWnJpTF0hMcXEG5Tew@mail.gmail.com>
2020-05-11 12:05           ` Paraschiv, Andra-Irina
2020-04-25 15:25     ` Liran Alon
2020-04-27  7:56       ` Paraschiv, Andra-Irina
2020-04-27 11:44         ` Liran Alon
2020-04-28 15:25           ` Alexander Graf
2020-04-28 16:01             ` Liran Alon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dbb58c31-f388-cd03-ff66-e77b027a7ba3@oracle.com \
    --to=liran.alon@oracle.com \
    --cc=aliguori@amazon.com \
    --cc=andraprs@amazon.com \
    --cc=benh@amazon.com \
    --cc=colmmacc@amazon.com \
    --cc=doebel@amazon.de \
    --cc=dwmw@amazon.co.uk \
    --cc=fllinden@amazon.com \
    --cc=graf@amazon.de \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpohlack@amazon.de \
    --cc=msw@amazon.com \
    --cc=ne-devel-upstream@amazon.com \
    --cc=pbonzini@redhat.com \
    --cc=sblbir@amazon.com \
    --cc=trawets@amazon.com \
    --cc=uwed@amazon.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git