Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751317AbdH3WwZ (ORCPT ); Wed, 30 Aug 2017 18:52:25 -0400 Received: from ozlabs.org ([103.22.144.67]:40955 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750814AbdH3WwY (ORCPT ); Wed, 30 Aug 2017 18:52:24 -0400 From: Michael Ellerman To: Gregory Fong , linuxppc-dev@lists.ozlabs.org Cc: "linux-kernel\@vger.kernel.org" Subject: Re: mpic IRQ_TYPE_BOTH handling In-Reply-To: References: User-Agent: Notmuch/0.21 (https://notmuchmail.org) Date: Thu, 31 Aug 2017 08:52:21 +1000 Message-ID: <87efrs8y8q.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2183 Lines: 63 Hi Gregory, Gregory Fong writes: > Hi all, > > In arch/powerpc/sysdev/mpic.c , it looks like IRQ_TYPE_EDGE_BOTH is > handled the same way as IRQ_TYPE_EDGE_FALLING: > > static unsigned int mpic_type_to_vecpri(struct mpic *mpic, unsigned int type) > { > /* Now convert sense value */ > switch(type & IRQ_TYPE_SENSE_MASK) { > case IRQ_TYPE_EDGE_RISING: > return MPIC_INFO(VECPRI_SENSE_EDGE) | > MPIC_INFO(VECPRI_POLARITY_POSITIVE); > case IRQ_TYPE_EDGE_FALLING: > case IRQ_TYPE_EDGE_BOTH: > return MPIC_INFO(VECPRI_SENSE_EDGE) | > MPIC_INFO(VECPRI_POLARITY_NEGATIVE); > case IRQ_TYPE_LEVEL_HIGH: > return MPIC_INFO(VECPRI_SENSE_LEVEL) | > MPIC_INFO(VECPRI_POLARITY_POSITIVE); > case IRQ_TYPE_LEVEL_LOW: > default: > return MPIC_INFO(VECPRI_SENSE_LEVEL) | > MPIC_INFO(VECPRI_POLARITY_NEGATIVE); > } > } > > If IRQ_TYPE_EDGE_BOTH is unsupported, shouldn't we be returning an > error, instead of silently setting to use IRQ_TYPE_EDGE_FALLING? > Something like the following (sorry if the diff wraps weirdly, on > webmail at the moment): I don't know this code so I asked Ben and he said something like "PowerMacs never use BOTH, so it hasn't mattered, but Freescale machines might". So if you want to send a proper signed-off patch, and confirm that all the callers can handle the error properly, then we can merge it. cheers > ----8<---- > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c > index b9aac95..5867ea2 100644 > --- a/arch/powerpc/sysdev/mpic.c > +++ b/arch/powerpc/sysdev/mpic.c > @@ -876,6 +876,9 @@ int mpic_set_irq_type(struct irq_data *d, unsigned > int flow_type) > if (src >= mpic->num_sources) > return -EINVAL; > > + if (flow_type & IRQ_TYPE_SENSE_MASK == IRQ_TYPE_EDGE_BOTH) > + return -EINVAL; > + > vold = mpic_irq_read(src, MPIC_INFO(IRQ_VECTOR_PRI)); > > /* We don't support "none" type */ > ---->8---- > > Thanks, > Gregory