linux-kernel - Re: [PATCHv2] rtc: cpcap: new rtc driver

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: <20170222081805.ap2yvarymlr5vvjo@piout.net>
Date:   Wed, 22 Feb 2017 09:18:05 +0100
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     Sebastian Reichel <sre@...nel.org>
Cc:     Tony Lindgren <tony@...mide.com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        rtc-linux@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2] rtc: cpcap: new rtc driver

On 22/02/2017 at 02:56:34 +0100, Sebastian Reichel wrote:
> > Does this mean there is a race condition?
> 
> The logic (incl. comments) in this section are from the vendor
> kernel driver and there is no documentation for CPCAP as far as
> I know. I don't know if the hardware has logic to prevent a race
> condition for the cpcap_tm.tod1 == 255 case.
> 

That's fine, I was just curious :)

> > > +	err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor);
> > I think this means it depends on the mfd tree.
> 
> Yes, but cpcap_get_vendor should get into mainline with the
> 4.11 mfd pull request. So if you base your 4.12 for-next tree
> on 4.11-rc1 everything should be fine.
> 

OK, I'll take it for 4.12 then

> > > +	if (err)
> > > +		return err;
> > > +
> > > +	rtc->alarm_irq= platform_get_irq(pdev, 0);
> > > +	err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL,
> > > +					cpcap_rtc_alarm_irq, IRQ_NONE,
> > > +					"rtc_alarm", rtc);
> > > +	if (err) {
> > > +		dev_err(dev, "Could not request alarm irq: %d\n", err);
> > > +		return err;
> > > +	}
> > > +	disable_irq(rtc->alarm_irq);
> > > +
> > > +	rtc->update_irq= platform_get_irq(pdev, 1);
> > > +	err = devm_request_threaded_irq(dev, rtc->update_irq, NULL,
> > > +					cpcap_rtc_update_irq, IRQ_NONE,
> > > +					"rtc_1hz", rtc);
> 
> > I don't think this IRQ is actually useful. It doesn't really harm but
> > the tests should pass without it.
> 
> Yes. RTC works perfectly fine with just the alarm irq. It also
> works perfectly fine with just the 1 Hz irq (except for wakeup).
> 
> I would like to keep the irq in the driver, so that it's explicitly
> disabled. On Droid 4 mainline kernel is booted via kexec from
> Android (AKA bootloader) and Motorola's Android kernel uses the
> 1 Hz IRQ for some proprietary "secure clock daemon".
> 
> I will add a comment.
> 

OK, my plan was to remove all the RTC_UF users. I'll give it more
thoughts.

> > > +	if (err) {
> > > +		dev_err(dev, "Could not request update irq: %d\n", err);
> > > +		return err;
> > > +	}
> > > +	disable_irq(rtc->update_irq);
> > > +
> > > +	err = device_init_wakeup(dev, 1);
> > 
> > If you use device_init_wakeup, I think it needs to be called before
> > devm_rtc_device_register() to properly work.
> 
> I successfully tested wakeup before sending this. But in case your 
> prefer it to be called before registering the RTC I can move the
> call accordingly.
> 

Then it is fine where it is.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com