| Message ID | 20190218164138.24921-2-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mockup: improve the user-space testing interface | expand |
On Mon, 18 Feb 2019 17:41:32 +0100 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > 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@baylibre.com> > --- > 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. I guess that despite all the noise, you don't really want this code in after all. M.
wt., 19 lut 2019 o 13:25 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > On Mon, 18 Feb 2019 17:41:32 +0100 > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > 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@baylibre.com> > > --- > > 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
On Tue, 19 Feb 2019 14:20:03 +0100 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > wt., 19 lut 2019 o 13:25 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > > > On Mon, 18 Feb 2019 17:41:32 +0100 > > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > 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@baylibre.com> > > > --- > > > 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. You expose a new set_type callback. Only a set of valid values can be handled by the users of this interface. At least your mockup gpio only handles a narrow set of configuration. Not rejecting erroneous values is a bug. This isn't a new requirement; this is how the irqchip API works, as drivers do expect such a failure if requiring an invalid type. > > 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. Yes. But it doesn't mean it should accept something that cannot be handled by the irqchip. If you push it to the code that does the handling, it still has to return an error if the backing code decides that this is not a supported configuration. At the end of the day, where you store it is irrelevant for the problem at hand. There is an API, and you need to implement it correctly. > 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? IRQ_TYPE_NONE doesn't mean anything, apart from "whatever defaults were there because I have no clue". In your case, I really don't see the point. The EDGE settings are what you handle, and are the only values that should be accepted. As for the rest of the patches, I haven't looked at them, and probably won't (if only because I'm on holiday and would like to do something else...). All I'm asking from you is to give me a correct patch #1. Once I see that, I'll gladly ack it. M.
wt., 19 lut 2019 o 14:40 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > On Tue, 19 Feb 2019 14:20:03 +0100 > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > wt., 19 lut 2019 o 13:25 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > > > > > On Mon, 18 Feb 2019 17:41:32 +0100 > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > 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@baylibre.com> > > > > --- > > > > 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. > > You expose a new set_type callback. Only a set of valid values can be > handled by the users of this interface. At least your mockup gpio only > handles a narrow set of configuration. Not rejecting erroneous values > is a bug. This isn't a new requirement; this is how the irqchip API > works, as drivers do expect such a failure if requiring an invalid > type. > > > > 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. > > Yes. But it doesn't mean it should accept something that cannot be > handled by the irqchip. If you push it to the code that does the > handling, it still has to return an error if the backing code decides > that this is not a supported configuration. > > At the end of the day, where you store it is irrelevant for the problem > at hand. There is an API, and you need to implement it correctly. > > > 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? > > IRQ_TYPE_NONE doesn't mean anything, apart from "whatever defaults were > there because I have no clue". In your case, I really don't see the > point. The EDGE settings are what you handle, and are the only values > that should be accepted. > Right, irq_set_type() isn't even called if there's no trigger set. > As for the rest of the patches, I haven't looked at them, and probably > won't (if only because I'm on holiday and would like to do something > else...). All I'm asking from you is to give me a correct patch #1. > Once I see that, I'll gladly ack it. > Thanks, Bartosz
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; +} + static struct irq_chip irq_sim_irqchip = { .name = "irq_sim", .irq_mask = irq_sim_irqmask, .irq_unmask = irq_sim_irqunmask, + .irq_set_type = irq_sim_set_type, }; static void irq_sim_handle_irq(struct irq_work *work)