| 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
| ||
|
Date: Sun, 2 Jul 2023 15:20:39 +0300
From: "Farber, Eliav" <farbere@...zon.com>
To: Rodolfo Giometti <giometti@...eenne.com>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <ronenk@...zon.com>, <talel@...zon.com>, <hhhawa@...zon.com>,
<jonnyc@...zon.com>, <itamark@...zon.com>, <shellykz@...zon.com>,
<amitlavi@...zon.com>, <almogbs@...zon.com>, <farbere@...zon.com>
Subject: Re: [PATCH 1/5] pps: add pulse-width calculation in nsec
On 6/27/2023 5:27 PM, Rodolfo Giometti wrote:
> On 25/06/23 16:21, Eliav Farber wrote:
>> This change adds PPS pulse-width calculation in nano seconds.
>> Width time can be calculated for both assert time and reset time.
>>
>> Calculation can be done only if capture ASSERT and capture CLEAR modes
>> are both enabled.
>>
>> Assert width is calculated as:
>> clear-time - assert-time
>> and clear width is calculated as:
>> assert-time - clear-time
>>
>> Read-only sysfs were added to get the last pulse-width time and event
>> sequence.
>> Examples:
>> * cat /sys/class/pps/pps0/pulse_width_assert
>> 20001450#85
>> * cat /sys/class/pps/pps1/pulse_width_clear
>> 979893314#16
>>
>> Signed-off-by: Eliav Farber <farbere@...zon.com>
>> ---
>> drivers/pps/kapi.c | 49 ++++++++++++++++++++++++++++++++++++++
>> drivers/pps/pps.c | 9 +++++++
>> drivers/pps/sysfs.c | 30 +++++++++++++++++++++++
>> include/linux/pps_kernel.h | 3 +++
>> include/uapi/linux/pps.h | 19 +++++++++++++++
>> 5 files changed, 110 insertions(+)
>>
>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>> index d9d566f70ed1..deeecfc0a3ee 100644
>> --- a/drivers/pps/kapi.c
>> +++ b/drivers/pps/kapi.c
>> @@ -82,6 +82,14 @@ struct pps_device *pps_register_source(struct
>> pps_source_info *info,
>> goto pps_register_source_exit;
>> }
>>
>> + if ((info->mode & PPS_WIDTHBOTH) &&
>> + ((info->mode & PPS_CAPTUREBOTH) != PPS_CAPTUREBOTH)) {
>> + pr_err("%s: width can't be calculated without both
>> captures (mode = 0x%x)\n",
>> + info->name, info->mode);
>> + err = -EINVAL;
>> + goto pps_register_source_exit;
>> + }
>
> See the comment below where you define PPS_WIDTHBOTH.
>
>> /* Allocate memory for the new PPS source struct */
>> pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
>> if (pps == NULL) {
>> @@ -143,6 +151,39 @@ void pps_unregister_source(struct pps_device *pps)
>> }
>> EXPORT_SYMBOL(pps_unregister_source);
>>
>> +static u64 pps_ktime_sub(struct pps_ktime *ts1, struct pps_ktime *ts2)
>> +{
>> + if (ts1->sec == ts2->sec)
>> + return (ts1->nsec > ts2->nsec) ? (ts1->nsec -
>> ts2->nsec) : (ts2->nsec - ts1->nsec);
>> +
>> + if (ts1->sec > ts2->sec)
>> + return (ts1->sec - ts2->sec) * NSEC_PER_SEC + ts1->nsec
>> - ts2->nsec;
>> +
>> + return (ts2->sec - ts1->sec) * NSEC_PER_SEC + ts2->nsec -
>> ts1->nsec;
>> +}
>> +
>> +static void pps_calc_clear_width(struct pps_device *pps)
>> +{
>> + if (pps->clear_sequence == 0)
>> + return;
>> +
>> + pps->clear_width.sequence++;
>
> I don't understand the meaning of this field... regarding assert and
> clear it
> states the n-th sample but in this case...? Why do you need it?
For assert and clear, the sequence parameter is basically the counter
of assert/clear events.
Similarly, I wanted to have a counter for the number of pulses which
there width was counted.
The sequence was used by me in the sysfs to show the pulse counter and
pulse width in nano-seconds.
Will counter make more sense instead of sequence?
Initially, I used the assert_sequence and clear_sequence as the pulse
counter, but there were few cases to handle.
In case first interrupt happened during a pulse, then
assert_sequence != clear_sequence, but if not then
assert_sequence == clear_sequence.
So I preferred to add an new independent value.
>> + pps->clear_width.nsec = pps_ktime_sub(&pps->assert_tu,
>> &pps->clear_tu);
>> + dev_dbg(pps->dev, "PPS clear width = %llu#%u\n",
>> + pps->clear_width.nsec, pps->clear_width.sequence);
>> +}
>> +
>> +static void pps_calc_assert_width(struct pps_device *pps)
>> +{
>> + if (pps->assert_sequence == 0)
>> + return;
>> +
>> + pps->assert_width.sequence++;
>
> Ditto.
>
>> + pps->assert_width.nsec = pps_ktime_sub(&pps->clear_tu,
>> &pps->assert_tu);
>> + dev_dbg(pps->dev, "PPS assert width = %llu#%u\n",
>> + pps->assert_width.nsec, pps->assert_width.sequence);
>> +}
>> +
>> /* pps_event - register a PPS event into the system
>> * @pps: the PPS device
>> * @ts: the event timestamp
>> @@ -191,6 +232,10 @@ void pps_event(struct pps_device *pps, struct
>> pps_event_time *ts, int event,
>> dev_dbg(pps->dev, "capture assert seq #%u\n",
>> pps->assert_sequence);
>>
>> + /* Calculate clear pulse-width */
>> + if (pps->params.mode & PPS_WIDTHCLEAR)
>> + pps_calc_clear_width(pps);
>> +
>> captured = ~0;
>> }
>> if (event & pps->params.mode & PPS_CAPTURECLEAR) {
>> @@ -205,6 +250,10 @@ void pps_event(struct pps_device *pps, struct
>> pps_event_time *ts, int event,
>> dev_dbg(pps->dev, "capture clear seq #%u\n",
>> pps->clear_sequence);
>>
>> + /* Calculate assert pulse-width */
>> + if (pps->params.mode & PPS_WIDTHASSERT)
>> + pps_calc_assert_width(pps);
>> +
>> captured = ~0;
>> }
>>
>> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
>> index 5d19baae6a38..8299a272af11 100644
>> --- a/drivers/pps/pps.c
>> +++ b/drivers/pps/pps.c
>> @@ -195,6 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
>> fdata.info.clear_tu = pps->clear_tu;
>> fdata.info.current_mode = pps->current_mode;
>>
>> + memcpy(&fdata.info.assert_width, &pps->assert_width,
>> + sizeof(struct pps_kwidth));
>> + memcpy(&fdata.info.clear_width, &pps->clear_width,
>> + sizeof(struct pps_kwidth));
>> +
>> spin_unlock_irq(&pps->lock);
>>
>> err = copy_to_user(uarg, &fdata, sizeof(struct
>> pps_fdata));
>> @@ -283,6 +288,10 @@ static long pps_cdev_compat_ioctl(struct file
>> *file,
>> sizeof(struct pps_ktime_compat));
>> memcpy(&compat.info.clear_tu, &pps->clear_tu,
>> sizeof(struct pps_ktime_compat));
>> + memcpy(&compat.info.assert_width, &pps->assert_width,
>> + sizeof(struct pps_kwidth_compat));
>> + memcpy(&compat.info.clear_width, &pps->clear_width,
>> + sizeof(struct pps_kwidth_compat));
>>
>> spin_unlock_irq(&pps->lock);
>>
>> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
>> index 134bc33f6ad0..3e34de77dba6 100644
>> --- a/drivers/pps/sysfs.c
>> +++ b/drivers/pps/sysfs.c
>> @@ -79,6 +79,34 @@ static ssize_t path_show(struct device *dev,
>> struct device_attribute *attr,
>> }
>> static DEVICE_ATTR_RO(path);
>>
>> +static ssize_t pulse_width_assert_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pps_device *pps = dev_get_drvdata(dev);
>> +
>> + if (!(pps->info.mode & PPS_WIDTHASSERT))
>> + return 0;
>> +
>> + return sprintf(buf, "%llu#%u\n",
>> + pps->assert_width.nsec,
>> pps->assert_width.sequence);
>> +}
>> +static DEVICE_ATTR_RO(pulse_width_assert);
>> +
>> +static ssize_t pulse_width_clear_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct pps_device *pps = dev_get_drvdata(dev);
>> +
>> + if (!(pps->info.mode & PPS_WIDTHCLEAR))
>> + return 0;
>> +
>> + return sprintf(buf, "%llu#%u\n",
>> + pps->clear_width.nsec, pps->clear_width.sequence);
>> +}
>> +static DEVICE_ATTR_RO(pulse_width_clear);
>> +
>> static struct attribute *pps_attrs[] = {
>> &dev_attr_assert.attr,
>> &dev_attr_clear.attr,
>> @@ -86,6 +114,8 @@ static struct attribute *pps_attrs[] = {
>> &dev_attr_echo.attr,
>> &dev_attr_name.attr,
>> &dev_attr_path.attr,
>> + &dev_attr_pulse_width_assert.attr,
>> + &dev_attr_pulse_width_clear.attr,
>> NULL,
>> };
>>
>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>> index 78c8ac4951b5..15f2338095c6 100644
>> --- a/include/linux/pps_kernel.h
>> +++ b/include/linux/pps_kernel.h
>> @@ -51,6 +51,9 @@ struct pps_device {
>> struct pps_ktime clear_tu;
>> int current_mode; /* PPS mode at event
>> time */
>>
>> + struct pps_kwidth assert_width; /* PPS assert
>> pulse-width time and event seq # */
>> + struct pps_kwidth clear_width; /* PPS clear
>> pulse-width time and event seq # */
>> +
>> unsigned int last_ev; /* last PPS event id */
>> wait_queue_head_t queue; /* PPS event queue */
>>
>> diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
>> index 009ebcd8ced5..dd93dac0afc1 100644
>> --- a/include/uapi/linux/pps.h
>> +++ b/include/uapi/linux/pps.h
>> @@ -64,12 +64,24 @@ struct pps_ktime_compat {
>> } __attribute__((packed, aligned(4)));
>> #define PPS_TIME_INVALID (1<<0) /* used to specify
>> timeout==NULL */
>>
>> +struct pps_kwidth {
>> + __u64 nsec;
>> + __u32 sequence;
>> +};
>> +
>> +struct pps_kwidth_compat {
>> + __u64 nsec;
>> + __u32 sequence;
>> +} __attribute__((packed, aligned(4)));
>
> Why do you need a new type? Since both assert_width and clear_width
> are time
> quantities as far as assert_tu and clear_tu, they can be of the same
> type, can't
> they? Or, at least they can simply be __u64 since having an
> assert_width or
> clear_width longer than 1 second is a non-sense...
For each pulse I wanted to save width in nsec (without sec) and
counter.
I need it twice for both assert and clear, hence I added a new
structure for it.
>> struct pps_kinfo {
>> __u32 assert_sequence; /* seq. num. of assert event */
>> __u32 clear_sequence; /* seq. num. of clear event */
>> struct pps_ktime assert_tu; /* time of assert event */
>> struct pps_ktime clear_tu; /* time of clear event */
>> int current_mode; /* current mode bits */
>> + struct pps_kwidth assert_width; /* assert pulse-width time and
>> seq. num. */
>> + struct pps_kwidth clear_width; /* clear pulse-width time and
>> seq. num. */
>> };
>
> Altering this structure may break userspace code... also rfc2783 at
> section-3.2
> states that:
>
> The API defines these new data structures:
>
> typedef struct {
> pps_seq_t assert_sequence; /* assert event seq # */
> pps_seq_t clear_sequence; /* clear event seq # */
> pps_timeu_t assert_tu;
> pps_timeu_t clear_tu;
> int current_mode; /* current mode bits */
> } pps_info_t;
>
> So, I'm not willing to change this structure just to add this new data
> that I
> don't even know where it's used...
>
> If you just read these information via sysfs, please drop these part.
ACK. I'll drop this part.
>> struct pps_kinfo_compat {
>> @@ -78,6 +90,8 @@ struct pps_kinfo_compat {
>> struct pps_ktime_compat assert_tu; /* time of assert event */
>> struct pps_ktime_compat clear_tu; /* time of clear event */
>> int current_mode; /* current mode bits */
>> + struct pps_kwidth_compat assert_width; /* assert pulse-width
>> time and seq. num. */
>> + struct pps_kwidth_compat clear_width; /* clear pulse-width
>> time and seq. num. */
>> };
>>
>> struct pps_kparams {
>> @@ -96,6 +110,11 @@ struct pps_kparams {
>> #define PPS_CAPTURECLEAR 0x02 /* capture clear events */
>> #define PPS_CAPTUREBOTH 0x03 /* capture assert and
>> clear events */
>>
>> +/* Pulse-width calculation */
>> +#define PPS_WIDTHASSERT 0x04 /* calculate assert
>> width */
>> +#define PPS_WIDTHCLEAR 0x08 /* calculate clear
>> width */
>> +#define PPS_WIDTHBOTH 0x0c /* calculate assert and
>> clear width */
>> +
>
> I don't understand why a process should ask for just PPS_WIDTHASSERT or
> PPS_WIDTHCLEAR... I think you can avoid defining these values and just
> enabling
> pulse width calculation when both assert and clear events are available.
ACK. I'll drop the new defines and enable width calculation when
PPS_CAPTUREASSERT and PPS_CAPTURECLEAR are both defined.
>> #define PPS_OFFSETASSERT 0x10 /* apply compensation for
>> assert event */
>> #define PPS_OFFSETCLEAR 0x20 /* apply compensation
>> for clear event */
>
> However, the real point is: since an userpsace program can retrieve
> the time of
> assert and clear events, why it cannot compute the pulses width by
> itself? :)
The userpsace program can retrieve the time of assert and clear events,
but it is not always clear how to compute it.
Initially that was how I did it:
Read both times, make sure sequence of both times was identical, and
then compute: clear_time – assert_time.
But as I mentioned, when using wide pulses, it might be that when
driver starts, it is a the middle of a pulse.
In that case clear_time will be captured first (seq #1).
Then assert_time is captured (seq #1).
However, assert pulse width can only be calculated for the second
clear-time sequence and first assert-time sequence.
So to simplify this for the user, I added the calculation to the
driver.
Hope this was clear.
Please let me know if this satisfies you, and then I’ll share a second
version of patches which fixes all the other comments you gave.
---
Regards, Eliav
Powered by blists - more mailing lists