From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755409AbbFOP5h (ORCPT ); Mon, 15 Jun 2015 11:57:37 -0400 Received: from mx.dave-tech.it ([2.229.21.40]:60668 "EHLO mx.dave-tech.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753425AbbFOP5c (ORCPT ); Mon, 15 Jun 2015 11:57:32 -0400 Message-ID: <557EF5DD.9070300@dave-tech.it> Date: Mon, 15 Jun 2015 17:57:17 +0200 From: Andrea Scian User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Alexandre Belloni CC: r.cerrato@til-technologies.fr, a.zummo@towertech.it, rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org, Andrea Scian Subject: Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user References: <1432628260-24652-1-git-send-email-rnd4@dave-tech.it> <20150608154251.GC5222@piout.net> <55785615.4050709@dave-tech.it> <20150612074245.GC3890@piout.net> In-Reply-To: <20150612074245.GC3890@piout.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alexandre, On 12/06/2015 09:42, Alexandre Belloni wrote: > On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote : >>> I would return -EINVAL here because the result might still pass >>> rtc_valid_tm() but be outdated. >> At first look I agree with you, but a bit later they say: >> >> /* the clock can give out invalid datetime, but we cannot return >> * -EINVAL otherwise hwclock will refuse to set the time on bootup. >> */ >> >> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91 >> >> so they don't actually return -EINVAL even if rtc_valid_tm() fails. >> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something.. >> > This has been copy pasted from other drivers and this is simply not > true. Thanks for point this out. I'll split the patch to fix this for all PCF drivers (which have nearly all the same structure) and later add the OFS flag >> If the comment above is correct, so we can't return -EINVAL, I will reset >> the time to epoch, with something like >> >> rtc_time64_to_tm((time64_t)0, tm); >> > Doing that is worse. You really want userspace to know that the time is > invalid instead of giving an incorrect value. This allow userspace to > actually choose its policy when the time is invalid. For example, use > epoch or any other later date that probabyl makes more sense for the > product. Most of minimal RFS I saw reset the date to what's inside /etc/timestamp (which is updated in runlevel 6). However, this is OT here. >>>> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev, >>>> switch (cmd) { >>>> case RTC_VL_READ: >>>> if (pcf2127->voltage_low) >>>> - dev_info(dev, "low voltage detected, date/time is not reliable.\n"); >>>> + dev_info(dev, "low voltage detected, check/replace battery\n"); >>> I would also print a warning about OFS here. >>> >> I'll do. >> Do you think is better to add another variable inside struct pcf2127 or is >> better to re-read the RTC registers? >> (for the former I have also to clear the variable inside >> pcf2127_set_datetime(), for the latter I have to issue another read in a >> function that, at the moment, does not read anything..) >> > I don't really care. But since one of them is already cached, it is > probably better to cache OFS. Maybe you could also use voltage_low as a > bit field which would allow userspace to make the difference between a > simple low voltage and the time loss condition. > > I'll cache OFS too, in a different variable. Returning different values depending on OFS when querying about voltage low may mislead some application. Moreover I think that there's may be some cases when OFS is set and voltage low is not (e.g. when replacing battery with a brand new one). I'll send the updated patch soon Thanks for your comments/help, -- Andrea SCIAN DAVE Embedded Systems