From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oliver Hartkopp Subject: Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card Date: Mon, 13 Feb 2012 20:55:53 +0100 Message-ID: <4F396AC9.8090308@hartkopp.net> References: <1328543779-9209-1-git-send-email-s.grosjean@peak-system.com> <4F38D46D.5010406@pengutronix.de> <4F38DF75.3040000@peak-system.com> <4F38E274.4050304@grandegger.com> <4F38E8DA.9060000@hartkopp.net> <4F38EDB1.7080209@grandegger.com> <4F38EEAD.3060607@pengutronix.de> <4F38EF3A.7040907@grandegger.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from mo-p00-ob.rzone.de ([81.169.146.161]:19012 "EHLO mo-p00-ob.rzone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755787Ab2BMTzv (ORCPT ); Mon, 13 Feb 2012 14:55:51 -0500 In-Reply-To: <4F38EF3A.7040907@grandegger.com> Sender: linux-can-owner@vger.kernel.org List-ID: To: Wolfgang Grandegger Cc: Marc Kleine-Budde , Stephane Grosjean , linux-can Mailing List On 13.02.2012 12:08, Wolfgang Grandegger wrote: > On 02/13/2012 12:06 PM, Marc Kleine-Budde wrote: >> On 02/13/2012 12:02 PM, Wolfgang Grandegger wrote: >>> On 02/13/2012 11:41 AM, Oliver Hartkopp wrote: >>>> On 13.02.2012 11:14, Wolfgang Grandegger wrote: >>>> >>>> >>>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c >>>>>>> b/drivers/net/can/sja1000/sja1000.c >>>>>>> index ebbcfca..f7526a7 100644 >>>>>>> --- a/drivers/net/can/sja1000/sja1000.c >>>>>>> +++ b/drivers/net/can/sja1000/sja1000.c >>>>>>> @@ -493,6 +493,10 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id) >>>>>>> n++; >>>>>>> status = priv->read_reg(priv, REG_SR); >>>>>>> >>>>>>> + /* check for absent controller due to hw unplug */ >>>>>>> + if (status == 0xFF) >>>>>>> + break; >>>>>>> + >>>>>>> if (isrc& IRQ_WUI) >>>>>>> netdev_warn(dev, "wakeup interrupt\n"); >>>>>>> >>>>>>> @@ -504,8 +508,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id) >>>>>>> netif_wake_queue(dev); >>>>>>> } >>>>>>> if (isrc& IRQ_RI) { >>>>>>> - /* receive interrupt */ >>>>>>> - while (status& SR_RBS) { >>>>>>> + /* receive interrupt / check for absent controller */ >>>>>>> + while (status& SR_RBS&& status != 0xFF) { >>>>>>> sja1000_rx(dev); >>>>>>> status = priv->read_reg(priv, REG_SR); >>>>>>> } >>>>>>> >>>>>>> @Stephane: Can you check that patch? I'm out of hw right now. >>>>>> >>>>>> I confirm that this patch works too... >>>>>> So I think I should be able to post a new version of the peak_pcmcia >>>>>> during that day (the previous should work but some calls to >>>>>> pcmcia_dev_present() are no more useful...) >>>>> >>>>> But that fix should not go to the common interrupt handler, if possible. >>>> >>>> >>>> Do you have an idea how to handle the while() statement without copying the >>>> entire interrupt handler code for the devices that might be unplugged? >>>> >>>> IMHO this patch is pretty cheap. >>> >>> I think it's enough to add a read in the custom pcmcia isr before >>> calling sja1000_interrupt(). Maybe the race window is a bit lower but we >>> are not able to react when the unplug happens anyway. Does this mean, it is 'less bad'? >> It's not about reacting when an unlug happens, it's about not having the >> interrupt handler to loop forever. At least we must limit the inner >> while loop. What happens, if inside this inner while() statement the unplug happens? Do we get two correct CAN frames and then we get eight CAN frames like this: 12345677 4 AA BB CC DD 12345678 6 AA BB CC DD EE FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF 1FFFFFFF 8 FF FF FF FF FF FF FF FF ??? Sending nothing is preferred to sending wrong data :-) > Of course, the loop should be limited in sja1000_interrupt anyway. IMO checking for absent hardware in the custom isr is fine and should fix most of the problems (as we can already see in the ems_pcmcia driver). But adding the patch which is checking two times for status != 0xFF reduces the race conditions to almost zero. Having a while() statement that potentially produces wrong data doesn't heal the problem IMO. Regards, Oliver