From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753230AbbDXKVi (ORCPT ); Fri, 24 Apr 2015 06:21:38 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:39647 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751461AbbDXKVg convert rfc822-to-8bit (ORCPT ); Fri, 24 Apr 2015 06:21:36 -0400 From: Juergen Borleis Organization: Pengutronix e.K. To: linux-arm-kernel@lists.infradead.org Subject: Re: [rtc-linux] [PATCH 2/5] RTC/i.MX/DryIce: add the unit recovery code Date: Fri, 24 Apr 2015 12:24:02 +0200 User-Agent: KMail/1.9.10 (enterprise35 0.20100827.1168748) Cc: Alexandre Belloni , Alessandro Zummo , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org, Robert Schwebel , kernel@pengutronix.de References: <1429002716-19821-1-git-send-email-jbe@pengutronix.de> <1429002716-19821-3-git-send-email-jbe@pengutronix.de> <20150421231411.GC8539@piout.net> In-Reply-To: <20150421231411.GC8539@piout.net> X-KMail-QuotePrefix: > MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT Content-Disposition: inline Message-Id: <201504241224.02705.jbe@pengutronix.de> X-SA-Exim-Connect-IP: 2001:6f8:1178:4:5054:ff:fe8d:eefb X-SA-Exim-Mail-From: jbe@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alexandre, On Wednesday 22 April 2015 01:14:11 Alexandre Belloni wrote: > On 14/04/2015 at 11:11:53 +0200, Juergen Borleis wrote : > > This code is requiered to recover the unit from a security violation. > > required ^ Sure :) > > [...] > > +/* do a write into the unit without interrupt support */ > > +static void di_write_busy_wait(const struct imxdi_dev *imxdi, u32 val, > > + unsigned reg) > > Alignment should match the open parenthesis, please fix all occurrences > in your patch. Okay. > > +{ > > + /* do the register write */ > > + __raw_writel(val, imxdi->ioaddr + reg); > > + > > + /* > > + * now it takes four 32,768 kHz clock cycles to take > > + * the change into effect = 122 us > > + */ > > + udelay(200); > > As the requirement is not that critical, you may want to use usleep_range() Good point. Will change it. > > +} > > + > > +static void di_report_tamper_info(struct imxdi_dev *imxdi, u32 dsr) > > +{ > > + u32 dtcr; > > + > > + dtcr = __raw_readl(imxdi->ioaddr + DTCR); > > + > > + dev_emerg(&imxdi->pdev->dev, "!! DryIce tamper event detected !!\n"); > > + /* the following flags force a transition into the "FAILURE STATE" */ > > + if (dsr & DSR_VTD) { > > + dev_emerg(&imxdi->pdev->dev, "!! Voltage Tamper event\n"); > > + if (!(dtcr & DTCR_VTE)) > > + dev_emerg(&imxdi->pdev->dev, > > + "!! But Voltage Tamper detection wasn't enabled. False positive?\n"); > > I'm not sure about prefixing all the messages with "!! ". dev_emerg() > seems important enough to be noticed. > I would remove " False positive?". What about > dev_emerg(&imxdi->pdev->dev, > "%sVoltage Tamper event\n", > (dtcr & DTCR_VTE) ? "" : "Spurious "); "spurious" sounds better. And I will change the messages. > [...] > > +static void di_what_is_to_be_done(struct imxdi_dev *imxdi, > > + const char *power_supply) > > +{ > > + dev_emerg(&imxdi->pdev->dev, " Please cycle the %s power supply in order to get the DryIce unit working again\n", > > remove that heading space ^ Yes. > I would also explain that the RTC is part of the DryIce unit. Within this message? Makes sense. Will do so. > [...] > > + sec); > > + /* > > + * the timer cannot be set/modified if > > + * - the TCHL or TCSL bit is set in DCR > > + */ > > + dcr = __raw_readl(imxdi->ioaddr + DCR); > > + if (!(dcr & DCR_TCE)) { > > + if (dcr & DCR_TCHL) { > > + /* we are out of luck */ > > + di_what_is_to_be_done(imxdi, "battery"); > > + return -ENODEV; > > + } > > + if (dcr & DCR_TCSL) { > > + di_what_is_to_be_done(imxdi, "main"); > > + return -ENODEV; > > + } > > + > > Unnecessary blank line Ups. Made accidentally. > [...] > > + /* success? */ > > + dsr = __raw_readl(imxdi->ioaddr + DSR); > > + if (dsr & DSR_SVF) { > > + dev_crit(&imxdi->pdev->dev, > > + "Cannot clear the security violation flag. We are ending up in an endless loop!\n"); > > + /* last resourt */ > > resort ^ .oO(note to myself: update dictionary...) > [...] > > @@ -498,44 +793,6 @@ static int __init dryice_rtc_probe(struct platform_device *pdev) goto err; > > } > > > > - /* put dryice into valid state */ > > - if (__raw_readl(imxdi->ioaddr + DSR) & DSR_NVF) { > > - rc = di_write_wait(imxdi, DSR_NVF | DSR_SVF, DSR); > > Multiple writes have switched from di_write_wait() which is checking > for a write error to di_write_busy_wait() which is not doing that check > is waiting 200us enough to ensure that the write has been done? Each write requires four 32 kHz clock cycles. So the 200 us should be enough. But I will take a closer look for what have to be really done in the case of an access error. Regards, Juergen -- Pengutronix e.K.                              | Juergen Borleis             | Industrial Linux Solutions      | http://www.pengutronix.de/ |