| Message ID | 20190926181109.4871-4-jacob.e.keller@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | new PTP ioctl fixes | expand |
On Thu, Sep 26, 2019 at 11:11:05AM -0700, Jacob Keller wrote: > Fix the mv88e6xxx PTP support to explicitly reject any future flags that > get added to the external timestamp request ioctl. > > In order to maintain currently functioning code, this patch accepts all > three current flags. This is because the PTP_RISING_EDGE and > PTP_FALLING_EDGE flags have unclear semantics For the record, the semantics are (or should be): flags Meaning ---------------------------------------------------- -------------------------- PTP_ENABLE_FEATURE invalid PTP_ENABLE_FEATURE|PTP_RISING_EDGE Time stamp rising edge PTP_ENABLE_FEATURE|PTP_FALLING_EDGE Time stamp falling edge PTP_ENABLE_FEATURE|PTP_RISING_EDGE|PTP_FALLING_EDGE Time stamp both edges > and each driver seems to > have interpreted them slightly differently. This driver has: flags Meaning ---------------------------------------------------- -------------------------- PTP_ENABLE_FEATURE Time stamp falling edge PTP_ENABLE_FEATURE|PTP_RISING_EDGE Time stamp rising edge PTP_ENABLE_FEATURE|PTP_FALLING_EDGE Time stamp falling edge PTP_ENABLE_FEATURE|PTP_RISING_EDGE|PTP_FALLING_EDGE Time stamp rising edge > Cc: Brandon Streiff <brandon.streiff@ni.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Reviewed-by: Richard Cochran <richardcochran@gmail.com>
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Saturday, October 12, 2019 11:24 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; > Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; Brandon Streiff > <brandon.streiff@ni.com> > Subject: Re: [net-next v3 3/7] mv88e6xxx: reject unsupported external > timestamp flags > > On Thu, Sep 26, 2019 at 11:11:05AM -0700, Jacob Keller wrote: > > Fix the mv88e6xxx PTP support to explicitly reject any future flags that > > get added to the external timestamp request ioctl. > > > > In order to maintain currently functioning code, this patch accepts all > > three current flags. This is because the PTP_RISING_EDGE and > > PTP_FALLING_EDGE flags have unclear semantics > > For the record, the semantics are (or should be): > > flags Meaning > ---------------------------------------------------- -------------------------- > PTP_ENABLE_FEATURE invalid > PTP_ENABLE_FEATURE|PTP_RISING_EDGE Time stamp rising edge > PTP_ENABLE_FEATURE|PTP_FALLING_EDGE Time stamp falling edge > PTP_ENABLE_FEATURE|PTP_RISING_EDGE|PTP_FALLING_EDGE Time stamp > both edges > > > and each driver seems to > > have interpreted them slightly differently. > > This driver has: > > flags Meaning > ---------------------------------------------------- -------------------------- > PTP_ENABLE_FEATURE Time stamp falling edge > PTP_ENABLE_FEATURE|PTP_RISING_EDGE Time stamp rising edge > PTP_ENABLE_FEATURE|PTP_FALLING_EDGE Time stamp falling edge > PTP_ENABLE_FEATURE|PTP_RISING_EDGE|PTP_FALLING_EDGE Time stamp > rising edge > > > Cc: Brandon Streiff <brandon.streiff@ni.com> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > Reviewed-by: Richard Cochran <richardcochran@gmail.com> Right, so in practice, unless it supports both edges, it should reject setting both RISING and FALLING together. Thanks, Jake
On Sat, Oct 12, 2019 at 07:36:31PM +0000, Keller, Jacob E wrote:
> Right, so in practice, unless it supports both edges, it should reject setting both RISING and FALLING together.
Enforcing that now *could* break existing user space, but I wonder
whether any programs would actually be affected.
Maybe we can add a STRICT flag than requests strict checking. If user
space uses the "2" ioctl, then we would add this flag before invoking
the driver callback.
Thanks,
Richard
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Saturday, October 12, 2019 4:27 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; > Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; Brandon Streiff > <brandon.streiff@ni.com> > Subject: Re: [net-next v3 3/7] mv88e6xxx: reject unsupported external > timestamp flags > > On Sat, Oct 12, 2019 at 07:36:31PM +0000, Keller, Jacob E wrote: > > Right, so in practice, unless it supports both edges, it should reject setting both > RISING and FALLING together. > > Enforcing that now *could* break existing user space, but I wonder > whether any programs would actually be affected. > > Maybe we can add a STRICT flag than requests strict checking. If user > space uses the "2" ioctl, then we would add this flag before invoking > the driver callback. > > Thanks, > Richard That could work. I don't know how much it's worth fixing it, but that would be the right way to fix it, I think. I think the strict flag should do the following: a) enforce that you must set at least one of the two edge flags (ensuring that a request to timestamp without one of the edges is rejected) b) drivers *must* honor the edge flags or exit with a rejection if they can't support it. (unlike without 'strict', which would be like today and driver defined). c) possibly: reject both flags at once since it doesn't really make sense to create a timestamp for each edge. At least, I think that makes it harder to actually use the timestamps since you need to be more careful to separate the two timestamps. I'm not 100% sure on (c) but (b) can only be implemented in drivers, since the stack wouldn't know which modes the driver supports. Thanks, Jake
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c index 073cbd0bb91b..076e622a64d6 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.c +++ b/drivers/net/dsa/mv88e6xxx/ptp.c @@ -273,6 +273,12 @@ static int mv88e6352_ptp_enable_extts(struct mv88e6xxx_chip *chip, int pin; int err; + /* Reject requests with unsupported flags */ + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | + PTP_RISING_EDGE | + PTP_FALLING_EDGE)) + return -EOPNOTSUPP; + pin = ptp_find_pin(chip->ptp_clock, PTP_PF_EXTTS, rq->extts.index); if (pin < 0)
Fix the mv88e6xxx PTP support to explicitly reject any future flags that get added to the external timestamp request ioctl. In order to maintain currently functioning code, this patch accepts all three current flags. This is because the PTP_RISING_EDGE and PTP_FALLING_EDGE flags have unclear semantics and each driver seems to have interpreted them slightly differently. Cc: Brandon Streiff <brandon.streiff@ni.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/dsa/mv88e6xxx/ptp.c | 6 ++++++ 1 file changed, 6 insertions(+)