Re: [PATCH v5 1/7] irq/irq_sim: add irq_set_type() callback

[Bartosz Golaszewski]

wt., 19 lut 2019 o 13:25 Marc Zyngier <marc.zyngier@xxxxxxx> napisaÅ(a):
>
> On Mon, 18 Feb 2019 17:41:32 +0100
> Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
> > Implement the irq_set_type() callback and call irqd_set_trigger_type()
> > internally so that users interested in the configured trigger type can
> > later retrieve it using irqd_get_trigger_type().
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > ---
> >  kernel/irq/irq_sim.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > index 98a20e1594ce..83ecc65d8be2 100644
> > --- a/kernel/irq/irq_sim.c
> > +++ b/kernel/irq/irq_sim.c
> > @@ -25,10 +25,18 @@ static void irq_sim_irqunmask(struct irq_data *data)
> >       irq_ctx->enabled = true;
> >  }
> >
> > +static int irq_sim_set_type(struct irq_data *data, unsigned int type)
> > +{
> > +     irqd_set_trigger_type(data, type);
> > +
> > +     return 0;
>
> You keep ignoring the requirement for sanitization of the trigger type.
> Frankly, I'm getting tired of fighting over 3 lines of incorrect code.
>

It used to be there in previous versions, but I removed it on purpose
in v5. I understand why we needed that earlier, but if we now moved
*all* the logic behind the trigger type to the users of this API and
we're now simply storing any config we get, then why would we impose
any limits on it here? We don't do this now and this patch doesn't
change the current behavior. I really don't understand how not
rejecting certain trigger types makes this patch incorrect.

> I guess that despite all the noise, you don't really want this code in
> after all.
>

I do want and need this, but I really can't figure out from your
reviews how you imagine the correct solution. You said previously that
the irq_set_type callback should push the configuration to wherever
it's needed. I believe the above patch does this. Should we then limit
the supported trigger types? That would mean that irq_sim would need
to know what users support. It seems inverted to me, but if you think
it's right, then my question is: will accepting only IRQ_TYPE_NONE,
IRQ_TYPE_EDGE_RISING and IRQ_TYPE_EDGE_FALLING in the above function
be enough for you to accept it? Is the rest fine?

Thanks,
Bartosz