All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Keith Busch <keith.busch@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	x86@kernel.org, linux-pci@vger.kernel.org,
	Jiang Liu <jiang.liu@linux.intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Bryan Veal <bryan.e.veal@intel.com>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Martin Mares <mj@ucw.cz>,
	Jon Derrick <jonathan.derrick@intel.com>
Subject: Re: [PATCHv4 5/6] x86/pci: Initial commit for new VMD device driver
Date: Sat, 7 Nov 2015 12:55:54 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.11.1511071243530.4032@nanos> (raw)
In-Reply-To: <1446849346-8242-6-git-send-email-keith.busch@intel.com>

Keith,

On Fri, 6 Nov 2015, Keith Busch wrote:
> +
> +static DEFINE_SPINLOCK(list_lock);

Can you please make that DEFINE_RAW_SPINLOCK as it nests into the irq
descriptor lock.

> +
> +struct vmd_irq {
> +	struct list_head	node;
> +	struct rcu_head		rcu;	/* rcu callback list */
> +	struct vmd_irq_list	*irq;	/* back pointer */

Please use KernelDoc style documentation if you want to document your
data structures.

> +/**
> + * XXX: Stubbed until a good way to not create conflicts with other devices
> + * sharing the same vector is developed.
> + */
> +static int vmd_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> +								bool force)
> +{
> +	return -EINVAL;
> +}

Well, this is a hard problem for shared vectors. The question is
whether we really want to expose that at the per device level or
rather at the demultiplexing level.

> +static struct vmd_irq_list *vmd_next_irq(struct vmd_dev *vmd)
> +{
> +	int i, best = 0;
> +
> +	spin_lock(&list_lock);
> +	for (i = 1; i < vmd->msix_count; i++)
> +		if (vmd->irqs[i].count < vmd->irqs[best].count)
> +			best = i;
> +	vmd->irqs[best].count++;
> +	spin_unlock(&list_lock);
> +
> +	return &vmd->irqs[best];

That looks smarter. Though we might need some more complex way to do
that when interrupt affinity comes into play.

> +static irqreturn_t vmd_irq(int irq, void *data)
> +{
> +	struct vmd_irq_list *irqs = data;
> +	struct vmd_irq *vmdirq;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(vmdirq, &irqs->irq_list, node)
> +		generic_handle_irq(vmdirq->virq);
> +	rcu_read_unlock();
> +
> +	return IRQ_HANDLED;

Please remind me, that I still need to fix that life time problem in
the core.

> +	for (i = 0; i < vmd->msix_count; i++) {
> +		INIT_LIST_HEAD(&vmd->irqs[i].irq_list);
> +		vmd->irqs[i].vmd_vector = vmd->msix_entries[i].vector;
> +		vmd->irqs[i].index = i;
> +
> +		err = devm_request_irq(&dev->dev, vmd->irqs[i].vmd_vector,
> +				vmd_irq, IRQF_SHARED, "vmd", &vmd->irqs[i]);

This is a MSI interrupt which is never shared.

Aside of the few nitpicks above I'm really happy with the interrupt
side of that driver. Thanks for following up on all the comments!

Thanks,

	tglx



  reply	other threads:[~2015-11-07 11:56 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-06 22:35 [PATCHv4 0/6] Driver for new VMD device Keith Busch
2015-11-06 22:35 ` [PATCHv4 1/6] msi: Relax msi_domain_alloc() to support parentless MSI irqdomains Keith Busch
2015-11-07 11:43   ` Thomas Gleixner
2015-11-06 22:35 ` [PATCHv4 2/6] pci: skip child bus with conflicting resources Keith Busch
2015-11-06 22:35 ` [PATCHv4 3/6] Export msi and irq functions for module use Keith Busch
2015-11-06 22:35 ` [PATCHv4 4/6] x86-pci: allow pci domain specific dma ops Keith Busch
2015-11-06 22:35 ` [PATCHv4 5/6] x86/pci: Initial commit for new VMD device driver Keith Busch
2015-11-07 11:55   ` Thomas Gleixner [this message]
2015-11-06 22:35 ` [PATCHv4 6/6] pciutils: Allow 32-bit domains Keith Busch

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=alpine.DEB.2.11.1511071243530.4032@nanos \
    --to=tglx@linutronix.de \
    --cc=bhelgaas@google.com \
    --cc=bryan.e.veal@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=hpa@zytor.com \
    --cc=jiang.liu@linux.intel.com \
    --cc=jonathan.derrick@intel.com \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=mj@ucw.cz \
    --cc=x86@kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.