| Message ID | 20200803194921.603151-1-olteanv@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] ptp: only allow phase values lower than 1 period | expand |
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.
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.
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
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
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;
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(+)