From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Grandegger Subject: Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling Date: Mon, 18 Nov 2013 21:04:52 +0100 Message-ID: <528A72E4.2070501@grandegger.com> References: <5288CFEF.3060701@hartkopp.net> <52892C36.8060709@grandegger.com> <5289B0EF.3070507@hartkopp.net> <20131118194833.GC1268@vandijck-laurijssen.be> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ngcobalt02.manitu.net ([217.11.48.102]:33107 "EHLO ngcobalt02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752355Ab3KRUEz (ORCPT ); Mon, 18 Nov 2013 15:04:55 -0500 In-Reply-To: <20131118194833.GC1268@vandijck-laurijssen.be> Sender: linux-can-owner@vger.kernel.org List-ID: To: Oliver Hartkopp , "linux-can@vger.kernel.org" On 11/18/2013 08:48 PM, Kurt Van Dijck wrote: > Oliver, > > just 2 small remarks below. > > On Mon, Nov 18, 2013 at 07:17:19AM +0100, Oliver Hartkopp wrote: >> >> >> On 17.11.2013 21:51, Wolfgang Grandegger wrote: >>> Hi Oliver, >>> >>> On 11/17/2013 03:17 PM, Oliver Hartkopp wrote: >>>> The SJA1000 driver provides optional function pointers to handle specific >>>> hardware setups at interrupt time. >>>> >>>> This patch fixes the issue that the sja1000_interrupt() function may have >>>> returned IRQ_NONE without processing the post_irq function. Additionally >>>> an introduced return value from the pre_irq function is checked to skip the >>>> entire interrupt handling on pre_irq function errors. >>>> >>>> Reported-by: Wolfgang Grandegger >>>> Signed-off-by: Oliver Hartkopp >>>> >>>> --- >>>> >>>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c >>>> index 7164a99..bb20470 100644 >>>> --- a/drivers/net/can/sja1000/sja1000.c >>>> +++ b/drivers/net/can/sja1000/sja1000.c >>>> @@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id) >>>> uint8_t isrc, status; >>>> int n = 0; >>>> >>>> + if (priv->pre_irq && priv->pre_irq(priv)) >>>> + goto out; >>>> + >>> >>> Well, not sure if we need that. Looks like a fatal error to me. >>> >> >> Why? >> Think about a pre_irq() function that can look into hw specific registers and >> which can determine, that there was no interrupt for this CAN device. >> In this case the access to the sja1000 registers might/should be skipped >> depending on the new return value of pre_irq(). >> >> So what's the error here? > > The accuracy of reading register SJA1000_IR remains unbeaten when > deciding if an SJA1000 has interrupts pending. > Any pre_irq() return value is close approximation. > >> Btw. by now there's no user of priv->pre_irq() anyway. > > "No in-kernel users" is a strong argument for deleting > the pre_irq entirely. In general yes. But we may rename that callback to "irq_is_pending" and use it as fast path to "return IRQ_NONE". I could then be used on the peak_pci to save quite a few instructions. Wolfgang.