[PATCH net 0/5] net: ptp: fix egregious supported flag checks

[PATCH net 0/5] net: ptp: fix egregious supported flag checks

[Jacob Keller]

In preparation for adding .supported_extts_flags and
.supported_perout_flags to the ptp_clock_info structure, fix a couple of
places where drivers get existing flag gets grossly incorrect.

The igb driver claims 82580 supports strictly validating PTP_RISING_EDGE
and PTP_FALLING_EDGE, but doesn't actually check the flags. Fix the driver
to require that the request match both edges, as this is implied by the
datasheet description.

The renesas driver also claims to support strict flag checking, but does
not actually check the flags either. I do not have the data sheet for this
device, so I do not know what edge it timestamps. For simplicity, just
reject all requests with PTP_STRICT_FLAGS. This essentially prevents the
PTP_EXTTS_REQUEST2 ioctl from working. Updating to correctly validate the
flags will require someone who has the hardware to confirm the behavior.

The lan743x driver supports (and strictly validates) that the request is
either PTP_RISING_EDGE or PTP_FALLING_EDGE but not both. However, it does
not check the flags are one of the known valid flags. Thus, requests for
PTP_EXT_OFF (and any future flag) will be accepted and misinterpreted. Add
the appropriate check to reject unsupported PTP_EXT_OFF requests and future
proof against new flags.

The broadcom PHY driver checks that PTP_PEROUT_PHASE is not set. This
appears to be an attempt at rejecting unsupported flags. It is not robust
against flag additions such as the PTP_PEROUT_ONE_SHOT, or anything added
in the future. Fix this by instead checking against the negation of the
supported PTP_PEROUT_DUTY_CYCLE instead.

The ptp_ocp driver supports PTP_PEROUT_PHASE and PTP_PEROUT_DUTY_CYCLE, but
does not check unsupported flags. Add the appropriate check to ensure
PTP_PEROUT_ONE_SHOT and any future flags are rejected as unsupported.

These are changes compile-tested, but I do not have hardware to validate the
behavior.

There are a number of other drivers which enable periodic output or
external timestamp requests, but which do not check flags at all. We could
go through each of these drivers one-by-one and meticulously add a flag
check. Instead, these drivers will be covered only by the upcoming
.supported_extts_flags and .supported_perout_flags checks in a net-next
series.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Jacob Keller (5):
      igb: reject invalid external timestamp requests for 82580-based HW
      renesas: reject PTP_STRICT_FLAGS as unsupported
      net: lan743x: reject unsupported external timestamp requests
      broadcom: fix supported flag check in periodic output function
      ptp: ocp: reject unsupported periodic output flags

 drivers/net/ethernet/intel/igb/igb_ptp.c     | 5 +++++
 drivers/net/ethernet/microchip/lan743x_ptp.c | 6 ++++++
 drivers/net/ethernet/renesas/ravb_ptp.c      | 3 +--
 drivers/net/phy/bcm-phy-ptp.c                | 3 ++-
 drivers/ptp/ptp_ocp.c                        | 4 ++++
 5 files changed, 18 insertions(+), 3 deletions(-)
---
base-commit: 992ee3ed6e9fdd0be83a7daa5ff738e3cf86047f
change-id: 20250310-jk-net-fixes-supported-extts-flags-e8434d8e2552

Best regards,
-- 
Jacob Keller <jacob.e.keller@intel.com>

[PATCH net 1/5] igb: reject invalid external timestamp requests for 82580-based HW

[Jacob Keller]

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);

-- 
2.48.1.397.gec9d649cc640

[PATCH net 2/5] renesas: reject PTP_STRICT_FLAGS as unsupported

[Jacob Keller]

The ravb_ptp_extts() function checks the flags coming from the
PTP_EXTTS_REQUEST ioctl, to ensure that future flags are not accepted on
accident.

This was updated to 'honor' the PTP_STRICT_FLAGS in commit 6138e687c7b6
("ptp: Introduce strict checking of external time stamp options.").
However, the driver does not *actually* validate the flags.

I originally fixed this driver to reject future flags in commit
592025a03b34 ("renesas: reject unsupported external timestamp flags"). It
is still unclear whether this hardware timestamps the rising, falling, or
both edges of the input signal.

Accepting requests with PTP_STRICT_FLAGS is a bug, as this could lead to
users mistakenly assuming a request with PTP_RISING_EDGE actually
timestamps the rising edge only.

Reject requests with PTP_STRICT_FLAGS (and hence all PTP_EXTTS_REQUEST2
requests) until someone with access to the datasheet or hardware knowledge
can confirm the timestamping behavior and update this driver.

Fixes: 6138e687c7b6 ("ptp: Introduce strict checking of external time stamp options.")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/renesas/ravb_ptp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index 6e4ef7af27bf31ab2aad8e06a65e0ede6046e3c0..b4365906669f3bd40953813e263aeaafd2e1eb70 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -179,8 +179,7 @@ static int ravb_ptp_extts(struct ptp_clock_info *ptp,
 	/* Reject requests with unsupported flags */
 	if (req->flags & ~(PTP_ENABLE_FEATURE |
 			   PTP_RISING_EDGE |
-			   PTP_FALLING_EDGE |
-			   PTP_STRICT_FLAGS))
+			   PTP_FALLING_EDGE))
 		return -EOPNOTSUPP;
 
 	if (req->index)

-- 
2.48.1.397.gec9d649cc640

[PATCH net 3/5] net: lan743x: reject unsupported external timestamp requests

[Jacob Keller]

The lan743x_ptp_io_event_cap_en() function checks that the given request
sets only one of PTP_RISING_EDGE or PTP_FALLING_EDGE, but not both.

However, this driver does not check whether other flags (such as
PTP_EXT_OFF) are set, nor whether any future unrecognized flags are set.

Fix this by adding the appropriate check to the lan743x_ptp_io_extts()
function.

Fixes: 60942c397af6 ("net: lan743x: Add support for PTP-IO Event Input External Timestamp (extts)")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/microchip/lan743x_ptp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c
index 4a777b449ecd03ac0d7576f8570f1a7794fb3d06..0be44dcb339387e9756f36f909f65c20870bc49b 100644
--- a/drivers/net/ethernet/microchip/lan743x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan743x_ptp.c
@@ -942,6 +942,12 @@ static int lan743x_ptp_io_extts(struct lan743x_adapter *adapter, int on,
 
 	extts = &ptp->extts[index];
 
+	if (extts_request->flags & ~(PTP_ENABLE_FEATURE |
+				     PTP_RISING_EDGE |
+				     PTP_FALLING_EDGE |
+				     PTP_STRICT_FLAGS))
+		return -EOPNOTSUPP;
+
 	if (on) {
 		extts_pin = ptp_find_pin(ptp->ptp_clock, PTP_PF_EXTTS, index);
 		if (extts_pin < 0)

-- 
2.48.1.397.gec9d649cc640

[PATCH net 4/5] broadcom: fix supported flag check in periodic output function

[Jacob Keller]

In bcm_ptp_perout_locked, the driver rejects requests which have
PTP_PEROUT_PHASE set. This appears to be an attempt to reject any
unsupported flags. Unfortunately, this only checks one flag, but does not
protect against PTP_PEROUT_ONE_SHOT, or any future flags which may be
added.

Fix the check to ensure that no flag other than the supported
PTP_PEROUT_DUTY_CYCLE is set.

Fixes: 7bfe91efd525 ("net: phy: Add support for 1PPS out and external timestamps")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/phy/bcm-phy-ptp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
index 208e8f561e0696e64bd5e842b66d88c65d70bfc0..eba8b5fb1365f4e43331e479e8e2f3c4b590ab96 100644
--- a/drivers/net/phy/bcm-phy-ptp.c
+++ b/drivers/net/phy/bcm-phy-ptp.c
@@ -597,7 +597,8 @@ static int bcm_ptp_perout_locked(struct bcm_ptp_private *priv,
 
 	period = BCM_MAX_PERIOD_8NS;	/* write nonzero value */
 
-	if (req->flags & PTP_PEROUT_PHASE)
+	/* Reject unsupported flags */
+	if (req->flags & ~PTP_PEROUT_DUTY_CYCLE)
 		return -EOPNOTSUPP;
 
 	if (req->flags & PTP_PEROUT_DUTY_CYCLE)

-- 
2.48.1.397.gec9d649cc640

[PATCH net 5/5] ptp: ocp: reject unsupported periodic output flags

[Jacob Keller]

The ptp_ocp_signal_from_perout() function supports PTP_PEROUT_DUTY_CYCLE
and PTP_PEROUT_PHASE. It does not support PTP_PEROUT_ONE_SHOT, but does not
reject a request with such an unsupported flag.

Add the appropriate check to ensure that unsupported requests are rejected
both for PTP_PEROUT_ONE_SHOT as well as any future flags.

Fixes: 1aa66a3a135a ("ptp: ocp: Program the signal generators via PTP_CLK_REQ_PEROUT")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/ptp/ptp_ocp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index b651087f426f50a73229ca57634fc5d6912e0a87..4a87af0980d695a9ab1b23e2544f620759ccb892 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -2090,6 +2090,10 @@ ptp_ocp_signal_from_perout(struct ptp_ocp *bp, int gen,
 {
 	struct ptp_ocp_signal s = { };
 
+	if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE |
+			   PTP_PEROUT_PHASE))
+		return -EOPNOTSUPP;
+
 	s.polarity = bp->signal[gen].polarity;
 	s.period = ktime_set(req->period.sec, req->period.nsec);
 	if (!s.period)

-- 
2.48.1.397.gec9d649cc640

Re: [PATCH net 5/5] ptp: ocp: reject unsupported periodic output flags

[Vadim Fedorenko]

On 10/03/2025 22:16, Jacob Keller wrote:
> The ptp_ocp_signal_from_perout() function supports PTP_PEROUT_DUTY_CYCLE
> and PTP_PEROUT_PHASE. It does not support PTP_PEROUT_ONE_SHOT, but does not
> reject a request with such an unsupported flag.
> 
> Add the appropriate check to ensure that unsupported requests are rejected
> both for PTP_PEROUT_ONE_SHOT as well as any future flags.
> 
> Fixes: 1aa66a3a135a ("ptp: ocp: Program the signal generators via PTP_CLK_REQ_PEROUT")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>   drivers/ptp/ptp_ocp.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index b651087f426f50a73229ca57634fc5d6912e0a87..4a87af0980d695a9ab1b23e2544f620759ccb892 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -2090,6 +2090,10 @@ ptp_ocp_signal_from_perout(struct ptp_ocp *bp, int gen,
>   {
>   	struct ptp_ocp_signal s = { };
>   
> +	if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE |
> +			   PTP_PEROUT_PHASE))
> +		return -EOPNOTSUPP;
> +
>   	s.polarity = bp->signal[gen].polarity;
>   	s.period = ktime_set(req->period.sec, req->period.nsec);
>   	if (!s.period)
> 

Thanks,
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH net 1/5] igb: reject invalid external timestamp requests for 82580-based HW

[Michal Swiatkowski]

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

RE: [PATCH net 1/5] igb: reject invalid external timestamp requests for 82580-based HW

[Keller, Jacob E]

> -----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.

RE: [PATCH net 1/5] igb: reject invalid external timestamp requests for 82580-based HW

[Keller, Jacob E]

> -----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

Re: [PATCH net 2/5] renesas: reject PTP_STRICT_FLAGS as unsupported

[Simon Horman]

+ Shimoda-san

On Mon, Mar 10, 2025 at 03:16:37PM -0700, Jacob Keller wrote:
> The ravb_ptp_extts() function checks the flags coming from the
> PTP_EXTTS_REQUEST ioctl, to ensure that future flags are not accepted on
> accident.
> 
> This was updated to 'honor' the PTP_STRICT_FLAGS in commit 6138e687c7b6
> ("ptp: Introduce strict checking of external time stamp options.").
> However, the driver does not *actually* validate the flags.
> 
> I originally fixed this driver to reject future flags in commit
> 592025a03b34 ("renesas: reject unsupported external timestamp flags"). It
> is still unclear whether this hardware timestamps the rising, falling, or
> both edges of the input signal.
> 
> Accepting requests with PTP_STRICT_FLAGS is a bug, as this could lead to
> users mistakenly assuming a request with PTP_RISING_EDGE actually
> timestamps the rising edge only.
> 
> Reject requests with PTP_STRICT_FLAGS (and hence all PTP_EXTTS_REQUEST2
> requests) until someone with access to the datasheet or hardware knowledge
> can confirm the timestamping behavior and update this driver.
> 
> Fixes: 6138e687c7b6 ("ptp: Introduce strict checking of external time stamp options.")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Adding Shimoda-san who may be able to help coordinate a review if
Niklas and Paul are unavailable for some reason.

> ---
>  drivers/net/ethernet/renesas/ravb_ptp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 6e4ef7af27bf31ab2aad8e06a65e0ede6046e3c0..b4365906669f3bd40953813e263aeaafd2e1eb70 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -179,8 +179,7 @@ static int ravb_ptp_extts(struct ptp_clock_info *ptp,
>  	/* Reject requests with unsupported flags */
>  	if (req->flags & ~(PTP_ENABLE_FEATURE |
>  			   PTP_RISING_EDGE |
> -			   PTP_FALLING_EDGE |
> -			   PTP_STRICT_FLAGS))
> +			   PTP_FALLING_EDGE))
>  		return -EOPNOTSUPP;
>  
>  	if (req->index)
> 
> -- 
> 2.48.1.397.gec9d649cc640
> 
> 

Re: [PATCH net 3/5] net: lan743x: reject unsupported external timestamp requests

[Simon Horman]

On Mon, Mar 10, 2025 at 03:16:38PM -0700, Jacob Keller wrote:
> The lan743x_ptp_io_event_cap_en() function checks that the given request
> sets only one of PTP_RISING_EDGE or PTP_FALLING_EDGE, but not both.
> 
> However, this driver does not check whether other flags (such as
> PTP_EXT_OFF) are set, nor whether any future unrecognized flags are set.
> 
> Fix this by adding the appropriate check to the lan743x_ptp_io_extts()
> function.
> 
> Fixes: 60942c397af6 ("net: lan743x: Add support for PTP-IO Event Input External Timestamp (extts)")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 4/5] broadcom: fix supported flag check in periodic output function

[Simon Horman]

On Mon, Mar 10, 2025 at 03:16:39PM -0700, Jacob Keller wrote:
> In bcm_ptp_perout_locked, the driver rejects requests which have
> PTP_PEROUT_PHASE set. This appears to be an attempt to reject any
> unsupported flags. Unfortunately, this only checks one flag, but does not
> protect against PTP_PEROUT_ONE_SHOT, or any future flags which may be
> added.
> 
> Fix the check to ensure that no flag other than the supported
> PTP_PEROUT_DUTY_CYCLE is set.
> 
> Fixes: 7bfe91efd525 ("net: phy: Add support for 1PPS out and external timestamps")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/5] renesas: reject PTP_STRICT_FLAGS as unsupported

[Simon Horman]

On Tue, Mar 18, 2025 at 01:59:47PM +0000, Simon Horman wrote:
> + Shimoda-san
> 
> On Mon, Mar 10, 2025 at 03:16:37PM -0700, Jacob Keller wrote:
> > The ravb_ptp_extts() function checks the flags coming from the
> > PTP_EXTTS_REQUEST ioctl, to ensure that future flags are not accepted on
> > accident.
> > 
> > This was updated to 'honor' the PTP_STRICT_FLAGS in commit 6138e687c7b6
> > ("ptp: Introduce strict checking of external time stamp options.").
> > However, the driver does not *actually* validate the flags.
> > 
> > I originally fixed this driver to reject future flags in commit
> > 592025a03b34 ("renesas: reject unsupported external timestamp flags"). It
> > is still unclear whether this hardware timestamps the rising, falling, or
> > both edges of the input signal.
> > 
> > Accepting requests with PTP_STRICT_FLAGS is a bug, as this could lead to
> > users mistakenly assuming a request with PTP_RISING_EDGE actually
> > timestamps the rising edge only.
> > 
> > Reject requests with PTP_STRICT_FLAGS (and hence all PTP_EXTTS_REQUEST2
> > requests) until someone with access to the datasheet or hardware knowledge
> > can confirm the timestamping behavior and update this driver.
> > 
> > Fixes: 6138e687c7b6 ("ptp: Introduce strict checking of external time stamp options.")
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Adding Shimoda-san who may be able to help coordinate a review if
> Niklas and Paul are unavailable for some reason.

Sorry for the noise. I now see that Niklas has reviewed v2.