| 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
| ||
|
Message-ID: <62222431-2693-4130-03ea-e0fad5d272ce@enneenne.com>
Date: Wed, 15 Mar 2023 08:44:52 +0100
From: Rodolfo Giometti <giometti@...eenne.com>
To: Alexander Komrakov <alexander.komrakov@...adcom.com>,
Greg KH <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] pps: Add elapsed realtime timestamping
On 15/03/23 06:02, Greg KH wrote:
> On Tue, Mar 14, 2023 at 05:52:26PM -0700, Alexander Komrakov wrote:
>> Some applications like Android needs elapsed realtime timestamping
>> to PPS pulse for its clock management. Add sysfs node for this.
>
> What exact applications will use this?
You should also prove a little documentation about what a "realtime
timestamping" is and why we need it.
>> Signed-off-by: Alexander Komrakov <alexander.komrakov@...adcom.com>
>> ---
>> Changes in v3:
>> - Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()
>
> Why are you not using sysfs_emit()? That is the correct call to use.
>
>> - .gittconfg updated, clock_gettime removed, trailing whitespaces remvoved
>
> What .gitconfig?
>
>> - COONFIG_ELAPSED_REALTIME_PPS added to enable elapsed assert/clear sysfs
>
> "OO"? And as I say below, just use the config option as-is, do not
> create a new one without a Kconfig change.
I suppose we can just drop this CONFIG entry.
>> Documentation/ABI/testing/sysfs-pps | 27 ++++++++++++++++++
>> drivers/pps/Makefile | 4 +++
>> drivers/pps/kapi.c | 24 ++++++++++++++--
>> drivers/pps/sysfs.c | 44 +++++++++++++++++++++++++++--
>> include/linux/pps_kernel.h | 2 ++
>> 5 files changed, 96 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-pps b/Documentation/ABI/testing/sysfs-pps
>> index 25028c7bc37d..031ec89e1ed6 100644
>> --- a/Documentation/ABI/testing/sysfs-pps
>> +++ b/Documentation/ABI/testing/sysfs-pps
>> @@ -1,3 +1,30 @@
>> +What: /sys/class/pps/pps0/assert_elapsed
>> +Date: October 2021
>> +Contact: Alexander Komrakov <alexander.komrakov@...adcom.com>
>> +Description:
>> + The /sys/class/pps/ppsX/assert_elapsed file reports the
>> + elapsed real-time assert events and the elapsed
>> + real-time assert sequence number of the X-th source
>> + in the form:
>> +
>> + <secs>.<nsec>#<sequence>
>> +
>> + If the source has no elapsed real-time assert events
>> + the content of this file is empty.
>
> What is a "real-time assert event"?
Please add these information to Documentation/driver-api/pps.rst too (maybe add
a note a the end of the "SYSFS support" section.
>> +
>> +What: /sys/class/pps/ppsX/clear_elapsed
>> +Date: October 2021
>> +Contact: Alexander Komrakov <alexander.komrakov@...adcom.com>
>> +Description:
>> + The /sys/class/pps/ppsX/clear_elapsed file reports the elapsed
>> + real-time clear events and the elapsed real-time clear
>> + sequence number of the X-th source in the form:
>> +
>> + <secs>.<nsec>#<sequence>
>> +
>> + If the source has no elapsed real-time clear events the
>> + content of this file is empty.
>> +
>> What: /sys/class/pps/
>> Date: February 2008
>> Contact: Rodolfo Giometti <giometti@...ux.it>
>> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
>> index ceaf65cc1f1d..443501310445 100644
>> --- a/drivers/pps/Makefile
>> +++ b/drivers/pps/Makefile
>> @@ -8,4 +8,8 @@ pps_core-$(CONFIG_NTP_PPS) += kc.o
>> obj-$(CONFIG_PPS) := pps_core.o
>> obj-y += clients/ generators/
>>
>> +ifeq ($(CONFIG_ELAPSED_REALTIME_PPS),y)
>> +EXTRA_CFLAGS += -DENABLE_ELAPSED_REALTIME_PPS
>
> Why are you defining a new flag just for a config option? Why not just
> check that config option in the code instead? That removes a different
> flag that will be impossible to track down over time.
>
>> +endif
>> +
>> ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>> index d9d566f70ed1..69b432873ce7 100644
>> --- a/drivers/pps/kapi.c
>> +++ b/drivers/pps/kapi.c
>> @@ -23,6 +23,7 @@
>> /*
>> * Local functions
>> */
>> + #define NANOSEC_PER_SEC 1000000000 /* 10^9 */
>
> Why isn't this in units.h? (hint, it is already, use that one.)
>
>
>>
>> static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
>> {
>> @@ -162,11 +163,20 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>> unsigned long flags;
>> int captured = 0;
>> struct pps_ktime ts_real = { .sec = 0, .nsec = 0, .flags = 0 };
>> + struct pps_ktime ts_real_elapsed = { .sec = 0, .nsec = 0, .flags = 0 };
>> + struct timespec64 ts64 = { .tv_sec = 0, .tv_nsec = 0 };
>>
>> /* check event type */
>> BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
>
> You are willing to crash a system because of a tiny driver issue? That
> is not good, please fix up in a later patch.
@Greg: I suppose you address this comment to me... :) Do you prefere a WARN_ON()
here?
>> + /* Calculate the monotonic clock from the timespec clock and stores the result in pps_ktime format
>> + ktime_get_boottime_ts64() : because elapsed realtime includes time spent in sleep */
>> + ktime_get_boottime_ts64(&ts64);
>> + timespec_to_pps_ktime(&ts_real_elapsed,ts64);
>>
>> - dev_dbg(pps->dev, "PPS event at %lld.%09ld\n",
>> + dev_dbg(pps->dev, "PPS event (monotonic) at %lld.%09d\n",
>> + (s64)ts_real_elapsed.sec, ts_real_elapsed.nsec);
>> +
>> + dev_dbg(pps->dev, "PPS event (timestamp) at %lld.%09ld\n",
>> (s64)ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
>>
>> timespec_to_pps_ktime(&ts_real, ts->ts_real);
>> @@ -181,11 +191,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>> pps->current_mode = pps->params.mode;
>> if (event & pps->params.mode & PPS_CAPTUREASSERT) {
>> /* We have to add an offset? */
>> - if (pps->params.mode & PPS_OFFSETASSERT)
>> + if (pps->params.mode & PPS_OFFSETASSERT) {
>> + pps_add_offset(&ts_real_elapsed,
>> + &pps->params.assert_off_tu);
>> pps_add_offset(&ts_real,
>> &pps->params.assert_off_tu);
>> + }
>>
>> /* Save the time stamp */
>> + pps->assert_elapsed_tu = ts_real_elapsed;
>> pps->assert_tu = ts_real;
>> pps->assert_sequence++;
>> dev_dbg(pps->dev, "capture assert seq #%u\n",
>> @@ -195,11 +209,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>> }
>> if (event & pps->params.mode & PPS_CAPTURECLEAR) {
>> /* We have to add an offset? */
>> - if (pps->params.mode & PPS_OFFSETCLEAR)
>> + if (pps->params.mode & PPS_OFFSETCLEAR) {
>> + pps_add_offset(&ts_real_elapsed,
>> + &pps->params.clear_off_tu);
>> pps_add_offset(&ts_real,
>> &pps->params.clear_off_tu);
>> + }
>>
>> /* Save the time stamp */
>> + pps->clear_elapsed_tu = ts_real_elapsed;
>> pps->clear_tu = ts_real;
>> pps->clear_sequence++;
>> dev_dbg(pps->dev, "capture clear seq #%u\n",
>> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
>> index 134bc33f6ad0..9e100e287ba7 100644
>> --- a/drivers/pps/sysfs.c
>> +++ b/drivers/pps/sysfs.c
>> @@ -10,6 +10,7 @@
>> #include <linux/module.h>
>> #include <linux/string.h>
>> #include <linux/pps_kernel.h>
>> +#include <linux/sysfs.h>
>
> Why is this now needed and it wasn't before?
>
>>
>> /*
>> * Attribute functions
>> @@ -24,11 +25,28 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
>> return 0;
>>
>> return sprintf(buf, "%lld.%09d#%d\n",
>> - (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
>> - pps->assert_sequence);
>> + (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
>> + pps->assert_sequence);
>
> Why make this change?
>
>
>
>> }
>> static DEVICE_ATTR_RO(assert);
>>
>> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
>> +static ssize_t assert_elapsed_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct pps_device *pps = dev_get_drvdata(dev);
>> +
>> + if (!(pps->info.mode & PPS_CAPTUREASSERT))
>> + return 0;
>> +
>> + return sprintf(buf, "%lld.%09d#%d\n",
>
> sysfs files should be using sysfs_emit()
>
>> + (long long) pps->assert_elapsed_tu.sec,
>> + pps->assert_elapsed_tu.nsec,
>> + pps->assert_sequence);
>> +}
>> +static DEVICE_ATTR_RO(assert_elapsed);
>> +#endif
>> +
>> static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -43,6 +61,24 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
>> }
>> static DEVICE_ATTR_RO(clear);
>>
>> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
>> +static ssize_t clear_elapsed_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pps_device *pps = dev_get_drvdata(dev);
>> +
>> + if (!(pps->info.mode & PPS_CAPTURECLEAR))
>> + return 0;
>> +
>> + return sprintf(buf, "%lld.%09d#%d\n",
>> + (long long) pps->clear_elapsed_tu.sec,
>> + pps->clear_elapsed_tu.nsec,
>> + pps->clear_sequence);
>> +}
>> +static DEVICE_ATTR_RO(clear_elapsed);
>> +#endif
>> +
>> static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -82,6 +118,10 @@ static DEVICE_ATTR_RO(path);
>> static struct attribute *pps_attrs[] = {
>> &dev_attr_assert.attr,
>> &dev_attr_clear.attr,
>> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
>> + &dev_attr_assert_elapsed.attr,
>> + &dev_attr_clear_elapsed.attr,
>> +#endif
>
> Don't use #ifdef in a .c file, these attributes should only show up if
> you determine at runtime they should not be there.
>
> But why woudn't they just always be there all the time anyway? Why do
> you need a new config option?
>
>
>> &dev_attr_mode.attr,
>> &dev_attr_echo.attr,
>> &dev_attr_name.attr,
>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>> index 78c8ac4951b5..1fecaaf4c8b9 100644
>> --- a/include/linux/pps_kernel.h
>> +++ b/include/linux/pps_kernel.h
>> @@ -47,6 +47,8 @@ struct pps_device {
>>
>> __u32 assert_sequence; /* PPS assert event seq # */
>> __u32 clear_sequence; /* PPS clear event seq # */
>> + struct pps_ktime assert_elapsed_tu; /* PPS elapsed rt assert seq # */
>> + struct pps_ktime clear_elapsed_tu; /* PPS elapsed rt clear event seq */
>
> no tabs?
>
> thanks,
>
> greg k-h
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@...eenne.com
Linux Device Driver giometti@...ux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Powered by blists - more mailing lists