[net-next,v3,3/7] mv88e6xxx: reject unsupported external timestamp flags
diff mbox series

Message ID 20190926181109.4871-4-jacob.e.keller@intel.com
State Accepted
Delegated to: Jeff Kirsher
Headers show
Series
  • new PTP ioctl fixes
Related show

Commit Message

Keller, Jacob E Sept. 26, 2019, 6:11 p.m. UTC
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(+)

Comments

Richard Cochran Oct. 12, 2019, 6:24 p.m. UTC | #1
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>
Keller, Jacob E Oct. 12, 2019, 7:36 p.m. UTC | #2
> -----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
Richard Cochran Oct. 12, 2019, 11:27 p.m. UTC | #3
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
Keller, Jacob E Oct. 14, 2019, 5:20 p.m. UTC | #4
> -----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

Patch
diff mbox series

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)