| Message ID | 20250310-jk-net-fixes-supported-extts-flags-v1-1-854ffb5f3a96@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ptp: fix egregious supported flag checks | expand |
On Mon, Mar 10, 2025 at 03:16:36PM -0700, Jacob Keller wrote: > The igb_ptp_feature_enable_82580 function correctly checks that unknown > flags are not passed to the function. However, it does not actually check > PTP_RISING_EDGE or PTP_FALLING_EDGE when configuring the external timestamp > function. > > The data sheet for the 82580 product says: > > Upon a change in the input level of one of the SDP pins that was > configured to detect Time stamp events using the TSSDP register, a time > stamp of the system time is captured into one of the two auxiliary time > stamp registers (AUXSTMPL/H0 or AUXSTMPL/H1). > > For example to define timestamping of events in the AUXSTMPL0 and > AUXSTMPH0 registers, Software should: > > 1. Set the TSSDP.AUX0_SDP_SEL field to select the SDP pin that detects > the level change and set the TSSDP.AUX0_TS_SDP_EN bit to 1. > > 2. Set the TSAUXC.EN_TS0 bit to 1 to enable timestamping > > The same paragraph is in the i350 and i354 data sheets. > > The wording implies that the time stamps are captured at any level change. > There does not appear to be any way to only timestamp one edge of the > signal. > > Reject requests which do not set both PTP_RISING_EDGE and PTP_FALLING_EDGE > when operating under PTP_STRICT_FLAGS mode via PTP_EXTTS_REQUEST2. > > Fixes: 38970eac41db ("igb: support EXTTS on 82580/i354/i350") > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/igb/igb_ptp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c > index f9457055612004c10f74379122063e8136fe7d76..b89ef4538a18d7ca11325ddc15944a878f4d807e 100644 > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c > @@ -509,6 +509,11 @@ static int igb_ptp_feature_enable_82580(struct ptp_clock_info *ptp, > PTP_STRICT_FLAGS)) > return -EOPNOTSUPP; > > + /* Both the rising and falling edge are timstamped */ > + if (rq->extts.flags & PTP_STRICT_FLAGS && > + (rq->extts.flags & PTP_EXTTS_EDGES) != PTP_EXTTS_EDGES) > + return -EOPNOTSUPP; > + > if (on) { > pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS, > rq->extts.index); Thanks for fixing Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> In igb_ptp_feature_enable_i210() there is the same check for both edges but also PTP_ENABLE_FEATURE is tested. There is no need for it here, or it is redundant even in i210? > > -- > 2.48.1.397.gec9d649cc640
> -----Original Message----- > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Sent: Monday, March 10, 2025 11:04 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>; David > S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; > Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard > Cochran <richardcochran@gmail.com>; Ruud Bos <kernel.hbk@gmail.com>; Paul > Barker <paul.barker.ct@bp.renesas.com>; Niklas Söderlund > <niklas.soderlund@ragnatech.se>; Bryan Whitehead > <bryan.whitehead@microchip.com>; UNGLinuxDriver@microchip.com; Raju > Lakkaraju <Raju.Lakkaraju@microchip.com>; Florian Fainelli > <florian.fainelli@broadcom.com>; Broadcom internal kernel review list <bcm- > kernel-feedback-list@broadcom.com>; Andrew Lunn <andrew@lunn.ch>; Heiner > Kallweit <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; > Jonathan Lemon <jonathan.lemon@gmail.com>; Lasse Johnsen > <l@ssejohnsen.me>; Vadim Fedorenko <vadim.fedorenko@linux.dev>; intel- > wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-renesas- > soc@vger.kernel.org > Subject: Re: [PATCH net 1/5] igb: reject invalid external timestamp requests for > 82580-based HW > > On Mon, Mar 10, 2025 at 03:16:36PM -0700, Jacob Keller wrote: > > The igb_ptp_feature_enable_82580 function correctly checks that unknown > > flags are not passed to the function. However, it does not actually check > > PTP_RISING_EDGE or PTP_FALLING_EDGE when configuring the external > timestamp > > function. > > > > The data sheet for the 82580 product says: > > > > Upon a change in the input level of one of the SDP pins that was > > configured to detect Time stamp events using the TSSDP register, a time > > stamp of the system time is captured into one of the two auxiliary time > > stamp registers (AUXSTMPL/H0 or AUXSTMPL/H1). > > > > For example to define timestamping of events in the AUXSTMPL0 and > > AUXSTMPH0 registers, Software should: > > > > 1. Set the TSSDP.AUX0_SDP_SEL field to select the SDP pin that detects > > the level change and set the TSSDP.AUX0_TS_SDP_EN bit to 1. > > > > 2. Set the TSAUXC.EN_TS0 bit to 1 to enable timestamping > > > > The same paragraph is in the i350 and i354 data sheets. > > > > The wording implies that the time stamps are captured at any level change. > > There does not appear to be any way to only timestamp one edge of the > > signal. > > > > Reject requests which do not set both PTP_RISING_EDGE and > PTP_FALLING_EDGE > > when operating under PTP_STRICT_FLAGS mode via PTP_EXTTS_REQUEST2. > > > > Fixes: 38970eac41db ("igb: support EXTTS on 82580/i354/i350") > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > --- > > drivers/net/ethernet/intel/igb/igb_ptp.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c > b/drivers/net/ethernet/intel/igb/igb_ptp.c > > index > f9457055612004c10f74379122063e8136fe7d76..b89ef4538a18d7ca11325ddc1594 > 4a878f4d807e 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c > > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c > > @@ -509,6 +509,11 @@ static int igb_ptp_feature_enable_82580(struct > ptp_clock_info *ptp, > > PTP_STRICT_FLAGS)) > > return -EOPNOTSUPP; > > > > + /* Both the rising and falling edge are timstamped */ > > + if (rq->extts.flags & PTP_STRICT_FLAGS && > > + (rq->extts.flags & PTP_EXTTS_EDGES) != PTP_EXTTS_EDGES) > > + return -EOPNOTSUPP; > > + > > if (on) { > > pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS, > > rq->extts.index); > > Thanks for fixing > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > In igb_ptp_feature_enable_i210() there is the same check for both edges > but also PTP_ENABLE_FEATURE is tested. There is no need for it here, or > it is redundant even in i210? > Hmm. It might be required, because requests to disable the clock won't have PTP_FEATURE_ENABLED set, and might have the edges cleared, which would prevent you from disabling the output..? I'll have to see what the kernel does when it disables the function.
> -----Original Message----- > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Sent: Monday, March 10, 2025 11:04 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>; David > S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard Cochran > <richardcochran@gmail.com>; Ruud Bos <kernel.hbk@gmail.com>; Paul Barker > <paul.barker.ct@bp.renesas.com>; Niklas Söderlund > <niklas.soderlund@ragnatech.se>; Bryan Whitehead > <bryan.whitehead@microchip.com>; UNGLinuxDriver@microchip.com; Raju > Lakkaraju <Raju.Lakkaraju@microchip.com>; Florian Fainelli > <florian.fainelli@broadcom.com>; Broadcom internal kernel review list <bcm- > kernel-feedback-list@broadcom.com>; Andrew Lunn <andrew@lunn.ch>; Heiner > Kallweit <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Jonathan > Lemon <jonathan.lemon@gmail.com>; Lasse Johnsen <l@ssejohnsen.me>; Vadim > Fedorenko <vadim.fedorenko@linux.dev>; intel-wired-lan@lists.osuosl.org; > netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org > Subject: Re: [PATCH net 1/5] igb: reject invalid external timestamp requests for > 82580-based HW > > On Mon, Mar 10, 2025 at 03:16:36PM -0700, Jacob Keller wrote: > > The igb_ptp_feature_enable_82580 function correctly checks that unknown > > flags are not passed to the function. However, it does not actually check > > PTP_RISING_EDGE or PTP_FALLING_EDGE when configuring the external > timestamp > > function. > > > > The data sheet for the 82580 product says: > > > > Upon a change in the input level of one of the SDP pins that was > > configured to detect Time stamp events using the TSSDP register, a time > > stamp of the system time is captured into one of the two auxiliary time > > stamp registers (AUXSTMPL/H0 or AUXSTMPL/H1). > > > > For example to define timestamping of events in the AUXSTMPL0 and > > AUXSTMPH0 registers, Software should: > > > > 1. Set the TSSDP.AUX0_SDP_SEL field to select the SDP pin that detects > > the level change and set the TSSDP.AUX0_TS_SDP_EN bit to 1. > > > > 2. Set the TSAUXC.EN_TS0 bit to 1 to enable timestamping > > > > The same paragraph is in the i350 and i354 data sheets. > > > > The wording implies that the time stamps are captured at any level change. > > There does not appear to be any way to only timestamp one edge of the > > signal. > > > > Reject requests which do not set both PTP_RISING_EDGE and > PTP_FALLING_EDGE > > when operating under PTP_STRICT_FLAGS mode via PTP_EXTTS_REQUEST2. > > > > Fixes: 38970eac41db ("igb: support EXTTS on 82580/i354/i350") > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > --- > > drivers/net/ethernet/intel/igb/igb_ptp.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c > b/drivers/net/ethernet/intel/igb/igb_ptp.c > > index > f9457055612004c10f74379122063e8136fe7d76..b89ef4538a18d7ca11325ddc > 15944a878f4d807e 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c > > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c > > @@ -509,6 +509,11 @@ static int igb_ptp_feature_enable_82580(struct > ptp_clock_info *ptp, > > PTP_STRICT_FLAGS)) > > return -EOPNOTSUPP; > > > > + /* Both the rising and falling edge are timstamped */ > > + if (rq->extts.flags & PTP_STRICT_FLAGS && > > + (rq->extts.flags & PTP_EXTTS_EDGES) != PTP_EXTTS_EDGES) > > + return -EOPNOTSUPP; > > + > > if (on) { > > pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS, > > rq->extts.index); > > Thanks for fixing > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > In igb_ptp_feature_enable_i210() there is the same check for both edges > but also PTP_ENABLE_FEATURE is tested. There is no need for it here, or > it is redundant even in i210? This needs a v2 with the flag check modified. Will fix, thanks for spotting it! > > > > > -- > > 2.48.1.397.gec9d649cc640
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index f9457055612004c10f74379122063e8136fe7d76..b89ef4538a18d7ca11325ddc15944a878f4d807e 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -509,6 +509,11 @@ static int igb_ptp_feature_enable_82580(struct ptp_clock_info *ptp, PTP_STRICT_FLAGS)) return -EOPNOTSUPP; + /* Both the rising and falling edge are timstamped */ + if (rq->extts.flags & PTP_STRICT_FLAGS && + (rq->extts.flags & PTP_EXTTS_EDGES) != PTP_EXTTS_EDGES) + return -EOPNOTSUPP; + if (on) { pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS, rq->extts.index);
The igb_ptp_feature_enable_82580 function correctly checks that unknown flags are not passed to the function. However, it does not actually check PTP_RISING_EDGE or PTP_FALLING_EDGE when configuring the external timestamp function. The data sheet for the 82580 product says: Upon a change in the input level of one of the SDP pins that was configured to detect Time stamp events using the TSSDP register, a time stamp of the system time is captured into one of the two auxiliary time stamp registers (AUXSTMPL/H0 or AUXSTMPL/H1). For example to define timestamping of events in the AUXSTMPL0 and AUXSTMPH0 registers, Software should: 1. Set the TSSDP.AUX0_SDP_SEL field to select the SDP pin that detects the level change and set the TSSDP.AUX0_TS_SDP_EN bit to 1. 2. Set the TSAUXC.EN_TS0 bit to 1 to enable timestamping The same paragraph is in the i350 and i354 data sheets. The wording implies that the time stamps are captured at any level change. There does not appear to be any way to only timestamp one edge of the signal. Reject requests which do not set both PTP_RISING_EDGE and PTP_FALLING_EDGE when operating under PTP_STRICT_FLAGS mode via PTP_EXTTS_REQUEST2. Fixes: 38970eac41db ("igb: support EXTTS on 82580/i354/i350") Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/igb/igb_ptp.c | 5 +++++ 1 file changed, 5 insertions(+)