linux-kernel - Re: [PATCH] mfd: add MAX8907 core driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120726215150.GI4560@opensource.wolfsonmicro.com>
Date:	Thu, 26 Jul 2012 22:51:51 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	Laxman Dewangan <ldewangan@...dia.com>,
	Liam Girdwood <lrg@...com>, linux-kernel@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org,
	Gyungoh Yoo <jack.yoo@...im-ic.com>,
	Stephen Warren <swarren@...dia.com>
Subject: Re: [PATCH] mfd: add MAX8907 core driver

On Thu, Jul 26, 2012 at 03:14:21PM -0600, Stephen Warren wrote:
> On 07/26/2012 02:35 PM, Mark Brown wrote:

> > This looks very suspicious...  why do we need to call 
> > irqd_irq_disabled() here?

> I believe the status register reflects the unmasked status, it's just
> the interrupt signal that's affected by the mask.

This is totally normal, the standard pattern here is to and the status
register with the mask register before parsing.

> So the idea here was that the IRQ core is already maintaining state
> which describes which IRQs are enabled/disabled and wake/not. Rather
> than have irq_enable/irq_disable/set_wake do nothing but save the same
> state to irq_chip-specific structures, I removed the body of those
> functions and instead just call irqd_irq_disabled() etc. wherever I
> would have accessed the "local" state. Is that not a legitimate design
> then?

It seems very smelly, if you were supposed to do this then you wouldn't
have to be defining the empty operations.  There's also the counter
argument that it means you've got to go through every interrupt and work
out the state before syncing rather than just being able to blast the
register states in but that's fairly weak.

I think it's a totally sensible idea to reuse the state in the core, but
I do think it's a bad idea to dance around the core's back like this
with the empty operations.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)