From: Alexander Graf <graf@amazon.de>
To: Greg KH <greg@kroah.com>, Andra Paraschiv <andraprs@amazon.com>
Cc: <linux-kernel@vger.kernel.org>,
Anthony Liguori <aliguori@amazon.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Colm MacCarthaigh <colmmacc@amazon.com>,
Bjoern Doebel <doebel@amazon.de>,
David Woodhouse <dwmw@amazon.co.uk>,
Frank van der Linden <fllinden@amazon.com>,
"Martin Pohlack" <mpohlack@amazon.de>,
Matt Wilson <msw@amazon.com>, Paolo Bonzini <pbonzini@redhat.com>,
Balbir Singh <sblbir@amazon.com>,
Stefano Garzarella <sgarzare@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
Stewart Smith <trawets@amazon.com>,
Uwe Dannowski <uwed@amazon.de>, <kvm@vger.kernel.org>,
<ne-devel-upstream@amazon.com>
Subject: Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver
Date: Sat, 23 May 2020 22:25:25 +0200 [thread overview]
Message-ID: <68b86d32-1255-f9ce-4366-12219ce07ba6@amazon.de> (raw)
In-Reply-To: <20200522070414.GB771317@kroah.com>
Hey Greg,
On 22.05.20 09: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.
I think the point here is to catch situations that should never happen,
but keep a sanity check in in case they do happen. This would've usually
been a BUG_ON, but people tend to dislike those these days because they
can bring down your system ...
So in this particular case here I agree that it's a bit silly to check
whether pdev is != NULL. In other device code internal APIs though it's
not quite as clear of a cut. I by far prefer code that tells me it's
broken over reverse engineering stray pointer accesses ...
>
>> +
>> + 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.
>
>> +
>> + 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?
In this particular function, no, so here it really should just be
dev_err. Other functions are implicitly callable from user space through
an ioctl, which means they really need to stay rate limited.
Thanks a lot for looking through the code and pointing all those bits out :)
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
next prev parent reply other threads:[~2020-05-23 20:25 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 6:29 [PATCH v2 00/18] Add support for Nitro Enclaves Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 01/18] nitro_enclaves: Add ioctl interface definition Andra Paraschiv
2020-05-22 7:00 ` Greg KH
2020-05-22 8:16 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 02/18] nitro_enclaves: Define the PCI device interface Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 03/18] nitro_enclaves: Define enclave info for internal bookkeeping Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 04/18] nitro_enclaves: Init PCI device driver Andra Paraschiv
2020-05-22 7:04 ` Greg KH
2020-05-23 20:25 ` Alexander Graf [this message]
2020-05-24 6:32 ` Greg KH
2020-05-25 11:15 ` Paraschiv, Andra-Irina
2020-05-25 10:54 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 05/18] nitro_enclaves: Handle PCI device command requests Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 06/18] nitro_enclaves: Handle out-of-band PCI device events Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 07/18] nitro_enclaves: Init misc device providing the ioctl interface Andra Paraschiv
2020-05-22 7:07 ` Greg KH
2020-05-25 20:49 ` Paraschiv, Andra-Irina
2020-05-26 6:42 ` Greg KH
2020-05-26 8:17 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 08/18] nitro_enclaves: Add logic for enclave vm creation Andra Paraschiv
2020-05-22 7:08 ` Greg KH
2020-05-25 20:53 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 09/18] nitro_enclaves: Add logic for enclave vcpu creation Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 10/18] nitro_enclaves: Add logic for enclave image load metadata Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 11/18] nitro_enclaves: Add logic for enclave memory region set Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 12/18] nitro_enclaves: Add logic for enclave start Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 13/18] nitro_enclaves: Add logic for enclave termination Andra Paraschiv
2020-05-22 6:29 ` [PATCH v2 14/18] nitro_enclaves: Add Kconfig for the Nitro Enclaves driver Andra Paraschiv
2020-05-22 7:09 ` Greg KH
2020-05-25 21:00 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 15/18] nitro_enclaves: Add Makefile " Andra Paraschiv
2020-05-22 7:09 ` Greg KH
2020-05-25 21:02 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 16/18] nitro_enclaves: Add sample for ioctl interface usage Andra Paraschiv
2020-05-22 7:08 ` Greg KH
2020-05-25 20:57 ` Paraschiv, Andra-Irina
2020-05-26 6:41 ` Greg KH
2020-05-26 8:06 ` Paraschiv, Andra-Irina
2020-05-22 7:11 ` Greg KH
2020-05-25 21:10 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 17/18] nitro_enclaves: Add overview documentation Andra Paraschiv
2020-05-22 7:09 ` Greg KH
2020-05-25 21:04 ` Paraschiv, Andra-Irina
2020-05-22 6:29 ` [PATCH v2 18/18] MAINTAINERS: Add entry for the Nitro Enclaves driver Andra Paraschiv
2020-05-22 7:03 ` Joe Perches
2020-05-22 8:20 ` Paraschiv, Andra-Irina
2020-05-22 6:39 ` [PATCH v2 00/18] Add support for Nitro Enclaves Paraschiv, Andra-Irina
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=68b86d32-1255-f9ce-4366-12219ce07ba6@amazon.de \
--to=graf@amazon.de \
--cc=aliguori@amazon.com \
--cc=andraprs@amazon.com \
--cc=benh@kernel.crashing.org \
--cc=colmmacc@amazon.com \
--cc=doebel@amazon.de \
--cc=dwmw@amazon.co.uk \
--cc=fllinden@amazon.com \
--cc=greg@kroah.com \
--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=sgarzare@redhat.com \
--cc=stefanha@redhat.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).