Re: [PATCH v2] rtc: Add tracepoints for RTC system

[Baolin Wang]

Hi Alexandre,

On 13 December 2017 at 06:16, Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxxxxxxxxx> wrote:
> Hi Baolin,
>
> On 16/11/2017 at 13:59:28 +0800, Baolin Wang wrote:
>> @@ -779,6 +797,7 @@ static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
>>       }
>>
>>       timerqueue_add(&rtc->timerqueue, &timer->node);
>> +     trace_rtc_timer_enqueue(timer);
>
> This doesn't apply because of 74717b28cb32e1ad3c1042cafd76b264c8c0f68d.
> Can you rebase?

Sure.

>
>> diff --git a/include/trace/events/rtc.h b/include/trace/events/rtc.h
>> new file mode 100644
>> index 0000000..b5a4add
>> --- /dev/null
>> +++ b/include/trace/events/rtc.h
>> @@ -0,0 +1,220 @@
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM rtc
>> +
>> +#if !defined(_TRACE_RTC_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_RTC_H
>> +
>> +#include <linux/rtc.h>
>> +#include <linux/tracepoint.h>
>> +
>> +DECLARE_EVENT_CLASS(rtc_time_alarm_class,
>> +
>> +     TP_PROTO(struct rtc_time *tm, int err),
>> +
>> +     TP_ARGS(tm, err),
>> +
>> +     TP_STRUCT__entry(
>> +             __field(int, sec)
>> +             __field(int, min)
>> +             __field(int, hour)
>> +             __field(int, mday)
>> +             __field(int, mon)
>> +             __field(int, year)
>> +             __field(time64_t, secs)
>> +             __field(int, err)
>> +     ),
>> +
>> +     TP_fast_assign(
>> +             __entry->sec = tm->tm_sec;
>> +             __entry->min = tm->tm_min;
>> +             __entry->hour = tm->tm_hour;
>> +             __entry->mday = tm->tm_mday;
>> +             __entry->mon = tm->tm_mon;
>> +             __entry->year = tm->tm_year;
>> +             __entry->secs = rtc_tm_to_time64(tm);
>> +             __entry->err = err;
>> +     ),
>> +
>> +     TP_printk("%d-%02d-%02d %02d:%02d:%02d UTC (%lld) (%d)",
>> +               __entry->year + 1900, __entry->mon + 1, __entry->mday,
>> +               __entry->hour, __entry->min, __entry->sec, __entry->secs,
>> +               __entry->err
>> +     )
>> +);
>> +
>
> Also, I'm a bit concerned about having a struct rtc_time here. I think
> its goal is mainly to have a nice representation on the time but maybe

Yes.

> the best would be to make printk able to pretty print the time (some
> patches were proposed).

If I understood your point correctly, you did not like the format of
TP_printk() here, right? So how about if I remove the 'struct
rtc_time' and just pass one 'ktime_t' parameter? But it will be not
readable for user to trace the RTC time/alarm.

>
> How bad would that be to change it later? I didn't follow the whole
> tracepoint ABI issue closely.


-- 
Baolin.wang
Best Regards