| Message ID | 20190926022820.7900-3-jacob.e.keller@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | new PTP ioctl fixes | expand |
On Wed, Sep 25, 2019 at 07:28:20PM -0700, Jacob Keller wrote: > This patch may not be correct for individual drivers, especially > regarding the rising vs falling edge flags. I interpreted the default > behavior to be to timestamp the rising edge of a pin transition. So I think this patch goes too far. It breaks the implied ABI. > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c > index fd3071f55bd3..2867a2581a36 100644 > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c > @@ -521,6 +521,10 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp, > > switch (rq->type) { > case PTP_CLK_REQ_EXTTS: > + /* Reject requests with unsupported flags */ > + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE)) > + return -EOPNOTSUPP; This HW always time stamps both edges, and that is not configurable. Here you reject PTP_FALLING_EDGE, and that is clearly wrong. If the driver had been really picky (my fault I guess), it should have always insisted on (PTP_RISING_EDGE | PTP_FALLING_EDGE) being set together. But it is too late to enforce that now, because it could break user space programs. I do agree with the sentiment of checking the flags at the driver level, but this needs to be done case by case, with the drivers' author's input. (The req.perout.flags can be done unconditionally in all drivers, since there were never any valid flags, but req.extts.flags needs careful attention.) Thanks, Richard
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > Behalf Of Richard Cochran > Sent: Wednesday, September 25, 2019 9:02 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>; Felipe Balbi > <felipe.balbi@linux.intel.com>; David S . Miller <davem@davemloft.net>; Hall, > Christopher S <christopher.s.hall@intel.com> > Subject: Re: [net-next v2 2/2] net: reject ptp requests with unsupported flags > > On Wed, Sep 25, 2019 at 07:28:20PM -0700, Jacob Keller wrote: > > This patch may not be correct for individual drivers, especially > > regarding the rising vs falling edge flags. I interpreted the default > > behavior to be to timestamp the rising edge of a pin transition. > > So I think this patch goes too far. It breaks the implied ABI. Sure, I didn't really know whether the rising vs falling edge and how it was supposed to work for each driver. I just want to ensure that any future flags get rejected until they are actually supported. > > > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c > b/drivers/net/ethernet/intel/igb/igb_ptp.c > > index fd3071f55bd3..2867a2581a36 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c > > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c > > @@ -521,6 +521,10 @@ static int igb_ptp_feature_enable_i210(struct > ptp_clock_info *ptp, > > > > switch (rq->type) { > > case PTP_CLK_REQ_EXTTS: > > + /* Reject requests with unsupported flags */ > > + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE)) > > + return -EOPNOTSUPP; > > This HW always time stamps both edges, and that is not configurable. > Here you reject PTP_FALLING_EDGE, and that is clearly wrong. If the > driver had been really picky (my fault I guess), it should have always > insisted on (PTP_RISING_EDGE | PTP_FALLING_EDGE) being set together. > But it is too late to enforce that now, because it could break user > space programs. Yes. > > I do agree with the sentiment of checking the flags at the driver > level, but this needs to be done case by case, with the drivers' > author's input. Sure. I think the best immediate approach is to make sure all drivers reject any *new* flags, and each driver can decide whether they should reject rising, falling, etc. > > (The req.perout.flags can be done unconditionally in all drivers, > since there were never any valid flags, but req.extts.flags needs > careful attention.) > Right. > Thanks, > Richard
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > Behalf Of Richard Cochran > Sent: Wednesday, September 25, 2019 9:02 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>; Felipe Balbi > <felipe.balbi@linux.intel.com>; David S . Miller <davem@davemloft.net>; Hall, > Christopher S <christopher.s.hall@intel.com> > Subject: Re: [net-next v2 2/2] net: reject ptp requests with unsupported flags > > On Wed, Sep 25, 2019 at 07:28:20PM -0700, Jacob Keller wrote: > > This patch may not be correct for individual drivers, especially > > regarding the rising vs falling edge flags. I interpreted the default > > behavior to be to timestamp the rising edge of a pin transition. > > So I think this patch goes too far. It breaks the implied ABI. > > > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c > b/drivers/net/ethernet/intel/igb/igb_ptp.c > > index fd3071f55bd3..2867a2581a36 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c > > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c > > @@ -521,6 +521,10 @@ static int igb_ptp_feature_enable_i210(struct > ptp_clock_info *ptp, > > > > switch (rq->type) { > > case PTP_CLK_REQ_EXTTS: > > + /* Reject requests with unsupported flags */ > > + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE)) > > + return -EOPNOTSUPP; > > This HW always time stamps both edges, and that is not configurable. > Here you reject PTP_FALLING_EDGE, and that is clearly wrong. If the > driver had been really picky (my fault I guess), it should have always > insisted on (PTP_RISING_EDGE | PTP_FALLING_EDGE) being set together. > But it is too late to enforce that now, because it could break user > space programs. > > I do agree with the sentiment of checking the flags at the driver > level, but this needs to be done case by case, with the drivers' > author's input. > > (The req.perout.flags can be done unconditionally in all drivers, > since there were never any valid flags, but req.extts.flags needs > careful attention.) > > Thanks, > Richard I will split this patch apart and make the extts part per-driver. Thanks, Jake
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c index 073cbd0bb91b..2364b6b67a7b 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.c +++ b/drivers/net/dsa/mv88e6xxx/ptp.c @@ -273,6 +273,11 @@ 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)) + return -EOPNOTSUPP; + pin = ptp_find_pin(chip->ptp_clock, PTP_PF_EXTTS, rq->extts.index); if (pin < 0) diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 77f3511b97de..ca3aa1250dd1 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -6280,6 +6280,10 @@ static int tg3_ptp_enable(struct ptp_clock_info *ptp, switch (rq->type) { case PTP_CLK_REQ_PEROUT: + /* Reject requests with unsupported flags */ + if (rq->perout.flags) + return -EOPNOTSUPP; + if (rq->perout.index != 0) return -EINVAL; diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index fd3071f55bd3..2867a2581a36 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -521,6 +521,10 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp, switch (rq->type) { case PTP_CLK_REQ_EXTTS: + /* Reject requests with unsupported flags */ + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | PTP_RISING_EDGE)) + return -EOPNOTSUPP; + if (on) { pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS, rq->extts.index); @@ -551,6 +555,10 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp, return 0; case PTP_CLK_REQ_PEROUT: + /* Reject requests with unsupported flags */ + if (rq->perout.flags) + return -EOPNOTSUPP; + if (on) { pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT, rq->perout.index); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c index 0059b290e095..9a40f24e3193 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c @@ -236,6 +236,12 @@ static int mlx5_extts_configure(struct ptp_clock_info *ptp, if (!MLX5_PPS_CAP(mdev)) return -EOPNOTSUPP; + /* Reject requests with unsupported flags */ + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | + PTP_RISING_EDGE | + PTP_FALLING_EDGE)) + return -EOPNOTSUPP; + if (rq->extts.index >= clock->ptp_info.n_pins) return -EINVAL; @@ -290,6 +296,10 @@ static int mlx5_perout_configure(struct ptp_clock_info *ptp, if (!MLX5_PPS_CAP(mdev)) return -EOPNOTSUPP; + /* Reject requests with unsupported flags */ + if (rq->perout.flags) + return -EOPNOTSUPP; + if (rq->perout.index >= clock->ptp_info.n_pins) return -EINVAL; diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c index 57b26c2acf87..e8fe9a90fe4f 100644 --- a/drivers/net/ethernet/microchip/lan743x_ptp.c +++ b/drivers/net/ethernet/microchip/lan743x_ptp.c @@ -429,6 +429,10 @@ static int lan743x_ptp_perout(struct lan743x_adapter *adapter, int on, int pulse_width = 0; int perout_bit = 0; + /* Reject requests with unsupported flags */ + if (perout->flags) + return -EOPNOTSUPP; + if (!on) { lan743x_ptp_perout_off(adapter); return 0; diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c index 9a42580693cb..fe66697aafec 100644 --- a/drivers/net/ethernet/renesas/ravb_ptp.c +++ b/drivers/net/ethernet/renesas/ravb_ptp.c @@ -182,6 +182,11 @@ static int ravb_ptp_extts(struct ptp_clock_info *ptp, struct net_device *ndev = priv->ndev; unsigned long flags; + /* Reject requests with unsupported flags */ + if (req->flags & ~(PTP_ENABLE_FEATURE | + PTP_RISING_EDGE)) + return -EOPNOTSUPP; + if (req->index) return -EINVAL; @@ -211,6 +216,10 @@ static int ravb_ptp_perout(struct ptp_clock_info *ptp, unsigned long flags; int error = 0; + /* Reject requests with unsupported flags */ + if (req->flags) + return -EOPNOTSUPP; + if (req->index) return -EINVAL; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c index 173493db038c..352dc4c68625 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c @@ -140,6 +140,10 @@ static int stmmac_enable(struct ptp_clock_info *ptp, switch (rq->type) { case PTP_CLK_REQ_PEROUT: + /* Reject requests with unsupported flags */ + if (rq->perout.flags) + return -EOPNOTSUPP; + cfg = &priv->pps[rq->perout.index]; cfg->start.tv_sec = rq->perout.start.sec; diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c index 6580094161a9..2781b0e2d947 100644 --- a/drivers/net/phy/dp83640.c +++ b/drivers/net/phy/dp83640.c @@ -469,6 +469,11 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp, switch (rq->type) { case PTP_CLK_REQ_EXTTS: + /* Reject requests with unsupported flags */ + if (rq->extts.flags & ~(PTP_ENABLE_FEATURE | + PTP_RISING_EDGE | + PTP_FALLING_EDGE)) + return -EOPNOTSUPP; index = rq->extts.index; if (index >= N_EXT_TS) return -EINVAL; @@ -491,6 +496,9 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp, return 0; case PTP_CLK_REQ_PEROUT: + /* Reject requests with unsupported flags */ + if (rq->perout.flags) + return -EOPNOTSUPP; if (rq->perout.index >= N_PER_OUT) return -EINVAL; return periodic_output(clock, rq, on, rq->perout.index);
Fix all of the drivers which implement support for the periodic output or external timestamp requests to reject unsupported flags. This is important for forward compatibility: if a new flag is introduced, the driver should reject requests to enable the flag until it has been modified to actually support the flag in question. This patch may not be correct for individual drivers, especially regarding the rising vs falling edge flags. I interpreted the default behavior to be to timestamp the rising edge of a pin transition. Cc: Richard Cochran <richardcochran@gmail.com> Cc: Felipe Balbi <felipe.balbi@linux.intel.com> Cc: David S. Miller <davem@davemloft.net> Cc: Christopher Hall <christopher.s.hall@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/dsa/mv88e6xxx/ptp.c | 5 +++++ drivers/net/ethernet/broadcom/tg3.c | 4 ++++ drivers/net/ethernet/intel/igb/igb_ptp.c | 8 ++++++++ drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c | 10 ++++++++++ drivers/net/ethernet/microchip/lan743x_ptp.c | 4 ++++ drivers/net/ethernet/renesas/ravb_ptp.c | 9 +++++++++ drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 4 ++++ drivers/net/phy/dp83640.c | 8 ++++++++ 8 files changed, 52 insertions(+)