From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Grandegger Subject: Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card Date: Wed, 15 Feb 2012 09:37:27 +0100 Message-ID: <4F3B6EC7.2090400@grandegger.com> 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> <4F396AC9.8090308@hartkopp.net> <4F397145.8040007@grandegger.com> <4F3A307C.1050706@hartkopp.net> <4F3A3493.8010900@peak-system.com> <4F3A8ECB.7090803@hartkopp.net> <4F3B58D1.1040804@grandegger.com> <4F3B6755.7080602@hartkopp.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from ngcobalt02.manitu.net ([217.11.48.102]:50740 "EHLO ngcobalt02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757736Ab2BOIhg (ORCPT ); Wed, 15 Feb 2012 03:37:36 -0500 In-Reply-To: <4F3B6755.7080602@hartkopp.net> Sender: linux-can-owner@vger.kernel.org List-ID: To: Oliver Hartkopp Cc: Stephane Grosjean , Marc Kleine-Budde , linux-can Mailing List On 02/15/2012 09:05 AM, Oliver Hartkopp wrote: > On 15.02.2012 08:03, Wolfgang Grandegger wrote: >=20 >> Hi Oliver, >> >> we start paving common code to partially cure pcmcia related problem= s, >> that's exactly what I do not like. >=20 >=20 > It's PCMCIA and PCIeC and Parallel port, etc. Any pluggable sja1000 h= ardware. >=20 >> >> On 02/14/2012 05:41 PM, Oliver Hartkopp wrote: >>> On 14.02.2012 11:16, Stephane Grosjean wrote: >>> >>>> >>>> Le 14/02/2012 10:59, Oliver Hartkopp a =E9crit : >>>> >>>>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *= dev_id) >>>>> while ((isrc =3D priv->read_reg(priv, REG_IR))&& (n< SJA1= 000_MAX_IRQ)) { >> >> In case of unplug, already the above read may return 0xff. >=20 >=20 > Right. But it does not get a bad effect as we quit from this bad cond= ition > three lines later. No previously retrieved isrc variable is reference= d then. > Please re-check the patched sja1000_interrupt() as a whole. >=20 >> >>>>> n++; >>>>> status =3D priv->read_reg(priv, REG_SR); >>>>> + /* check for absent controller due to hw unplug */ >>>>> + if (status =3D=3D 0xFF&& !sja1000_probe_chip(dev)) >>>>> + break; >> >> Once... >> >>>>> if (isrc& IRQ_WUI) >>>>> netdev_warn(dev, "wakeup interrupt\n"); >>>>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *= dev_id) >>>>> while (status& SR_RBS) { >>>>> sja1000_rx(dev); >>>>> status =3D priv->read_reg(priv, REG_SR); >>>>> + /* check for absent controller */ >>>>> + if (status =3D=3D 0xFF&& !sja1000_probe_chip(de= v)) >>>>> + break; >> >> ... twice... What about the other accesses? >=20 >=20 > Which ones do you have in mind? >=20 > The really only thing that could happen, that exactly ONE CAN frame c= an be > damaged on read while accessing the SJA1000 registers. This can not b= e solved > unless you check for sja1000_probe_chip() at the end of reading the f= rame and > before pushing the skbuff to the rx queue. >=20 > But this would add indeed a new ioport access each time we read a CAN= frame. >=20 > I don't think that we should do this additional effort - even if it's= probably > the right(TM) thing to do 8-) Well, see my comment below. >=20 > My patch doesn't do any additional ioport accesses in a 'good' condit= ion. >=20 > (..) >=20 >>> But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq = =2E.. >>> >=20 >>> So if we add the 'return IRQ_NONE' to the above patch the private c= heck for >>> present hardware becomes obsolete as the private ISRs terminate pro= perly when >>> they get an IRQ_NONE. >>> >>> Right? >> >> If it's clear that the interrupt came from that device, we should re= turn >> IRQ_HANDLED otherwise it's treated as spurious. >=20 >=20 > Can this usually happen in the case of unplugging devices? >=20 > Or we need some kind of wrapper for the sja1000 isr that returns >=20 > IRQ_NONE > IRQ_HANDLED > IRQ_PLUGOUT >=20 > while HANDLED & PLUGOUT would lead to IRQ_HANDLED on the top level. >=20 >> I would accept *one* "if >> (status =3D=3D 0xFF ..." check in a good place. >=20 >=20 > Sorry but you need an additional check within the while statement. > There are two potential occurrences where we read the status. Why che= cking > only one time? As i wrote above: Please re-check the entire patched f= unction. Why do we care so much about improper handling of the hardware. It is *illegal* to unplug the device while it's under operation and if it happens we cannot guarantee valid behavior and data anyway. The system should not hang, of course, everything else is not that dramatic, I think. Mark, what is your opinion? Wolfgang.