linux-kernel - Re: [RFC] regmap-irq: add "main register" and level-irq support

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]

Date:   Fri, 7 Dec 2018 13:14:18 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     gregkh@...uxfoundation.org, rafael@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC] regmap-irq: add "main register" and level-irq support

On Fri, Dec 07, 2018 at 09:58:29AM +0200, Matti Vaittinen wrote:
> On Wed, Dec 05, 2018 at 05:27:01PM +0000, Mark Brown wrote:

> > If they're full subdevices you'd be pointing at the relevant platform
> > device anyway.

> I am not sure if I have explained me well enough =) When we look at
> regmap_add_irq_chip we see that it creates the IRQ domains. And we
> further see:
> 
> 		d->domain = irq_domain_add_linear(map->dev->of_node,
> 						  chip->num_irqs,
> 						  &regmap_domain_ops, d);

> where map->dev->of_node points the node added to regmap. And as we
> really want to use the i2c to access registers, we should have done the
> regmap using:

> devm_regmap_init_i2c(i2c, &regmap);

> where the i2c is the struct i2c_client *. The dev in i2c_client is the
> i2c device - which has of_node pointing at the "main i2c node" - not the
> sub block nodes. Hence all irq chips created by regmap_add_irq_chip for
> this physical i2c slave device will point to the same DT node. 

Hrm, right.  You'd need to have a proxy regmap for the child devices for
that.  Not ideal.

> 	bool mask_writeonly:1;
> +	bool no_of:1;
> 
> and in the regmap_add_irq_chip do:

> 		node = (chip->no_of) ? NULL : map->dev->of_node;
> 		d->domain = irq_domain_add_linear(node, chip->num_irqs,
> 						  &regmap_domain_ops, d);

> Then we could have chip->no_of set for the 'main irq chip' and for those
> chips we don't wan't to expose via DT. In my case I would leave no_of
> unset only for the irq-chip which I created for the GPIO? Is this a
> silly idea?

That's worth a shot, yes - I'd need to see it fully fleshed out I think
but it looks sensible (no ternery operator please).

> > The mapping isn't guaranteed to be a 1:1 mapping - one way this hardware
> > gets structured is that the central interrupt controller is saying which
> > IP block is flagging an interrupt rather than which register has an
> > interrupt set in it.  You can then have either more than one detailed
> > status register for that IP

> Correct. But I guess the IP blocks often have limited set of registers for the
> "sub interrupts". For such cases my idea would work, right. My RFC
> handled case where there is many 'sub registers' to read for one 'main
> bit.

Your idea definitely works for the current case, yes - I'm just thinking
about future edge and extension cases.

> > or several smaller IPs reporting through a
> > single detailed status register.

> I think that if we have more than two layers of irqs (main and sub) -
> then we should do cascaded controllers.

Yeah, and my first thought here is that we should just be using cascaded
controllers all the time (but like I say I'm not 100% certain on that).

> > Right, it's about working out which subregister to read - the point is
> > you can do this by adding a link in either direction, I'm suggesting
> > doing it from the individual interrupt to the main register since we
> > already have individual data structures for those and it feels less
> > likely to run into hard to represent corner cases.

> I see your point now. But as I said, I am not sure we should add the
> overhead of 'main irq bit description' for simple cases just to cover
> the corner cases. Yet I can try seeing what I can come up with if you
> think this is the way to go.

If you could take a look that'd be great.

> > >  * @main_status: Optional base main status register address. Some chips have
> > >  *		 "main status register(s)" which indicate actual irq registers
> > >  *		 with unhandled interrupts. Provide main status register
> > >  *		 address here to avoid reading irq registers with no
> > >  *		 active interrupts. Mapping from main status value to
> > >  *		 interrupt register can be provided in sub_reg_offsets.

> > > Perhaps this would clarify that the status_base, mask_base, ack_base,
> > > etc. should still be used normally?

> > Probably.

> I am not sure this means I was able to convince you or if this is the
> polite way to tell me not to bother =) I am Finnish guy who does not
> understand small talk or polite hints ;)

I mean that probably if you add clarifications like you suggest that
might do it.  It's one of these things where you can't 100% tell how
things will look until you see the actual thing.

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