LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Greg KH <greg@kroah.com>
To: Alexander Graf <graf@amazon.de>
Cc: Andra Paraschiv <andraprs@amazon.com>,
	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: Sun, 24 May 2020 08:32:10 +0200
Message-ID: <20200524063210.GA1369260@kroah.com> (raw)
In-Reply-To: <68b86d32-1255-f9ce-4366-12219ce07ba6@amazon.de>

On Sat, May 23, 2020 at 10:25:25PM +0200, Alexander Graf wrote:
> 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 ...

Same for WARN_ON when you run with panic-on-warn enabled :(

> 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 ...

For static calls where you control the callers, don't do checks like
this.  Otherwise the kernel would just be full of these all over the
place and things would slow down.  It's just not needed.

> > > +     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.

Think through these as the driver seems to ONLY use these ratelimited
calls right now, which is not correct.

Also, if a user can create a printk, that almost always is not a good
idea.  But yes, those should be ratelimited.

thanks,

greg k-h

  reply index

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
2020-05-24  6:32       ` Greg KH [this message]
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=20200524063210.GA1369260@kroah.com \
    --to=greg@kroah.com \
    --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=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=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

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

	# 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