Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1264822

Re: [PATCHv4 5/6] x86/pci: Initial commit for new VMD device driver

Show all headers | View raw

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCHv4 0/6] Driver for new VMD device Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
  [PATCHv4 3/6] Export msi and irq functions for module use Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
  [PATCHv4 4/6] x86-pci: allow pci domain specific dma ops Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
  [PATCHv4 2/6] pci: skip child bus with conflicting resources Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
  [PATCHv4 6/6] pciutils: Allow 32-bit domains Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
  [PATCHv4 5/6] x86/pci: Initial commit for new VMD device driver Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
    Re: [PATCHv4 5/6] x86/pci: Initial commit for new VMD device  driver Thomas Gleixner <tglx@linutronix.de> - 2015-11-07 13:00 +0100

csiph-web