linux-kernel - RE: [PATCH] rtc: pcf2127: clear the flag TSF1 before enabling interrupt generation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB6PR0401MB2438C8601E14BBD7F2C4DAE78FF30@DB6PR0401MB2438.eurprd04.prod.outlook.com>
Date:   Wed, 2 Dec 2020 02:14:46 +0000
From:   "Biwen Li (OSS)" <biwen.li@....nxp.com>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "Biwen Li (OSS)" <biwen.li@....nxp.com>
CC:     Leo Li <leoyang.li@....com>, Anson Huang <anson.huang@....com>,
        Aisheng Dong <aisheng.dong@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jiafei Pan <jiafei.pan@....com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>
Subject: RE: [PATCH] rtc: pcf2127: clear the flag TSF1 before enabling
 interrupt generation

> 
> Hi,
> 
> On 01/12/2020 16:47:46+0800, Biwen Li wrote:
> > From: Biwen Li <biwen.li@....com>
> >
> > - clear the flag TSF1 before enabling interrupt generation
> > - properly set flag WD_CD for rtc chips(pcf2129, pca2129)
> >
> 
> This change has to be a separate patch.
Sure, np. Will separate the patch in v2.
> 
> > Signed-off-by: Biwen Li <biwen.li@....com>
> > ---
> >  drivers/rtc/rtc-pcf2127.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 07a5630ec841..0a45e2512258 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -601,6 +601,10 @@ static int pcf2127_probe(struct device *dev, struct
> regmap *regmap,
> >  	 * Watchdog timer enabled and reset pin /RST activated when timed out.
> >  	 * Select 1Hz clock source for watchdog timer.
> >  	 * Note: Countdown timer disabled and not available.
> > +	 * For pca2129, pcf2129, only bit[7] is for Symbol WD_CD
> > +	 * of register watchdg_tim_ctl. The bit[6] is labeled
> > +	 * as T. Bits labeled as T must always be written with
> > +	 * logic 0.
> >  	 */
> >  	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> >  				 PCF2127_BIT_WD_CTL_CD1 |
> > @@ -608,7 +612,7 @@ static int pcf2127_probe(struct device *dev, struct
> regmap *regmap,
> >  				 PCF2127_BIT_WD_CTL_TF1 |
> >  				 PCF2127_BIT_WD_CTL_TF0,
> >  				 PCF2127_BIT_WD_CTL_CD1 |
> > -				 PCF2127_BIT_WD_CTL_CD0 |
> > +				 has_nvmem ? (PCF2127_BIT_WD_CTL_CD0) : (0) |
> 
> I don't like that because has_nvmem has nothing to do with
> PCF2127_BIT_WD_CTL_CD0 and nothing guarantees that we won't ever get an
> RTC without RST but with NVRAM and that willprobbly be overlooked.
Okay, got it. Will correct it in v2.
> 
> >  				 PCF2127_BIT_WD_CTL_TF1);
> >  	if (ret) {
> >  		dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__); @@
> > -659,6 +663,21 @@ static int pcf2127_probe(struct device *dev, struct
> regmap *regmap,
> >  		return ret;
> >  	}
> >
> > +	/*
> > +	 * Clear TSF1 field of ctrl1 register to clear interrupt
> > +	 * before enabling interrupt generation when
> > +	 * timestamp flag set. Unless the flag TSF1 won't
> > +	 * be cleared and the interrupt(INT pin) is
> > +	 * triggered continueously.
> > +	 */
> > +	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> > +				 PCF2127_BIT_CTRL1_TSF1,
> > +				 0);
> > +	if (ret) {
> > +		dev_err(dev, "%s:  control and status register 1 (ctrl1) failed, ret =
> 0x%x\n",
> > +			__func__, ret);
> > +		return ret;
> > +	}
> 
> Doing that means ignoring timestamps taken while the system is offline.
> It also doesn't fully solve the issue because you are not clearing TSF2 here and
> also it never gets cleared by the driver later on so I guess you will get the
> interrupt storm once a timestamp is taken.
Okay, got it. Thanks. Will clear TSF2 flag in v2.
> 
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com