lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sat, 7 Nov 2015 12:55:54 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Keith Busch <keith.busch@...el.com>
cc:	LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
	linux-pci@...r.kernel.org, Jiang Liu <jiang.liu@...ux.intel.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Bryan Veal <bryan.e.veal@...el.com>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Martin Mares <mj@....cz>,
	Jon Derrick <jonathan.derrick@...el.com>
Subject: Re: [PATCHv4 5/6] x86/pci: Initial commit for new VMD device
 driver

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ