linux-kernel - Re: [PATCH v3 2/2] irqchip: dw-apb-ictl: add irq_set

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150706221709.3c120bb9@xhacker>
Date:	Mon, 6 Jul 2015 22:17:09 +0800
From:	Jisheng Zhang <jszhang@...vell.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	<jason@...edaemon.net>, <linux@....linux.org.uk>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 2/2] irqchip: dw-apb-ictl: add irq_set_affinity
 support

Dear Thomas,

On Mon, 6 Jul 2015 15:51:28 +0200
Thomas Gleixner <tglx@...utronix.de> wrote:

> On Mon, 6 Jul 2015, Jisheng Zhang wrote:
> > On Mon, 6 Jul 2015 12:30:01 +0200
> > Thomas Gleixner <tglx@...utronix.de> wrote:
> > 
> > > On Mon, 6 Jul 2015, Jisheng Zhang wrote:
> > > > +static int dw_apb_ictl_set_affinity(struct irq_data *d,
> > > > +				    const struct cpumask *mask_val,
> > > > +				    bool force)
> > > > +{
> > > > +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> > > > +	struct dw_apb_ictl_priv *priv = gc->private;
> > > > +	struct irq_chip *chip = irq_get_chip(priv->parent_irq);
> > > > +	struct irq_data *data = irq_get_irq_data(priv->parent_irq);
> > > > +
> > > > +	if (chip && chip->irq_set_affinity)
> > > > +		return chip->irq_set_affinity(data, mask_val, force);
> > > 
> > > This is wrong as it lacks proper locking of the parent irq. That needs
> > > to be solved at the core code level in a clean way.
> > 
> > Is it acceptable to call irq_set_affinity() or irq_force_affinity() as the
> > following:
> > 
> > if (force)
> > 	return irq_force_affinity(priv->parent_irq, mask_val);
> > else
> > 	return irq_set_affinity(priv->parent_irq, mask_val);
> 
> Not from the driver, as you run into lock nesting hell. As I said,
> this needs to be solved at the core code level and needs a proper
> thought out design.

Got it. Thanks for the clarification.

> 
> Just for the record: I'm not too happy about that 'fiddle with the
> parent' mechanism because it opens just a large can of worms. I wish
> hardware designers would talk to OS people before they implement random
> nonsense.
> 

Fully agree with you. I'm requesting our HW people to connect timer to GIC
directly in future chips. But in existing chips, it seems we have to wait
for core code ready or use this lack of proper locking set_affinity patch
ourself.

Thanks,
Jisheng
--
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/