Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935375AbdCJKgF (ORCPT ); Fri, 10 Mar 2017 05:36:05 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:55241 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935290AbdCJKgB (ORCPT ); Fri, 10 Mar 2017 05:36:01 -0500 Date: Fri, 10 Mar 2017 11:35:53 +0100 (CET) From: Thomas Gleixner To: gengdongjiu cc: jason@lakedaemon.net, benh@kernel.crashing.org, linux-kernel@vger.kernel.org, zhanghaibin7@huawei.com, huangshaoyu@huawei.com Subject: Re: [PATCH] irqdomain: handle the per-CPU irq trigger type settings In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2072 Lines: 69 On Fri, 10 Mar 2017, gengdongjiu wrote: > when devices parse and map an per-cpu interrupt into linux virq space > using irq_of_parse_and_map API, it will always be failed if needs to set > the specified irq trigger type, because irq_set_irq_type is only for 1-N > mode interrupt source, not for per-cpu interrupt source. so handle per-cpu > IRQs for this failure. Please format your changelogs proper into sections: 1) Context 2) Problem 3) Solution Writing one big lump of a sentence is just unreadable. Aside of that: 1) No indentation of the changelog 2) Sentences start with upper case letters 3) Function references want () after the function name Aside > > Signed-off-by: Dongjiu Geng > Zhanghai bin That SOB is bogus. > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index 9fd618d..8116cf2 100755 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -542,8 +542,16 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data) > > /* Set type if specified and different than the current one */ > if (type != IRQ_TYPE_NONE && > - type != irq_get_trigger_type(virq)) > - irq_set_irq_type(virq, type); > + type != irq_get_trigger_type(virq)) { > + int ret = 0; > + struct irq_data *irq_data = irq_get_irq_data(virq); > + > + ret = irq_set_irq_type(virq, type); > + > + /* Handle per-cpu IRQ: just save type in irq_data */ > + if (-EINVAL == ret && irq_data) > + irqd_set_trigger_type(irq_data, type); This is completely broken. That stores a trigger type for any interrupt if the set type function fails. You fail to explain - WHY irq_set_irq_type() fails for these per cpu interrupts - WHY storing the type in irqdata solves anything - WHAT sets the type in the actual interrupt hardware That information should be in the changelog .... Thanks, tglx