From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 830D9C43381 for ; Tue, 19 Feb 2019 13:40:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5D44521773 for ; Tue, 19 Feb 2019 13:40:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728802AbfBSNkk convert rfc822-to-8bit (ORCPT ); Tue, 19 Feb 2019 08:40:40 -0500 Received: from foss.arm.com ([217.140.101.70]:45536 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726221AbfBSNkk (ORCPT ); Tue, 19 Feb 2019 08:40:40 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CE57EA78; Tue, 19 Feb 2019 05:40:39 -0800 (PST) Received: from why.wild-wind.fr.eu.org (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2B6563F675; Tue, 19 Feb 2019 05:40:37 -0800 (PST) Date: Tue, 19 Feb 2019 13:40:33 +0000 From: Marc Zyngier To: Bartosz Golaszewski Cc: Linus Walleij , Thomas Gleixner , Uwe =?UTF-8?B?S2xlaW5lLUvDtm5pZw==?= , "open list:GPIO SUBSYSTEM" , Linux Kernel Mailing List , Bartosz Golaszewski Subject: Re: [PATCH v5 1/7] irq/irq_sim: add irq_set_type() callback Message-ID: <20190219134033.23a53fc1@why.wild-wind.fr.eu.org> In-Reply-To: References: <20190218164138.24921-1-brgl@bgdev.pl> <20190218164138.24921-2-brgl@bgdev.pl> <20190219122524.2e2b01db@why.wild-wind.fr.eu.org> Organization: ARM Ltd X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 19 Feb 2019 14:20:03 +0100 Bartosz Golaszewski wrote: > wt., 19 lut 2019 o 13:25 Marc Zyngier napisał(a): > > > > On Mon, 18 Feb 2019 17:41:32 +0100 > > Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski > > > > > > 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 > > > --- > > > 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. -- Without deviation from the norm, progress is not possible.