* [PATCH net-next] ptp: only allow phase values lower than 1 period
@ 2020-08-03 19:49 Vladimir Oltean
2020-08-04 1:18 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Vladimir Oltean @ 2020-08-03 19:49 UTC (permalink / raw)
To: kuba, davem, netdev; +Cc: richardcochran, jacob.e.keller
The way we define the phase (the difference between the time of the
signal's rising edge, and the closest integer multiple of the period),
it doesn't make sense to have a phase value larger than 1 period.
So deny these settings coming from the user.
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
drivers/ptp/ptp_chardev.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index e0e6f85966e1..02fcd5e8b998 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -218,6 +218,19 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
break;
}
}
+ if (perout->flags & PTP_PEROUT_PHASE) {
+ /*
+ * The phase should be specified modulo the
+ * period, therefore anything larger than 1
+ * period is invalid.
+ */
+ if (perout->phase.sec > perout->period.sec ||
+ (perout->phase.sec == perout->period.sec &&
+ perout->phase.nsec > perout->period.nsec)) {
+ err = -ERANGE;
+ break;
+ }
+ }
} else if (cmd == PTP_PEROUT_REQUEST) {
req.perout.flags &= PTP_PEROUT_V1_VALID_FLAGS;
req.perout.rsv[0] = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ptp: only allow phase values lower than 1 period
2020-08-03 19:49 [PATCH net-next] ptp: only allow phase values lower than 1 period Vladimir Oltean
@ 2020-08-04 1:18 ` David Miller
2020-08-04 23:04 ` David Miller
2020-08-04 23:23 ` Richard Cochran
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2020-08-04 1:18 UTC (permalink / raw)
To: olteanv; +Cc: kuba, netdev, richardcochran, jacob.e.keller
From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon, 3 Aug 2020 22:49:21 +0300
> The way we define the phase (the difference between the time of the
> signal's rising edge, and the closest integer multiple of the period),
> it doesn't make sense to have a phase value larger than 1 period.
>
> So deny these settings coming from the user.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Richard, please review.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ptp: only allow phase values lower than 1 period
2020-08-03 19:49 [PATCH net-next] ptp: only allow phase values lower than 1 period Vladimir Oltean
2020-08-04 1:18 ` David Miller
@ 2020-08-04 23:04 ` David Miller
2020-08-04 23:23 ` Richard Cochran
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2020-08-04 23:04 UTC (permalink / raw)
To: olteanv; +Cc: kuba, netdev, richardcochran, jacob.e.keller
From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon, 3 Aug 2020 22:49:21 +0300
> The way we define the phase (the difference between the time of the
> signal's rising edge, and the closest integer multiple of the period),
> it doesn't make sense to have a phase value larger than 1 period.
>
> So deny these settings coming from the user.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Richard, I need your review on this patch.
Thank you.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ptp: only allow phase values lower than 1 period
2020-08-03 19:49 [PATCH net-next] ptp: only allow phase values lower than 1 period Vladimir Oltean
2020-08-04 1:18 ` David Miller
2020-08-04 23:04 ` David Miller
@ 2020-08-04 23:23 ` Richard Cochran
2020-08-04 23:40 ` Vladimir Oltean
2 siblings, 1 reply; 5+ messages in thread
From: Richard Cochran @ 2020-08-04 23:23 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: kuba, davem, netdev, jacob.e.keller
On Mon, Aug 03, 2020 at 10:49:21PM +0300, Vladimir Oltean wrote:
> @@ -218,6 +218,19 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> break;
> }
> }
> + if (perout->flags & PTP_PEROUT_PHASE) {
> + /*
> + * The phase should be specified modulo the
> + * period, therefore anything larger than 1
> + * period is invalid.
> + */
> + if (perout->phase.sec > perout->period.sec ||
> + (perout->phase.sec == perout->period.sec &&
> + perout->phase.nsec > perout->period.nsec)) {
> + err = -ERANGE;
> + break;
> + }
So if perout->period={1,0} and perout->phase={1,0} then the phase has
wrapped 360 degrees back to zero.
Shouldn't this code catch that case as well?
So why not test for (perout->phase.nsec >= perout->period.nsec) instead?
Thanks,
Richard
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ptp: only allow phase values lower than 1 period
2020-08-04 23:23 ` Richard Cochran
@ 2020-08-04 23:40 ` Vladimir Oltean
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Oltean @ 2020-08-04 23:40 UTC (permalink / raw)
To: Richard Cochran; +Cc: kuba, davem, netdev, jacob.e.keller
On Tue, Aug 04, 2020 at 04:23:35PM -0700, Richard Cochran wrote:
> On Mon, Aug 03, 2020 at 10:49:21PM +0300, Vladimir Oltean wrote:
> > @@ -218,6 +218,19 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> > break;
> > }
> > }
> > + if (perout->flags & PTP_PEROUT_PHASE) {
> > + /*
> > + * The phase should be specified modulo the
> > + * period, therefore anything larger than 1
> > + * period is invalid.
> > + */
> > + if (perout->phase.sec > perout->period.sec ||
> > + (perout->phase.sec == perout->period.sec &&
> > + perout->phase.nsec > perout->period.nsec)) {
> > + err = -ERANGE;
> > + break;
> > + }
>
> So if perout->period={1,0} and perout->phase={1,0} then the phase has
> wrapped 360 degrees back to zero.
>
> Shouldn't this code catch that case as well?
>
> So why not test for (perout->phase.nsec >= perout->period.nsec) instead?
>
> Thanks,
> Richard
Oof, I guess this is what one would call 'brain fart'. In my mind,
checking for equality between period and phase required an extra 'if',
which I was reluctant to add (or to even think about, it seems). I
converted the nsec check to >= and it works as it should (I checked with
a modified ts2phc).
ts2phc[326.764]: config item /dev/ptp1.ts2phc.perout_phase is 1000000000
ts2phc[326.764]: config item /dev/ptp1.ts2phc.pulsewidth is 500000000
ts2phc[326.764]: PTP_PEROUT_REQUEST2 failed: Numerical result out of range
I'm sending a v2 with your change very soon.
Thanks.
-Vladimir
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-04 23:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-03 19:49 [PATCH net-next] ptp: only allow phase values lower than 1 period Vladimir Oltean
2020-08-04 1:18 ` David Miller
2020-08-04 23:04 ` David Miller
2020-08-04 23:23 ` Richard Cochran
2020-08-04 23:40 ` Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.