[rtc-linux] Re: [PATCH 00/51] rtc: stop using rtc deprecated functions

[Thomas Gleixner]

On Tue, 20 Jun 2017, Alexandre Belloni wrote:
> On 20/06/2017 at 22:15:36 +0100, Russell King - ARM Linux wrote:
> > On Tue, Jun 20, 2017 at 05:07:46PM +0200, Benjamin Gaignard wrote:
> > > 2017-06-20 15:48 GMT+02:00 Alexandre Belloni
> > > :
> > > >> Yes, that's argument against changing rtc _drivers_ for hardware that
> > > >> can not do better than 32bit. For generic code (such as 44/51 sysfs,
> > > >> 51/51 suspend test), the change still makes sense.
> > > 
> > > What I had in mind when writing those patches was to remove the 
> > > limitations
> > > coming from those functions usage, even more since they been marked has
> > > deprecated.
> > 
> > I'd say that they should not be marked as deprecated.  They're entirely
> > appropriate for use with hardware that only supports a 32-bit
> > representation of time.
> > 
> > It's entirely reasonable to fix the ones that use other representations
> > that exceed that, but for those which do not, we need to keep using the
> > 32-bit versions.  Doing so actually gives us _more_ flexibility in the
> > future.
> > 
> > Consider that at the moment, we define the 32-bit RTC representation to
> > start at a well known epoch.  We _could_ decide that when it wraps to
> > 0x8000 seconds, we'll define the lower 0x4000 seconds to mean
> > dates in the future - and keep rolling that forward each time we cross
> > another 0x4000 seconds.  Unless someone invents a real time machine,
> > we shouldn't need to set a modern RTC back to 1970.
> > 
> 
> I agree with that but not the android guys. They seem to mandate an RTC
> that can store time from 01/01/1970. I don't know much more than that
> because they never cared to explain why that was actually necessary
> (apart from a laconic "this will result in a bad user experience")
> 
> I think tglx had a plan for offsetting the time at some point so 32-bit
> platform can pass 2038 properly.

Yes, but there are still quite some issues to solve there:

 1) How do you tell the system that it should apply the offset in the
first place, i.e at boot time before NTP or any other mechanism can
correct it?

 2) Deal with creative vendors who have their own idea about the 'start
of the epoch'

 3) Add the information of wraparound time to the rtc device which
needs to be filled in for each device. That way the rtc_***
accessor functions can deal with them whether they wrap in 2038 or
2100 or whatever.

#3 is the simplest problem of them :)   

> My opinion is that as long as userspace is not ready to handle those
> dates, it doesn't really matter because it is quite unlikely that
> anything will be able to continue running anyway.

That's a different story. Making the kernel y2038 ready in general is a
good thing. Whether userspace will be ready by then or not is completely
irrelevant.

Thanks,

tglx


-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH v2] x86: i8259: export legacy_pic symbol

[Thomas Gleixner]

On Fri, 14 Apr 2017, Ingo Molnar wrote:

> 
> * Alexandre Belloni <alexandre.bell...@free-electrons.com> wrote:
> 
> > Hi,
> > 
> > On 08/04/2017 at 23:03:27 +0200, Hans de Goede wrote:
> > > The classic PC rtc-coms driver has a workaround for broken ACPI device
> > > nodes for it which lack an irq resource. This workaround used to
> > > unconditionally hardcode the irq to 8 in these cases.
> > > 
> > > This was causing irq conflict problems on systems without a legacy-pic
> > > so a recent patch added an if (nr_legacy_irqs()) guard to the
> > > workaround to avoid this irq conflict.
> > > 
> > > nr_legacy_irqs() uses the legacy_pic symbol under the hood causing
> > > an undefined symbol error if the rtc-cmos code is build as a module.
> > > 
> > 
> > This is kind of a pressing issue as this makes linux-next fail to build
> > for certain configurations.
> > 
> > I can carry it in my tree with your ack or let you apply it for 4.12.
> > What would you prefer?
> 
> Unless Thomas objects it looks good to me and feel free to carry it in your 
> tree:
> 
> Acked-by: Ingo Molnar <mi...@kernel.org>

Acked-by: Thomas Gleixner <t...@linutronix.de>

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH][RFC v7] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Thomas Gleixner]

On Thu, 10 Nov 2016, Chen Yu wrote:

> Previously we encountered some memory overflow issues due to
> the bogus sleep time brought by inconsistent rtc, which is
> triggered when pm_trace is enabled, and we have fixed it
> in recent kernel. However it's improper in the first place
> to call __timekeeping_inject_sleeptime() in case that pm_trace
> is enabled simply because that "hash" time value will wreckage
> the timekeeping subsystem.
> 
> This patch is originally written by Thomas, which would bypass
> the bogus rtc interval when pm_trace is enabled.
> Meanwhile, if system succeed to resume back with pm_trace set, the
> users are warned to adjust the bogus rtc either by ntp-date or rdate,
> by resetting pm_trace_rtc_abused to false, otherwise above tools might
> not work as expected.
> 
> Originally-from: Thomas Gleixner <t...@linutronix.de>
> Cc: "Rafael J. Wysocki" <r...@rjwysocki.net>
> Cc: John Stultz <john.stu...@linaro.org>
> Cc: Xunlei Pang <xlp...@redhat.com>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: Len Brown <l...@kernel.org>
> Cc: "H. Peter Anvin" <h...@zytor.com>
> Cc: Pavel Machek <pa...@ucw.cz>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Signed-off-by: Chen Yu <yu.c.c...@intel.com>


Acked-by: Thomas Gleixner <t...@linutronix.de>

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 2/2] time: alarmtimer: Add the trcepoints for alarmtimer

[Thomas Gleixner]

On Wed, 21 Sep 2016, Steven Rostedt wrote:
> On Wed, 21 Sep 2016 09:26:20 +0200 (CEST)
> Thomas Gleixner <t...@linutronix.de> wrote:
> > A single u64 does not take more storage space than this and it's a single
> > store.
> 
> So to use rtc_tm_to_time64()? Is the work to do the calculations of the
> conversion faster than a bunch of stores that are going to be in hot
> cache?

Look at the call site. It has already the scalar nsec value and it does a
conversion to rtc time in order to trace it.

Ditto for the other tracepoints where the conversion from scalar nsec is
done in the tracepoint itself.

Thanks,

tglx

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 2/2] time: alarmtimer: Add the trcepoints for alarmtimer

[Thomas Gleixner]

On Wed, 21 Sep 2016, Baolin Wang wrote:
> On 21 September 2016 at 06:27, Thomas Gleixner <t...@linutronix.de> wrote:
> >> + TP_fast_assign(
> >> + __entry->second = rtc_time->tm_sec;
> >> + __entry->minute = rtc_time->tm_min;
> >> + __entry->hour = rtc_time->tm_hour;
> >> + __entry->day = rtc_time->tm_mday;
> >> + __entry->mon = rtc_time->tm_mon;
> >> + __entry->year = rtc_time->tm_year;
> >> + __entry->alarm_type = flag;
> >
> > What's the value of storing the alarm time in RTC format?
> 
> As suggested by Steven, change the type of RTC value to save trace buffer.

A single u64 does not take more storage space than this and it's a single
store.
 
> > 2) You store the expiry time again in RTC format. Store the information in
> >a plain u64 and be done with it.
> 
> But I still think the RTC format is more readable for debugging alarm timer.

That's what post processing is for.
 
> > What's the point of this conditional? Avoiding rtc_ktime_to_tm() ? Oh 
> > well...
> >
> >> + tm_set = rtc_ktime_to_tm(now);
> >> + trace_alarmtimer_suspend(_set, type);
> >
> > "now" is CLOCK_REALTIME based. You store the type of the alarm timer which
> > is the first to expire and therefor is the one setting the RTC value, but
> > we don't know which timer it is. Useful - NOT!
> 
> We can know the timer by comparing the expire time.

Please make it similar to the timer/hrtimer tracing so people can reuse
their postprocessing scripts with minimial tweaks.

Thanks,

tglx

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 2/2] time: alarmtimer: Add the trcepoints for alarmtimer

[Thomas Gleixner]

On Sun, 18 Sep 2016, Baolin Wang wrote:
> +DECLARE_EVENT_CLASS(alarm_setting,

What is alarm_setting? Without looking at the DEFINE_EVENT which uses this
I cannot decode the meaning.

> + TP_STRUCT__entry(
> + __field(unsigned char, second)
> + __field(unsigned char, minute)
> + __field(unsigned char, hour)
> + __field(unsigned char, day)
> + __field(unsigned char, mon)
> + __field(unsigned short, year)
> + __field(unsigned char, alarm_type)
> + ),
> +
> + TP_fast_assign(
> + __entry->second = rtc_time->tm_sec;
> + __entry->minute = rtc_time->tm_min;
> + __entry->hour = rtc_time->tm_hour;
> + __entry->day = rtc_time->tm_mday;
> + __entry->mon = rtc_time->tm_mon;
> + __entry->year = rtc_time->tm_year;
> + __entry->alarm_type = flag;

What's the value of storing the alarm time in RTC format?

What's wrong with simply storing the flat u64 based wall clock time?
Nothing, because you can do the RTC format conversion in user space.

> +DECLARE_EVENT_CLASS(alarm_processing,

Again alarm_processing is not telling me anything. 

> +
> + TP_PROTO(struct alarm *alarm, char *process_name),

Why do you want to store process_name? The tracer already tracks the name
of the process in which context the tracepoint is taken. So what's the
point of this? Look at the output:

system_server-2976  [003] d..2  1076.605608: alarmtimer_start: 
process:system_server

Completely pointless duplicated information.

> +
> + TP_ARGS(alarm, process_name),
> +
> + TP_STRUCT__entry(
> + __field(unsigned long long, expires)
> + __field(unsigned char, second)
> + __field(unsigned char, minute)
> + __field(unsigned char, hour)
> + __field(unsigned char, day)
> + __field(unsigned char, mon)
> + __field(unsigned short, year)
> + __field(unsigned char, alarm_type)
> + __string(name, process_name)
> + ),
> +
> + TP_fast_assign(
> + __entry->expires = alarm->node.expires.tv64;
> + __entry->alarm_type = alarm->type;
> + __assign_str(name, process_name);
> + __entry->second = rtc_ktime_to_tm(alarm->node.expires).tm_sec;
> + __entry->minute = rtc_ktime_to_tm(alarm->node.expires).tm_min;
> + __entry->hour = rtc_ktime_to_tm(alarm->node.expires).tm_hour;
> + __entry->day = rtc_ktime_to_tm(alarm->node.expires).tm_mday;
> + __entry->mon = rtc_ktime_to_tm(alarm->node.expires).tm_mon;
> + __entry->year = rtc_ktime_to_tm(alarm->node.expires).tm_year;

This is utter crap for various reasons:

1) You store the expiry time over and over and in most cases it's simply
   pointless.

   - If the timer is started then we want to store the expiry time.

   - If the timer fires then the programmed expiry time is available from
 the start trace point and you miss to store the information which is
 really interesting: The actual time at which the timer expires
 (REAL/BOOT)

   - If the timer is canceled then the expiry time in the timer is not
 interesting at all. All you care is about the fact that the timer has
 been canceled. The expiry time can still be retrieved from the start
 trace point.

   - The restart tracepoint is redundant as well because either you see:

 start -> expire -> start or start -> start which is clearly a restart.

 If you put the start trace point into alarmtimer_enqueue() then you
 spare the extra code size for two tracepoints because that is used in
 start and restart

   See the hrtimer and timer tracepoints for reference. 


2) You store the expiry time again in RTC format. Store the information in
   a plain u64 and be done with it.


> +DEFINE_EVENT(alarm_processing, alarmtimer_fired,
> +
> + TP_PROTO(struct alarm *alarm, char *process_name),

So you hand in a NULL pointer to this tracepoint to have even more useless
information in the trace.

> @@ -264,6 +270,11 @@ static int alarmtimer_suspend(struct device *dev)
>   now = rtc_tm_to_ktime(tm);
>   now = ktime_add(now, min);
>  
> + if (trace_alarmtimer_suspend_enabled()) {

What's the point of this conditional? Avoiding rtc_ktime_to_tm() ? Oh well...

> + tm_set = rtc_ktime_to_tm(now);
> + trace_alarmtimer_suspend(_set, type);

"now" is CLOCK_REALTIME based. You store the type of the alarm timer which
is the first to expire and therefor is the one setting the RTC value, but
we don't know which timer it is. Useful - NOT!

Thanks,

tglx




-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this 

[rtc-linux] Re: [PATCH] rtc/rtc-cmos: Initialize hpet timer before irq is registered

[Thomas Gleixner]

On Thu, 15 Sep 2016, Pratyush Anand wrote:
> 
> Suggested-by: Thomas Gleixner <t...@linutronix.de>
> Signed-off-by: Pratyush Anand <pan...@redhat.com>

Acked-by: Thomas Gleixner <t...@linutronix.de>

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH V3 2/2] rtc/rtc-cmos: Initialize software counters before irq is registered

[Thomas Gleixner]

On Tue, 16 Aug 2016, Pratyush Anand wrote:

That's a lot of churn to fix that simple problem. The two liner below
should fix that as well, right?

Thanks,

tglx

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 43745cac0141..cb8dfc3ee012 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -707,6 +707,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, 
int rtc_irq)
goto cleanup1;
}
 
+   hpet_rtc_timer_init();
if (is_valid_irq(rtc_irq)) {
irq_handler_t rtc_cmos_int_handler;
 
@@ -714,6 +715,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, 
int rtc_irq)
rtc_cmos_int_handler = hpet_rtc_interrupt;
retval = hpet_register_irq_handler(cmos_interrupt);
if (retval) {
+   hpet_mask_rtc_irq_bit(RTC_IRQMASK);
dev_warn(dev, "hpet_register_irq_handler "
" failed in rtc_init().");
goto cleanup1;
@@ -729,7 +731,6 @@ cmos_do_probe(struct device *dev, struct resource *ports, 
int rtc_irq)
goto cleanup1;
}
}
-   hpet_rtc_timer_init();
 
/* export at least the first block of NVRAM */
nvram.size = address_space - NVRAM_OFFSET;


-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups 
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH RFC 1/2] rtc/hpet: Factorize hpet_rtc_timer_init()

[Thomas Gleixner]

On Tue, 21 Jun 2016, Pratyush Anand wrote:

> This patch factorize hpet_rtc_timer_init(), so that counter can be
> initialized before irq is registered.

This changelog is useless. It tells what the patch does, but not WHY this is
required.
 
Thanks,

tglx