[PATCH net-next v3 00/10] ravb: Add gPTP support for Gen4

[PATCH net-next v3 00/10] ravb: Add gPTP support for Gen4

[Niklas Söderlund]

Hello,

This series is the second part cleaning up how PTP timer support is
implemented on R-Car Gen4. Currently there is partial support for it in
some of the Ethernet devices that can use it, but not all.

On Gen2 and Gen3 each RAVB instance have it's own private PTP clock as
part of the RAVB register space. For this reason the PTP functionally
was implemented directly in the RAVB driver. For Gen4 however there is a
system-wide PTP clock shared by all RAVB instances, and on some Gen4
platforms with other Ethernet devices.

The RAVB Gen4 driver currently advertise PTP support to user-space, but
the support is in fact completely broken. It tries to use RAVB private
PTP clock, which do not exist on Gen4.

Further more the PTP clock behaved slightly different on Gen2 and Gen3.
These decencies have been handled by adding platform specific flags
inside the driver.

The usage of these flags have grown organically and been extended all
over the driver as it gained new features. Adding a new third set of
flags for Gen4 would be messy and add to the confusion.

Therefore patches 1/10 thru 8/10 refactors the usage of the PTP flags
into optional callbacks and untangles the usage. This allows adding Gen4
support easy as it can just implement it's own set of Gen4 specific
callbacks without altering the driver logic.

Patch 9/10 is a small DT binding addition adding an optional phandle to
link a RAVB device to the external PTP clock. Ideally this property
should be mandatory, but for backward comp ability is is made optional.
If the phandle is not set, or the PTP clock not enabled, the RAVB driver
will continue to faction as before, but no longer advertise PTP support
to user space.

Finally patch 10/10 adds the Gen4 specific PTP callbacks which allows
the driver to use the external PTP clock.

For part one of this work see [1]. The two series are independent of
each other but both are needed before a third series liking the RAVB to
the PTP clock in the platforms device tree. I will hold posting the
third series until all dependencies are merged. For this reason there is
no user of the new renesas,gptp device tree property added in this
series.

The work is tested on both Gen3 and Gen4 R-Car devices (with [1] and
appropriate DTS). I do however not have access to any Gen2 device where
the RAVB IP is wired to a MAC, so the small rework of the flags for Gen2
to callbacks have only been compiled tested. If anybody have a RZ device
where RAVB is wired and uses the Gen2 method of starting/stopping the
PTP timer together with the DMAC please test this.

1.  https://lore.kernel.org/all/20260803095232.3527111-1-niklas.soderlund+renesas@ragnatech.se/

Niklas Söderlund (10):
  net: ethernet: ravb: Remove gPTP control from WoL setup and restore
  net: ethernet: ravb: Move programming of gPTP timer interval
  net: ethernet: ravb: Simplify gPTP start and stop
  net: ethernet: ravb: Remove redundant argument to ravb_ptp_init()
  net: ethernet: ravb: Propagate error from ptp_clock_register()
  net: ethernet: ravb: Replace gPTP flags with callbacks
  net: ethernet: ravb: Add callback for gPTP probe
  net: ethernet: ravb: Add callback for gPTP clock index
  dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4
  net: ethernet: ravb: Add gPTP support for Gen4

 .../bindings/net/renesas,etheravb.yaml        |  11 +
 drivers/net/ethernet/renesas/ravb.h           |  35 ++-
 drivers/net/ethernet/renesas/ravb_main.c      | 249 ++++++++++++------
 drivers/net/ethernet/renesas/ravb_ptp.c       |  36 +--
 4 files changed, 224 insertions(+), 107 deletions(-)

-- 
2.55.0

[PATCH net-next v3 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[Niklas Söderlund]

Since commit a6a85ba36fd0 ("net: ravb: Move PTP initialization in the
driver's ndo_open API for ccc_gac platorms") the gPTP clock (if
supported) is stopped and started by opening and closing the ndev.

This makes the special case to stop and start it when resuming from WoL
redundant. As the ndev will always be closed and re-opened when
suspending and resuming the system.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ea1c7e536791..36173a09e2fd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
 	/* Enable MagicPacket */
 	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
 
-	if (priv->info->ccc_gac)
-		ravb_ptp_stop(ndev);
-
 	return enable_irq_wake(priv->emac_irq);
 }
 
@@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
 	if (error)
 		return error;
 
-	if (priv->info->ccc_gac)
-		ravb_ptp_init(ndev, priv->pdev);
-
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 	napi_enable(&priv->napi[RAVB_BE]);
-- 
2.55.0

[PATCH net-next v3 02/10] net: ethernet: ravb: Move programming of gPTP timer interval

[Niklas Söderlund]

Commit f384ab481cab ("net: ravb: Split GTI computation and set
operations") broke apart the operations of computing the timer interval
and programming of it. However it kept the programming of the interval
in the RAVB main logic.

Having split the two apart this can be improved further by moving the
programming to the gPTP initialization function, as the first action of
the gPTP init function is to wait for the timer interval programming to
be acknowledge by the hardware.

As an added bonus the interaction with the gPTP registers for the
programming can then also be done while holding the gPTP registers lock.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 16 ----------------
 drivers/net/ethernet/renesas/ravb_ptp.c  | 11 ++++++++++-
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 36173a09e2fd..a89f6fe7866f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1846,20 +1846,6 @@ static int ravb_set_config_mode(struct net_device *ndev)
 	return error;
 }
 
-static void ravb_set_gti(struct net_device *ndev)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
-
-	if (!(info->gptp || info->ccc_gac))
-		return;
-
-	ravb_write(ndev, priv->gti_tiv, GTI);
-
-	/* Request GTI loading */
-	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
-}
-
 static int ravb_compute_gti(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -1974,8 +1960,6 @@ static int ravb_open(struct net_device *ndev)
 
 	ravb_emac_init(ndev);
 
-	ravb_set_gti(ndev);
-
 	/* Initialise PTP Clock driver */
 	if (info->gptp || info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index 43218bc15b15..157fb78209c7 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -321,11 +321,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 
 	priv->ptp.info = ravb_ptp_info;
 
-	priv->ptp.default_addend = ravb_read(ndev, GTI);
+	priv->ptp.default_addend = priv->gti_tiv;
 	priv->ptp.current_addend = priv->ptp.default_addend;
 
 	spin_lock_irqsave(&priv->lock, flags);
+
+	/* Set gPTP Timer Increment Value. */
+	ravb_write(ndev, priv->ptp.default_addend, GTI);
+
+	/* Request GTI loading. */
+	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+
+	/* Wait for GIT loading to complete. */
 	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
+
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-- 
2.55.0

[PATCH net-next v3 03/10] net: ethernet: ravb: Simplify gPTP start and stop

[Niklas Söderlund]

For devices that do not support the gPTP clock in config mode the
somewhat oddly named flag gptp is set, compared to devices that do
support the gPTP clock in config and operation mode where the flag
ccc_gac is set instead. The two flags are mutually exclusive.

For the gptp-flag devices (Gen2) the clock is tied to the AVB-DMAC, when
it is stopped so is the gPTP clock. For ccc_gac-flag devices (Gen3) the
gPTP clock is available whenever the ndev is open.

Prepare to add Gen4 support which will add a third way by cleaning the
Gen2 and Gen3 cases up a bit.

Fold the gptp-flag start and stop calls into ravb_dmac_init() and
ravb_stop_dma(), which start and stop the AVB-DMAC. There are no
functional change as all call sites to the construct:

    if (info->gptp)
        ravb_ptp_init(ndev, priv->pdev);

are always just after a call to into ravb_dmac_init() and all call sites
to the construct:

    if (info->gptp)
        ravb_ptp_stop(ndev);

are always directly followed by a call to ravb_stop_dma().

There are two special cases where the calling construct covers both the
gptp-flag and info->ccc_gac devices, one for start and one for stop. The
condition that it is preceded by a call to ravb_dmac_init(), or followed
by a call to ravb_stop_dma() are however true for them too. Reworked the
two special cases to drop the check of info->gptp.

The end result is that the gPTP clock will be started or stopped for the
gptp-flag devices in tandem with the AVB-DMAC, while the info->ccc_gac
devices will be controlled, as before, when the ndev is opened or
closed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v1
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb_main.c | 37 ++++++++++--------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a89f6fe7866f..2d0022a4f85c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -707,7 +707,15 @@ static int ravb_dmac_init(struct net_device *ndev)
 		return error;
 
 	/* Setting the control will start the AVB-DMAC process. */
-	return ravb_set_opmode(ndev, CCC_OPC_OPERATION);
+	error = ravb_set_opmode(ndev, CCC_OPC_OPERATION);
+	if (error)
+		return error;
+
+	/* Initialise PTP Clock driver */
+	if (info->gptp)
+		ravb_ptp_init(ndev, priv->pdev);
+
+	return 0;
 }
 
 static void ravb_get_tx_tstamp(struct net_device *ndev)
@@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
 			netdev_err(ndev, "failed to stop AXI BUS\n");
 	}
 
+	/* Stop PTP Clock driver */
+	if (info->gptp)
+		ravb_ptp_stop(ndev);
+
 	/* Stop AVB-DMAC process */
 	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
@@ -1719,9 +1731,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
 
 	if (netif_running(ndev)) {
 		netif_device_detach(ndev);
-		/* Stop PTP Clock driver */
-		if (info->gptp)
-			ravb_ptp_stop(ndev);
+
 		/* Wait for DMA stopping */
 		error = ravb_stop_dma(ndev);
 		if (error) {
@@ -1752,10 +1762,6 @@ static int ravb_set_ringparam(struct net_device *ndev,
 
 		ravb_emac_init(ndev);
 
-		/* Initialise PTP Clock driver */
-		if (info->gptp)
-			ravb_ptp_init(ndev, priv->pdev);
-
 		netif_device_attach(ndev);
 	}
 
@@ -1961,7 +1967,7 @@ static int ravb_open(struct net_device *ndev)
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
+	if (info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
 
 	/* PHY control start */
@@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
 	return 0;
 
 out_ptp_stop:
-	/* Stop PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
-		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
 out_set_reset:
 	ravb_set_opmode(ndev, CCC_OPC_RESET);
@@ -2020,10 +2023,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 
 	netif_tx_stop_all_queues(ndev);
 
-	/* Stop PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_stop(ndev);
-
 	/* Wait for DMA stopping */
 	if (ravb_stop_dma(ndev)) {
 		/* If ravb_stop_dma() fails, the hardware is still operating
@@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 	ravb_emac_init(ndev);
 
 out:
-	/* Initialise PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_init(ndev, priv->pdev);
-
 	netif_tx_start_all_queues(ndev);
 
 out_unlock:
@@ -2374,7 +2369,7 @@ static int ravb_close(struct net_device *ndev)
 	}
 
 	/* Stop PTP Clock driver */
-	if (info->gptp || info->ccc_gac)
+	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
 
 	/* Set the config mode to stop the AVB-DMAC's processes */
-- 
2.55.0

[PATCH net-next v3 04/10] net: ethernet: ravb: Remove redundant argument to ravb_ptp_init()

[Niklas Söderlund]

There is no need to explicitly pass the struct platform_device pointer
to ravb_ptp_init(), it can retrieve it directly from the private data
structure.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 4 ++--
 drivers/net/ethernet/renesas/ravb_ptp.c  | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 3ee4c6108189..aa45f5466001 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1163,7 +1163,7 @@ void ravb_modify(struct net_device *ndev, enum ravb_reg reg, u32 clear,
 int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value);
 
 void ravb_ptp_interrupt(struct net_device *ndev);
-void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev);
+void ravb_ptp_init(struct net_device *ndev);
 void ravb_ptp_stop(struct net_device *ndev);
 
 #endif	/* #ifndef __RAVB_H__ */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 2d0022a4f85c..e44d065f77cc 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -713,7 +713,7 @@ static int ravb_dmac_init(struct net_device *ndev)
 
 	/* Initialise PTP Clock driver */
 	if (info->gptp)
-		ravb_ptp_init(ndev, priv->pdev);
+		ravb_ptp_init(ndev);
 
 	return 0;
 }
@@ -1968,7 +1968,7 @@ static int ravb_open(struct net_device *ndev)
 
 	/* Initialise PTP Clock driver */
 	if (info->ccc_gac)
-		ravb_ptp_init(ndev, priv->pdev);
+		ravb_ptp_init(ndev);
 
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index 157fb78209c7..f70b616ac847 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -313,7 +313,7 @@ void ravb_ptp_interrupt(struct net_device *ndev)
 	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
 }
 
-void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
+void ravb_ptp_init(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	struct ptp_clock *clock;
@@ -338,7 +338,7 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
+	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
 	if (IS_ERR(clock)) {
 		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
 		clock = NULL;
-- 
2.55.0

[PATCH net-next v3 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[Niklas Söderlund]

If the PTP clock registration failed the error is silently ignored.
Before reworking all callers of ravb_ptp_init() handle and propagate the
error so it can be used.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v2
- Rework to remove the caching of phc_index added by LLM without access
  to hardware.

* Changes since v1
- New in v2.
---
 drivers/net/ethernet/renesas/ravb.h      |  3 +--
 drivers/net/ethernet/renesas/ravb_main.c |  3 +--
 drivers/net/ethernet/renesas/ravb_ptp.c  | 25 ++++++++++--------------
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index aa45f5466001..0c122a815840 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1028,7 +1028,6 @@ struct ravb_ptp_perout {
 struct ravb_ptp {
 	struct ptp_clock *clock;
 	struct ptp_clock_info info;
-	int phc_index;
 	u32 default_addend;
 	u32 current_addend;
 	int extts[N_EXT_TS];
@@ -1163,7 +1162,7 @@ void ravb_modify(struct net_device *ndev, enum ravb_reg reg, u32 clear,
 int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value);
 
 void ravb_ptp_interrupt(struct net_device *ndev);
-void ravb_ptp_init(struct net_device *ndev);
+int ravb_ptp_init(struct net_device *ndev);
 void ravb_ptp_stop(struct net_device *ndev);
 
 #endif	/* #ifndef __RAVB_H__ */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e44d065f77cc..7c7b3ac81e5d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 			(1 << HWTSTAMP_FILTER_ALL);
-		info->phc_index = READ_ONCE(priv->ptp.phc_index);
+		info->phc_index = ptp_clock_index(priv->ptp.clock);
 	}
 
 	return 0;
@@ -2934,7 +2934,6 @@ static int ravb_probe(struct platform_device *pdev)
 	priv->rstc = rstc;
 	priv->ndev = ndev;
 	priv->pdev = pdev;
-	priv->ptp.phc_index = -1;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
 	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
 	if (info->nc_queues) {
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index f70b616ac847..19d039b9d24a 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -313,11 +313,11 @@ void ravb_ptp_interrupt(struct net_device *ndev)
 	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
 }
 
-void ravb_ptp_init(struct net_device *ndev)
+int ravb_ptp_init(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	struct ptp_clock *clock;
 	unsigned long flags;
+	int ret = 0;
 
 	priv->ptp.info = ravb_ptp_info;
 
@@ -338,15 +338,14 @@ void ravb_ptp_init(struct net_device *ndev)
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
-	if (IS_ERR(clock)) {
-		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
-		clock = NULL;
+	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
+	if (IS_ERR(priv->ptp.clock)) {
+		ret = PTR_ERR(priv->ptp.clock);
+		priv->ptp.clock = NULL;
+		ravb_ptp_stop(ndev);
 	}
 
-	WRITE_ONCE(priv->ptp.clock, clock);
-	if (clock)
-		WRITE_ONCE(priv->ptp.phc_index, ptp_clock_index(clock));
+	return ret;
 }
 
 static void ravb_ptp_disable(struct net_device *ndev)
@@ -369,14 +368,10 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
 void ravb_ptp_stop(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	struct ptp_clock *clock;
-
-	WRITE_ONCE(priv->ptp.phc_index, -1);
-	clock = xchg(&priv->ptp.clock, NULL);
 
 	ravb_ptp_disable(ndev);
 	ravb_ptp_sync_irqs(ndev);
 
-	if (clock)
-		ptp_clock_unregister(clock);
+	if (priv->ptp.clock)
+		ptp_clock_unregister(priv->ptp.clock);
 }
-- 
2.55.0

[PATCH net-next v3 06/10] net: ethernet: ravb: Replace gPTP flags with callbacks

[Niklas Söderlund]

Prepare for adding Gen4 support which will add a third and new way to
interact with the gPTP clock by replacing the flags for Gen2 behavior
(info->gptp) and Gen3 behavior (info->ccc_gac) with callbacks.

This will make adding Gen4 support cleaner as the code will not have "if
else if else" sprinkled all over to handle each generations special
cases.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v1
- Handle error from starting the PTP clock.
---
 drivers/net/ethernet/renesas/ravb.h      | 24 ++++++-
 drivers/net/ethernet/renesas/ravb_main.c | 83 +++++++++++++++---------
 2 files changed, 76 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 0c122a815840..6603f90b19e1 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1034,6 +1034,27 @@ struct ravb_ptp {
 	struct ravb_ptp_perout perout[N_PER_OUT];
 };
 
+/**
+ * struct ravb_gptp_info - Platform specific gPTP behavior
+ *
+ * Each generation of RAVB have slightly different behaviors when interacting
+ * with the gPTP clock. This struct provides the callbacks to be called at
+ * critical points in the RAVB driver.
+ *
+ * @set_config_mode:	Enter config mode
+ * @dmac_start:		Called when the DMAC starts
+ * @dmac_stop:		Called when the DMAC stops
+ * @ndev_open:		Called when the ndev is opened
+ * @ndev_close:		Called when the ndev is closed
+ */
+struct ravb_gptp_info {
+	int (*set_config_mode)(struct net_device *ndev);
+	int (*dmac_start)(struct net_device *ndev);
+	void (*dmac_stop)(struct net_device *ndev);
+	int (*ndev_open)(struct net_device *ndev);
+	void (*ndev_close)(struct net_device *ndev);
+};
+
 struct ravb_hw_info {
 	int (*receive)(struct net_device *ndev, int budget, int q);
 	void (*set_rate)(struct net_device *ndev);
@@ -1052,6 +1073,7 @@ struct ravb_hw_info {
 	u32 rx_buffer_size;
 	u32 rx_desc_size;
 	u32 dbat_entry_num;
+	const struct ravb_gptp_info *ptp; /* Callbacks to handle gPTP interactions. */
 	unsigned aligned_tx: 1;
 	unsigned coalesce_irqs:1;	/* Needs software IRQ coalescing */
 
@@ -1062,8 +1084,6 @@ struct ravb_hw_info {
 	unsigned multi_irqs:1;		/* AVB-DMAC and E-MAC has multiple irqs */
 	unsigned irq_en_dis:1;		/* Has separate irq enable and disable regs */
 	unsigned err_mgmt_irqs:1;	/* Line1 (Err) and Line2 (Mgmt) irqs are separate */
-	unsigned gptp:1;		/* AVB-DMAC has gPTP support */
-	unsigned ccc_gac:1;		/* AVB-DMAC has gPTP support active in config mode */
 	unsigned gptp_ref_clk:1;	/* gPTP has separate reference clock */
 	unsigned nc_queues:1;		/* AVB-DMAC has RX and TX NC queues */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 7c7b3ac81e5d..5d5b9c0e2f45 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -712,8 +712,8 @@ static int ravb_dmac_init(struct net_device *ndev)
 		return error;
 
 	/* Initialise PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_init(ndev);
+	if (info->ptp && info->ptp->dmac_start)
+		return info->ptp->dmac_start(ndev);
 
 	return 0;
 }
@@ -1124,8 +1124,8 @@ static int ravb_stop_dma(struct net_device *ndev)
 	}
 
 	/* Stop PTP Clock driver */
-	if (info->gptp)
-		ravb_ptp_stop(ndev);
+	if (info->ptp && info->ptp->dmac_stop)
+		info->ptp->dmac_stop(ndev);
 
 	/* Stop AVB-DMAC process */
 	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
@@ -1774,7 +1774,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *hw_info = priv->info;
 
-	if (hw_info->gptp || hw_info->ccc_gac) {
+	if (hw_info->ptp) {
 		info->so_timestamping =
 			SOF_TIMESTAMPING_TX_SOFTWARE |
 			SOF_TIMESTAMPING_TX_HARDWARE |
@@ -1835,21 +1835,11 @@ static int ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	int error;
 
-	if (info->gptp) {
-		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
-		if (error)
-			return error;
-		/* Set CSEL value */
-		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
-	} else if (info->ccc_gac) {
-		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
-	} else {
-		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
-	}
+	if (info->ptp && info->ptp->set_config_mode)
+		return info->ptp->set_config_mode(ndev);
 
-	return error;
+	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
 
 static int ravb_compute_gti(struct net_device *ndev)
@@ -1860,7 +1850,7 @@ static int ravb_compute_gti(struct net_device *ndev)
 	unsigned long rate;
 	u64 inc;
 
-	if (!(info->gptp || info->ccc_gac))
+	if (!info->ptp)
 		return 0;
 
 	if (info->gptp_ref_clk)
@@ -1967,8 +1957,11 @@ static int ravb_open(struct net_device *ndev)
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_init(ndev);
+	if (info->ptp && info->ptp->ndev_open) {
+		error = info->ptp->ndev_open(ndev);
+		if (error)
+			goto out_ptp_stop;
+	}
 
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
@@ -2187,7 +2180,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	desc->dptr = cpu_to_le32(dma_addr);
 
 	/* TX timestamp required */
-	if (info->gptp || info->ccc_gac) {
+	if (info->ptp) {
 		if (q == RAVB_NC) {
 			ts_skb = kmalloc_obj(*ts_skb, GFP_ATOMIC);
 			if (!ts_skb) {
@@ -2369,8 +2362,8 @@ static int ravb_close(struct net_device *ndev)
 	}
 
 	/* Stop PTP Clock driver */
-	if (info->ccc_gac)
-		ravb_ptp_stop(ndev);
+	if (info->ptp && info->ptp->ndev_close)
+		info->ptp->ndev_close(ndev);
 
 	/* Set the config mode to stop the AVB-DMAC's processes */
 	if (ravb_stop_dma(ndev) < 0)
@@ -2378,7 +2371,7 @@ static int ravb_close(struct net_device *ndev)
 			   "device will be stopped after h/w processes are done.\n");
 
 	/* Clear the timestamp list */
-	if (info->gptp || info->ccc_gac) {
+	if (info->ptp) {
 		list_for_each_entry_safe(ts_skb, ts_skb2, &priv->ts_skb_list, list) {
 			list_del(&ts_skb->list);
 			kfree_skb(ts_skb->skb);
@@ -2660,6 +2653,26 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
+{
+	int ret;
+
+	ret = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
+	if (ret)
+		return ret;
+
+	/* gPTP Clock Select High-speed peripheral bus clock. */
+	ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
+
+	return 0;
+}
+
+static const struct ravb_gptp_info ravb_gen2_ptp_info = {
+	.set_config_mode = ravb_gen2_ptp_set_config_mode,
+	.dmac_start = ravb_ptp_init,
+	.dmac_stop = ravb_ptp_stop,
+};
+
 static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2678,12 +2691,24 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.dbat_entry_num = 22,
+	.ptp = &ravb_gen2_ptp_info,
 	.aligned_tx = 1,
-	.gptp = 1,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
 
+static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
+{
+	/* Enable gPTP Clock and Select High-speed peripheral bus clock. */
+	return ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
+}
+
+static const struct ravb_gptp_info ravb_gen3_ptp_info = {
+	.set_config_mode = ravb_gen3_ptp_set_config_mode,
+	.ndev_open = ravb_ptp_init,
+	.ndev_close = ravb_ptp_stop,
+};
+
 static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2702,11 +2727,11 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
 	.dbat_entry_num = 22,
+	.ptp = &ravb_gen3_ptp_info,
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
 	.irq_en_dis = 1,
-	.ccc_gac = 1,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -2733,7 +2758,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.tx_counters = 1,
 	.multi_irqs = 1,
 	.irq_en_dis = 1,
-	.ccc_gac = 1,
+	.ptp = &ravb_gen3_ptp_info,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -2758,7 +2783,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.dbat_entry_num = 22,
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
-	.gptp = 1,
+	.ptp = &ravb_gen2_ptp_info,
 	.gptp_ref_clk = 1,
 	.nc_queues = 1,
 	.magic_pkt = 1,
-- 
2.55.0

[PATCH net-next v3 07/10] net: ethernet: ravb: Add callback for gPTP probe

[Niklas Söderlund]

Different generations of the RAVB IP have different needs when it probes
the gPTP timer clock. Add a callback in the PTP information to allow
each generation to probe its own way.

With this the last gPTP specific flag (gptp_ref_clk) can be removed.
However the primary motivation for the change is to prepare for Gen4
support, which compared to other generations with gPTP support does not
have the clock as part of the IP itself.

Gen4 will not need to compute GTI value as it have nowhere to write it,
as the gPTP clock is external. For this reason move the computation of
it into the newly gPTP probe specific callbacks for the RAVB IP's that
support it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v1
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb.h      |  3 +-
 drivers/net/ethernet/renesas/ravb_main.c | 53 +++++++++++++++---------
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 6603f90b19e1..b788e8f5260d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1041,6 +1041,7 @@ struct ravb_ptp {
  * with the gPTP clock. This struct provides the callbacks to be called at
  * critical points in the RAVB driver.
  *
+ * @probe:		Probe the gPTP clock
  * @set_config_mode:	Enter config mode
  * @dmac_start:		Called when the DMAC starts
  * @dmac_stop:		Called when the DMAC stops
@@ -1048,6 +1049,7 @@ struct ravb_ptp {
  * @ndev_close:		Called when the ndev is closed
  */
 struct ravb_gptp_info {
+	int (*probe)(struct net_device *ndev);
 	int (*set_config_mode)(struct net_device *ndev);
 	int (*dmac_start)(struct net_device *ndev);
 	void (*dmac_stop)(struct net_device *ndev);
@@ -1084,7 +1086,6 @@ struct ravb_hw_info {
 	unsigned multi_irqs:1;		/* AVB-DMAC and E-MAC has multiple irqs */
 	unsigned irq_en_dis:1;		/* Has separate irq enable and disable regs */
 	unsigned err_mgmt_irqs:1;	/* Line1 (Err) and Line2 (Mgmt) irqs are separate */
-	unsigned gptp_ref_clk:1;	/* gPTP has separate reference clock */
 	unsigned nc_queues:1;		/* AVB-DMAC has RX and TX NC queues */
 	unsigned magic_pkt:1;		/* E-MAC supports magic packet detection */
 	unsigned half_duplex:1;		/* E-MAC supports half duplex mode */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5d5b9c0e2f45..7e53eca5327f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1842,21 +1842,14 @@ static int ravb_set_config_mode(struct net_device *ndev)
 	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
 
-static int ravb_compute_gti(struct net_device *ndev)
+static int ravb_compute_gti(struct net_device *ndev, struct clk *clk)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
 	struct device *dev = ndev->dev.parent;
 	unsigned long rate;
 	u64 inc;
 
-	if (!info->ptp)
-		return 0;
-
-	if (info->gptp_ref_clk)
-		rate = clk_get_rate(priv->gptp_clk);
-	else
-		rate = clk_get_rate(priv->clk);
+	rate = clk_get_rate(clk);
 	if (!rate)
 		return -EINVAL;
 
@@ -2653,6 +2646,13 @@ static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static int ravb_gen2_ptp_probe(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	return ravb_compute_gti(ndev, priv->clk);
+}
+
 static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 {
 	int ret;
@@ -2668,6 +2668,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 }
 
 static const struct ravb_gptp_info ravb_gen2_ptp_info = {
+	.probe = ravb_gen2_ptp_probe,
 	.set_config_mode = ravb_gen2_ptp_set_config_mode,
 	.dmac_start = ravb_ptp_init,
 	.dmac_stop = ravb_ptp_stop,
@@ -2704,6 +2705,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
 }
 
 static const struct ravb_gptp_info ravb_gen3_ptp_info = {
+	.probe = ravb_gen2_ptp_probe,
 	.set_config_mode = ravb_gen3_ptp_set_config_mode,
 	.ndev_open = ravb_ptp_init,
 	.ndev_close = ravb_ptp_stop,
@@ -2763,6 +2765,24 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.magic_pkt = 1,
 };
 
+static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	priv->gptp_clk = devm_clk_get(&priv->pdev->dev, "gptp");
+	if (IS_ERR(priv->gptp_clk))
+		return PTR_ERR(priv->gptp_clk);
+
+	return ravb_compute_gti(ndev, priv->gptp_clk);
+}
+
+static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
+	.probe = ravb_rzv2m_ptp_probe,
+	.set_config_mode = ravb_gen2_ptp_set_config_mode,
+	.dmac_start = ravb_ptp_init,
+	.dmac_stop = ravb_ptp_stop,
+};
+
 static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2783,8 +2803,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.dbat_entry_num = 22,
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
-	.ptp = &ravb_gen2_ptp_info,
-	.gptp_ref_clk = 1,
+	.ptp = &ravb_rzv2m_ptp_info,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -2976,12 +2995,10 @@ static int ravb_probe(struct platform_device *pdev)
 		goto out_reset_assert;
 	}
 
-	if (info->gptp_ref_clk) {
-		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
-		if (IS_ERR(priv->gptp_clk)) {
-			error = PTR_ERR(priv->gptp_clk);
+	if (info->ptp && info->ptp->probe) {
+		error = info->ptp->probe(ndev);
+		if (error)
 			goto out_reset_assert;
-		}
 	}
 
 	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
@@ -3034,10 +3051,6 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
 
-	error = ravb_compute_gti(ndev);
-	if (error)
-		goto out_rpm_put;
-
 	ravb_parse_delay_mode(np, ndev);
 
 	/* Allocate descriptor base address table */
-- 
2.55.0

[PATCH net-next v3 08/10] net: ethernet: ravb: Add callback for gPTP clock index

[Niklas Söderlund]

Prepare for adding Gen4 support which have an optional external gPTP
clock. Add a callback to get the clock index and use it to determine if
the device shall report gPTP support.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  2 ++
 drivers/net/ethernet/renesas/ravb_main.c | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index b788e8f5260d..27e2b31955ba 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1042,6 +1042,7 @@ struct ravb_ptp {
  * critical points in the RAVB driver.
  *
  * @probe:		Probe the gPTP clock
+ * @clock_index:	Get the PTP clock index, if any
  * @set_config_mode:	Enter config mode
  * @dmac_start:		Called when the DMAC starts
  * @dmac_stop:		Called when the DMAC stops
@@ -1050,6 +1051,7 @@ struct ravb_ptp {
  */
 struct ravb_gptp_info {
 	int (*probe)(struct net_device *ndev);
+	int (*clock_index)(struct net_device *ndev);
 	int (*set_config_mode)(struct net_device *ndev);
 	int (*dmac_start)(struct net_device *ndev);
 	void (*dmac_stop)(struct net_device *ndev);
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 7e53eca5327f..402370e1be63 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1773,8 +1773,13 @@ static int ravb_get_ts_info(struct net_device *ndev,
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *hw_info = priv->info;
+	int index = -1;
 
-	if (hw_info->ptp) {
+	if (hw_info->ptp && hw_info->ptp->clock_index)
+		index = hw_info->ptp->clock_index(ndev);
+
+	/* Only advertise ptp clock if present. */
+	if (index >= 0) {
 		info->so_timestamping =
 			SOF_TIMESTAMPING_TX_SOFTWARE |
 			SOF_TIMESTAMPING_TX_HARDWARE |
@@ -1785,7 +1790,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 			(1 << HWTSTAMP_FILTER_ALL);
-		info->phc_index = ptp_clock_index(priv->ptp.clock);
+		info->phc_index = index;
 	}
 
 	return 0;
@@ -2653,6 +2658,13 @@ static int ravb_gen2_ptp_probe(struct net_device *ndev)
 	return ravb_compute_gti(ndev, priv->clk);
 }
 
+static int ravb_gen2_ptp_clock_index(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	return ptp_clock_index(priv->ptp.clock);
+}
+
 static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 {
 	int ret;
@@ -2669,6 +2681,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
 
 static const struct ravb_gptp_info ravb_gen2_ptp_info = {
 	.probe = ravb_gen2_ptp_probe,
+	.clock_index = ravb_gen2_ptp_clock_index,
 	.set_config_mode = ravb_gen2_ptp_set_config_mode,
 	.dmac_start = ravb_ptp_init,
 	.dmac_stop = ravb_ptp_stop,
@@ -2706,6 +2719,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
 
 static const struct ravb_gptp_info ravb_gen3_ptp_info = {
 	.probe = ravb_gen2_ptp_probe,
+	.clock_index = ravb_gen2_ptp_clock_index,
 	.set_config_mode = ravb_gen3_ptp_set_config_mode,
 	.ndev_open = ravb_ptp_init,
 	.ndev_close = ravb_ptp_stop,
@@ -2778,6 +2792,7 @@ static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
 
 static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
 	.probe = ravb_rzv2m_ptp_probe,
+	.clock_index = ravb_gen2_ptp_clock_index,
 	.set_config_mode = ravb_gen2_ptp_set_config_mode,
 	.dmac_start = ravb_ptp_init,
 	.dmac_stop = ravb_ptp_stop,
-- 
2.55.0

[PATCH net-next v3 09/10] dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

[Niklas Söderlund]

The RAVB module on Gen4 have no gPTP clock as part of the RAVB module
itself, instead it relies on an external system wide gPTP clock. The
gPTP clock is shared with RTSN on V4H and RSWITCH on S4.

Add an optional phandle so that the RAVB driver can find and use the
gPTP clock. Ideally this should have been a mandatory property but for
backward compatible it is optional. The RAVB module is capable of
functioning without it, but can in such cases not provided PTP
functionality.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
* Changes since v1
- Use generic ptp-timer property instead of vendor specific one.
- Improve spelling in commit message.
---
 .../devicetree/bindings/net/renesas,etheravb.yaml     | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
index 1e00ef5b3acd..fad002492421 100644
--- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
+++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
@@ -97,6 +97,8 @@ properties:
 
   phy-handle: true
 
+  ptp-timer: true
+
   '#address-cells':
     description: Number of address cells for the MDIO bus.
     const: 1
@@ -305,6 +307,15 @@ allOf:
             items:
               - const: fck
               - const: refclk
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: renesas,etheravb-rcar-gen4
+    then:
+      properties:
+        ptp-timer: false
 
 additionalProperties: false
 
-- 
2.55.0

[PATCH net-next v3 10/10] net: ethernet: ravb: Add gPTP support for Gen4

[Niklas Söderlund]

While driver advertises gPTP support on Gen4 platforms it is in fact
completely broken. On R-Car Gen4 devices the RAVB module have no
internal gPTP clock as generations before it. Instead it utilizes a
system-wide gPTP clock.

This change utilizes the refactoring of the RAVB gPTP code to add
support for a system-wide clock and stops the Gen4 devices trying to use
the non-existing internal gPTP clock.

To remain backward compatible the ptp-timer property is optional.If not
present, or not enabled, the RAVB driver will no longer advertise gPTP
support to user-space.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
---
* Changes since v2
- Use full 100 character width instead of breaking at 80.
- Remove superfluous return statement.

* Changes since v1
- Use the standard generic ptp-timer property from
  ethernet-controller.yaml instead of a vendor specific one.
- Improve spelling in commit message.
---
 drivers/net/ethernet/renesas/ravb.h      |  3 ++
 drivers/net/ethernet/renesas/ravb_main.c | 54 +++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 27e2b31955ba..5d35b40d7c58 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -249,6 +249,8 @@ enum APSR_BIT {
 	APSR_RDM	= 0x00002000,
 	APSR_TDM	= 0x00004000,
 	APSR_MIISELECT	= 0x01000000,	/* R-Car V4M only */
+	APSR_GPTPTIMER_SOURCE = BIT(25), /* Gen4 */
+	APSR_GPTPCLOCK	= BIT(29),	/* Gen4 */
 };
 
 /* RCR */
@@ -1132,6 +1134,7 @@ struct ravb_private {
 	struct list_head ts_skb_list;
 	u32 ts_skb_tag;
 	struct ravb_ptp ptp;
+	struct device_node *of_gptp;	/* Reference to external gPTP clock, if any. */
 	spinlock_t lock;		/* Register access lock */
 	u32 cur_rx[NUM_RX_QUEUE];	/* Consumer ring indices */
 	u32 dirty_rx[NUM_RX_QUEUE];	/* Producer ring indices */
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 402370e1be63..66c9cd15a19e 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2752,6 +2752,57 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.magic_pkt = 1,
 };
 
+static int ravb_gen4_ptp_probe(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);
+	if (!priv->of_gptp)
+		return 0;
+
+	if (!of_device_is_available(priv->of_gptp)) {
+		of_node_put(priv->of_gptp);
+		priv->of_gptp = NULL;
+	}
+
+	return 0;
+}
+
+static int ravb_gen4_ptp_clock_index(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	/* If no clock, mimic ptp_clock_index_by_of_node() fail and return -1 */
+	if (!priv->of_gptp)
+		return -1;
+
+	return ptp_clock_index_by_of_node(priv->of_gptp);
+}
+
+static int ravb_gen4_ptp_set_config_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ret;
+
+	/* Enable gPTP Clock and Select High-speed peripheral bus clock. */
+	ret = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
+	if (ret)
+		return ret;
+
+	/* Set PTP source to GPTP module, only option on Gen4. */
+	if (priv->of_gptp)
+		ravb_modify(ndev, APSR, APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK,
+			    APSR_GPTPTIMER_SOURCE | APSR_GPTPCLOCK);
+
+	return 0;
+}
+
+static const struct ravb_gptp_info ravb_gen4_ptp_info = {
+	.probe = ravb_gen4_ptp_probe,
+	.clock_index = ravb_gen4_ptp_clock_index,
+	.set_config_mode = ravb_gen4_ptp_set_config_mode,
+};
+
 static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
@@ -2774,7 +2825,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.tx_counters = 1,
 	.multi_irqs = 1,
 	.irq_en_dis = 1,
-	.ptp = &ravb_gen3_ptp_info,
+	.ptp = &ravb_gen4_ptp_info,
 	.nc_queues = 1,
 	.magic_pkt = 1,
 };
@@ -3188,6 +3239,7 @@ static void ravb_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	clk_unprepare(priv->refclk);
+	of_node_put(priv->of_gptp);
 	reset_control_assert(priv->rstc);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
-- 
2.55.0

Re: [PATCH net-next v3 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling `ptp_clock_index()` with a NULL pointer causes a deterministic kernel panic when querying ethtool timestamp info.
- [Critical] Failing to clear `priv->ptp.clock` in `ravb_ptp_stop()` leaves a dangling pointer, leading to a Use-After-Free during ethtool queries.
--

commit 8ab377468104948ff315e1ab381238bac1a1783e
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Propagate error from ptp_clock_register()

If the PTP clock registration failed the error is silently ignored.
Before reworking all callers of ravb_ptp_init() handle and propagate the
error so it can be used.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index e44d065f77cc8..7c7b3ac81e5d6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = READ_ONCE(priv->ptp.phc_index);
> +		info->phc_index = ptp_clock_index(priv->ptp.clock);

[Severity: Critical]
Can this code trigger a NULL pointer dereference if priv->ptp.clock is NULL?

If the interface is not brought up yet, or if PTP registration failed during
initialization, priv->ptp.clock will be NULL. When querying ethtool for
timestamping information, ravb_get_ts_info() invokes
ptp_clock_index(priv->ptp.clock).

Since ptp_clock_index() unconditionally dereferences the pointer, it seems
this could result in a crash.

>  	}
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index f70b616ac847c..19d039b9d24a5 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
[ ... ]
> @@ -369,14 +368,10 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
>  void ravb_ptp_stop(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
> -
> -	WRITE_ONCE(priv->ptp.phc_index, -1);
> -	clock = xchg(&priv->ptp.clock, NULL);
>  
>  	ravb_ptp_disable(ndev);
>  	ravb_ptp_sync_irqs(ndev);
>  
> -	if (clock)
> -		ptp_clock_unregister(clock);
> +	if (priv->ptp.clock)
> +		ptp_clock_unregister(priv->ptp.clock);

[Severity: Critical]
Does this leave priv->ptp.clock as a dangling pointer?

The original code used xchg() to set priv->ptp.clock to NULL before
unregistering it. With this change, when the interface is brought down,
ravb_ptp_stop() unregisters and frees the PTP clock but leaves the pointer
pointing to freed memory.

If an unprivileged user or system daemon queries ethtool while the interface
is down, ravb_get_ts_info() will pass this dangling pointer to
ptp_clock_index(), potentially causing a use-after-free read. Should
priv->ptp.clock be explicitly set to NULL here?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=5

Re: [PATCH net-next v3 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch erroneously removes `ravb_ptp_stop()` from `ravb_wol_setup()`, leaving the gPTP hardware active during a WoL-enabled suspend.
--

commit bbd624fcad5eb4869638d985be16666aae0f0ca2
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Remove gPTP control from WoL setup and restore

This commit removes explicit gPTP stop/init calls from the Wake-on-LAN setup
and restore paths. The commit message states this is redundant as the device
is always closed and re-opened during suspend and resume.

> net: ethernet: ravb: Remove gPTP control from WoL setup and restore
> 
> Since commit a6a85ba36fd0 ("net: ravb: Move PTP initialization in the
> driver's ndo_open API for ccc_gac platorms") the gPTP clock (if
> supported) is stopped and started by opening and closing the ndev.
> 
> This makes the special case to stop and start it when resuming from WoL
> redundant. As the ndev will always be closed and re-opened when
> suspending and resuming the system.

[Severity: High]
Is this description accurate when Wake-on-LAN is enabled?

Looking at ravb_suspend() in drivers/net/ethernet/renesas/ravb_main.c,
ravb_close() is explicitly bypassed when wol_enabled is true:

	if (priv->wol_enabled) {
		ret = ravb_wol_setup(ndev);
		rtnl_unlock();
		return ret;
	}

	ret = ravb_close(ndev);

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791e..36173a09e2fdb 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
>  	/* Enable MagicPacket */
>  	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_stop(ndev);
> -
>  	return enable_irq_wake(priv->emac_irq);
>  }

[Severity: High]
Since ravb_close() is not called during a WoL-enabled suspend, doesn't removing
ravb_ptp_stop() here leave the gPTP hardware active during suspend?

>  
> @@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
>  	if (error)
>  		return error;
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  	napi_enable(&priv->napi[RAVB_BE]);

[Severity: High]
If the PTP clock remains active during suspend, won't it be abruptly torn down
by the ravb_close(ndev) call that occurs later in this ravb_wol_restore()
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=1

Re: [PATCH net-next v3 07/10] net: ethernet: ravb: Add callback for gPTP probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Dangling pointer in `priv->ptp.clock` leads to double free and use-after-free on error recovery paths.
- [High] Early shared IRQ registration in `ravb_probe` leads to Use-After-Free and NULL pointer dereferences.
--

commit 252e91f926cfc67c49e9dd06194245579401d492
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Add callback for gPTP probe

This commit introduces a gPTP probe callback to accommodate different
generations of the RAVB IP and removes the gptp_ref_clk flag. It also
moves GTI computation into the callbacks to prepare for Gen4 support
where the clock is external.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[ ... ]
> @@ -2976,12 +2995,10 @@ static int ravb_probe(struct platform_device *pdev)
>  		goto out_reset_assert;
>  	}
>  
> -	if (info->gptp_ref_clk) {
> -		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
> -		if (IS_ERR(priv->gptp_clk)) {
> -			error = PTR_ERR(priv->gptp_clk);
> +	if (info->ptp && info->ptp->probe) {
> +		error = info->ptp->probe(ndev);
> +		if (error)
>  			goto out_reset_assert;

[Severity: High]
This is a pre-existing issue, but does adding this new failure point expand
a hazard window with the shared interrupt handler?

In ravb_probe(), devm_request_irq() is called to register a shared IRQ
before priv->addr is mapped. If a premature IRQ is delivered, could it force
a panic by accessing an unmapped register?

Additionally, if info->ptp->probe() fails here, the error path jumps to
out_reset_assert and eventually out_free_netdev:

out_free_netdev:
    free_netdev(ndev);
    return error;

Could this leave a live devm-managed IRQ handler targeting freed memory if
the IRQ fires before the devm cleanup removes the handler?


[Severity: Critical]
This is a pre-existing issue, but is there a risk of a double free in the
error recovery paths related to the ptp clock?

When ravb_tx_timeout_work() initiates a reset, it can fail to re-initialize
the DMA. If it takes the error path without neutralizing the clock pointer:

ravb_tx_timeout_work() {
    ...
    error = ravb_dmac_init(ndev);
    if (error) {
        ...
        goto out_unlock;
    }
}

The error path leaves the device up, and a secondary teardown in
ravb_ptp_stop() (such as during ravb_close) will attempt to unregister the
clock again:

ravb_ptp_stop() {
    ...
    if (priv->ptp.clock)
        ptp_clock_unregister(priv->ptp.clock);
}

Since the pointer is freed during the initial timeout recovery but not
set to NULL, could this result in a use-after-free and double free when
ravb_close() is subsequently executed?

> -		}
>  	}
>  
>  	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=7

Re: [PATCH net-next v3 03/10] net: ethernet: ravb: Simplify gPTP start and stop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing PTP clock unregistration for `ccc_gac` (Gen3) devices on the `ravb_open()` error path leads to a Use-After-Free.
- [High] Missing PTP clock driver teardown for `gptp` (Gen2) devices on hardware timeout leads to a Use-After-Free.
- [Medium] Silent loss of PTP clock functionality during TX timeout recovery for `gptp` devices.
--

commit 13e73cf3cf26d2c226a9c19d3728cc27d5cffb72
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Simplify gPTP start and stop

This commit simplifies the gPTP start and stop logic by folding the gptp flag
handling directly into ravb_dmac_init() and ravb_stop_dma(). It aims to
control the clock for gptp devices in tandem with the AVB-DMAC, while leaving
ccc_gac devices tied to device open/close operations. However, this
refactoring appears to introduce regressions in error handling and timeout
recovery paths.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a89f6fe7866fe..2d0022a4f85c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[ ... ]
> @@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
>  			netdev_err(ndev, "failed to stop AXI BUS\n");
>  	}
>  
> +	/* Stop PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_stop(ndev);
> +
>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
>  }

[Severity: High]
Can a hardware timeout cause a use-after-free by skipping the PTP clock
teardown?

Earlier in ravb_stop_dma(), there are multiple ravb_wait() hardware polling
loops. If any of these wait loops time out, the function returns an error
immediately, bypassing this new ravb_ptp_stop() call. During a module unload
or interface close, if ravb_stop_dma() fails, teardown continues but the PTP
clock remains registered. Does this leave the /dev/ptp* device pointing to
freed memory?

[ ... ]
> @@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
>  	return 0;
>  
>  out_ptp_stop:
> -	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> -		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);
>  out_set_reset:
>  	ravb_set_opmode(ndev, CCC_OPC_RESET);

[Severity: High]
Does this error path leak the PTP clock driver for ccc_gac (Gen3) devices?

When ravb_ptp_init() is called successfully but a subsequent step like
ravb_phy_start() fails, the code jumps here to out_ptp_stop. Since
ravb_stop_dma() only stops the clock for gptp devices, the clock remains
registered for ccc_gac devices. If the network device is subsequently freed
by a module unload, could this lead to a use-after-free when userspace
accesses the dangling PTP device?

[ ... ]
> @@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	ravb_emac_init(ndev);
>  
>  out:
> -	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	netif_tx_start_all_queues(ndev);
>  
>  out_unlock:

[Severity: Medium]
Does removing this initialization cause a silent loss of PTP clock
functionality during TX timeout recovery?

If ravb_stop_dma() successfully stops the PTP clock but fails at its final
step (ravb_set_opmode()), it returns an error. The error path in
ravb_tx_timeout_work() jumps to the out label, re-enables queues, and
aborts full re-initialization.

Since ravb_ptp_init() is no longer called at the out label, does the PTP
clock remain permanently stopped and unregistered until the interface is
manually brought down and up again?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=3

Re: [PATCH net-next v3 06/10] net: ethernet: ravb: Replace gPTP flags with callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] ravb_dmac_init leaves the hardware in CCC_OPC_OPERATION mode if dmac_start fails.

Pre-existing issues:
- [High] Calling ethtool -T when the interface is down will trigger a NULL pointer dereference or a Use-After-Free in ravb_get_ts_info().
--

commit eb974310ac24dea7fe34514d8fd0eeeea0662742
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Replace gPTP flags with callbacks

Prepare for adding Gen4 support which will add a third and new way to
interact with the gPTP clock by replacing the flags for Gen2 behavior
(info->gptp) and Gen3 behavior (info->ccc_gac) with callbacks.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7c7b3ac81e5d6..5d5b9c0e2f450 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -712,8 +712,8 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->dmac_start)
> +		return info->ptp->dmac_start(ndev);

[Severity: Medium]
If dmac_start() (e.g. PTP initialization) fails during operations like setting
ring parameters or recovering from a TX timeout, ravb_dmac_init() returns the
error immediately.

Does this leave the hardware in CCC_OPC_OPERATION mode without rolling back
the hardware state to CCC_OPC_CONFIG?

Could this leave the device in an inconsistent state where the DMA is running
but the EMAC is disabled and driver queues are detached, causing a local
DoS until the interface is manually restarted?

>  
>  	return 0;
>  }

[ ... ]

> @@ -1774,7 +1774,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *hw_info = priv->info;
>  
> -	if (hw_info->gptp || hw_info->ccc_gac) {
> +	if (hw_info->ptp) {

[Severity: High]
This is a pre-existing issue, but calling ethtool -T when the interface is
down can trigger a NULL pointer dereference or a Use-After-Free here in
ravb_get_ts_info().

If a local unprivileged user queries timestamping info via SIOCETHTOOL while
the interface is administratively down, ravb_get_ts_info() eventually passes
priv->ptp.clock to ptp_clock_index(), which unconditionally dereferences it:

ravb_get_ts_info()
    info->phc_index = ptp_clock_index(priv->ptp.clock);

If the device is probed but not yet opened, priv->ptp.clock is NULL,
resulting in a NULL pointer dereference.

Furthermore, when the device is closed, ravb_ptp_stop() unregisters the
clock but does not clear the pointer:

ravb_ptp_stop()
    if (priv->ptp.clock)
        ptp_clock_unregister(priv->ptp.clock);

Calling ethtool -T in this state triggers a Use-After-Free. Could this
pointer be cleared on stop, and checked before use in ravb_get_ts_info()?

>  		info->so_timestamping =
>  			SOF_TIMESTAMPING_TX_SOFTWARE |
>  			SOF_TIMESTAMPING_TX_HARDWARE |

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=6

Re: [PATCH net-next v3 08/10] net: ethernet: ravb: Add callback for gPTP clock index

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling ethtool -T (get_ts_info) when the interface is down causes a kernel panic due to a NULL pointer dereference in ptp_clock_index().
--

commit 861424103c229fb3c19f621c9c92a3cf127c7143
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Add callback for gPTP clock index

This patch prepares for adding Gen4 support which has an optional external
gPTP clock. It adds a callback to get the clock index and uses it to
determine if the device should report gPTP support.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7e53eca5327f5..402370e1be634 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1773,8 +1773,13 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *hw_info = priv->info;
> +	int index = -1;
>  
> -	if (hw_info->ptp) {
> +	if (hw_info->ptp && hw_info->ptp->clock_index)
> +		index = hw_info->ptp->clock_index(ndev);

[Severity: High]
This is a pre-existing issue, but does this safely handle an absent clock
when the interface is down?

If userspace queries timestamping capabilities (e.g., via ethtool -T) while
the interface is down, ravb_get_ts_info() will call the clock_index callback.
However, priv->ptp.clock is only initialized when the interface is brought up
in ravb_ptp_init().

[ ... ]

> @@ -2653,6 +2658,13 @@ static int ravb_gen2_ptp_probe(struct net_device *ndev)
>  	return ravb_compute_gti(ndev, priv->clk);
>  }
>  
> +static int ravb_gen2_ptp_clock_index(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	return ptp_clock_index(priv->ptp.clock);

[Severity: High]
This isn't a bug introduced by this patch, but will this crash if
priv->ptp.clock is NULL?

Because ptp_clock_index() dereferences the passed pointer without validation,
calling it when the interface is down and the clock hasn't been initialized
appears to lead to a NULL pointer dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=8

Re: [PATCH net-next v3 02/10] net: ethernet: ravb: Move programming of gPTP timer interval

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The wait condition intended to synchronize GTI (gPTP Timer Increment) loading completion checks the wrong register bit (GCCR_TCR instead of GCCR_LTI), leading to a tight race condition and potential hardware state corruption.
--

commit 9509ae0d35af26310454ae24d344c98a9575f582
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Move programming of gPTP timer interval

This patch moves the programming of the gPTP timer interval from the RAVB
main logic directly into the gPTP initialization function, ensuring it is
done under the gPTP registers lock.

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 43218bc15b151..157fb78209c78 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -321,11 +321,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
> -	priv->ptp.default_addend = ravb_read(ndev, GTI);
> +	priv->ptp.default_addend = priv->gti_tiv;
>  	priv->ptp.current_addend = priv->ptp.default_addend;
>  
>  	spin_lock_irqsave(&priv->lock, flags);
> +
> +	/* Set gPTP Timer Increment Value. */
> +	ravb_write(ndev, priv->ptp.default_addend, GTI);
> +
> +	/* Request GTI loading. */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Wait for GIT loading to complete. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

[Severity: Medium]
Are we polling the wrong register bit to wait for GTI loading to complete?

The comment says "Wait for GIT loading to complete", but the wait polls
GCCR_TCR instead of GCCR_LTI which was just set. Because GCCR_TCR is
likely already 0 (NOREQ), the wait returns immediately.

If the hardware is still processing the LTI load when the read-modify-write
on GCCR executes on the next line, could it read GCCR_LTI as 1 and
inadvertently write it back as 1?

By moving the GTI loading directly above this incorrect wait, does this
create a tight race window that might corrupt the hardware state?

>  	spin_unlock_irqrestore(&priv->lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=2

Re: [PATCH net-next v3 10/10] net: ethernet: ravb: Add gPTP support for Gen4

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing `of_node_put()` in the error paths of `ravb_probe()` causes a memory leak of the `ptp-timer` device node reference acquired by `ravb_gen4_ptp_probe()`.
--

commit d2e45518cd8b99519759a683fe36054ff2cdc88d
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Add gPTP support for Gen4

This commit adds gPTP support for RAVB Gen4 platforms by utilizing a
system-wide clock rather than an internal one. It updates the RAVB gPTP
driver code to parse and use an optional "ptp-timer" device tree property
for Gen4 devices.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2752,6 +2752,57 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {

[ ... ]

> +static int ravb_gen4_ptp_probe(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);
> +	if (!priv->of_gptp)
> +		return 0;

[ ... ]

> @@ -3188,6 +3239,7 @@ static void ravb_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(dev);
>  	clk_unprepare(priv->refclk);
> +	of_node_put(priv->of_gptp);
>  	reset_control_assert(priv->rstc);
>  	free_netdev(ndev);
>  	platform_set_drvdata(pdev, NULL);

[Severity: Medium]
Since priv->of_gptp acquires a node reference via of_parse_phandle() in
ravb_gen4_ptp_probe(), and is correctly released here in ravb_remove(), do the
error paths in ravb_probe() also need to call of_node_put(priv->of_gptp)?

If an initialization step fails in ravb_probe() in
drivers/net/ethernet/renesas/ravb_main.c after info->ptp->probe(ndev)
successfully acquires the node reference, the error paths jump to these labels:

out_rpm_disable:
	pm_runtime_disable(&pdev->dev);
	pm_runtime_dont_use_autosuspend(&pdev->dev);
	clk_unprepare(priv->refclk);
out_reset_assert:
	reset_control_assert(rstc);
out_free_netdev:
	free_netdev(ndev);
	return error;

Will this leak the device node reference if probe fails here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=10

Re: [PATCH net-next v3 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[Niklas Söderlund]

On 2026-09-03 08:47:55 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The patch erroneously removes `ravb_ptp_stop()` from `ravb_wol_setup()`, leaving the gPTP hardware active during a WoL-enabled suspend.

This is a non-issue as the module clock is the same, ignore.

> --
> 
> commit bbd624fcad5eb4869638d985be16666aae0f0ca2
> Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> net: ethernet: ravb: Remove gPTP control from WoL setup and restore
> 
> This commit removes explicit gPTP stop/init calls from the Wake-on-LAN setup
> and restore paths. The commit message states this is redundant as the device
> is always closed and re-opened during suspend and resume.
> 
> > net: ethernet: ravb: Remove gPTP control from WoL setup and restore
> > 
> > Since commit a6a85ba36fd0 ("net: ravb: Move PTP initialization in the
> > driver's ndo_open API for ccc_gac platorms") the gPTP clock (if
> > supported) is stopped and started by opening and closing the ndev.
> > 
> > This makes the special case to stop and start it when resuming from WoL
> > redundant. As the ndev will always be closed and re-opened when
> > suspending and resuming the system.
> 
> [Severity: High]
> Is this description accurate when Wake-on-LAN is enabled?
> 
> Looking at ravb_suspend() in drivers/net/ethernet/renesas/ravb_main.c,
> ravb_close() is explicitly bypassed when wol_enabled is true:
> 
> 	if (priv->wol_enabled) {
> 		ret = ravb_wol_setup(ndev);
> 		rtnl_unlock();
> 		return ret;
> 	}
> 
> 	ret = ravb_close(ndev);
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index ea1c7e536791e..36173a09e2fdb 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
> >  	/* Enable MagicPacket */
> >  	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
> >  
> > -	if (priv->info->ccc_gac)
> > -		ravb_ptp_stop(ndev);
> > -
> >  	return enable_irq_wake(priv->emac_irq);
> >  }
> 
> [Severity: High]
> Since ravb_close() is not called during a WoL-enabled suspend, doesn't removing
> ravb_ptp_stop() here leave the gPTP hardware active during suspend?
> 
> >  
> > @@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
> >  	if (error)
> >  		return error;
> >  
> > -	if (priv->info->ccc_gac)
> > -		ravb_ptp_init(ndev, priv->pdev);
> > -
> >  	if (info->nc_queues)
> >  		napi_enable(&priv->napi[RAVB_NC]);
> >  	napi_enable(&priv->napi[RAVB_BE]);
> 
> [Severity: High]
> If the PTP clock remains active during suspend, won't it be abruptly torn down
> by the ravb_close(ndev) call that occurs later in this ravb_wol_restore()
> function?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=1

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net-next v3 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[Niklas Söderlund]

On 2026-09-03 08:47:54 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Calling `ptp_clock_index()` with a NULL pointer causes a deterministic kernel panic when querying ethtool timestamp info.
> - [Critical] Failing to clear `priv->ptp.clock` in `ravb_ptp_stop()` leaves a dangling pointer, leading to a Use-After-Free during ethtool queries.
> --
> 
> commit 8ab377468104948ff315e1ab381238bac1a1783e
> Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> net: ethernet: ravb: Propagate error from ptp_clock_register()
> 
> If the PTP clock registration failed the error is silently ignored.
> Before reworking all callers of ravb_ptp_init() handle and propagate the
> error so it can be used.
> 
> Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index e44d065f77cc8..7c7b3ac81e5d6 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
> >  			(1 << HWTSTAMP_FILTER_NONE) |
> >  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
> >  			(1 << HWTSTAMP_FILTER_ALL);
> > -		info->phc_index = READ_ONCE(priv->ptp.phc_index);
> > +		info->phc_index = ptp_clock_index(priv->ptp.clock);
> 
> [Severity: Critical]
> Can this code trigger a NULL pointer dereference if priv->ptp.clock is NULL?
> 
> If the interface is not brought up yet, or if PTP registration failed during
> initialization, priv->ptp.clock will be NULL. When querying ethtool for
> timestamping information, ravb_get_ts_info() invokes
> ptp_clock_index(priv->ptp.clock).
> 
> Since ptp_clock_index() unconditionally dereferences the pointer, it seems
> this could result in a crash.

Please check patch 9 in this series. The change is an effect of the LLM 
driven fix merged a head of this series with tried to correct this issue 
in a way to complicated way by using the READ_ONCE and caching the 
value...

> 
> >  	}
> >  
> >  	return 0;
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> > index f70b616ac847c..19d039b9d24a5 100644
> > --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> > +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> [ ... ]
> > @@ -369,14 +368,10 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
> >  void ravb_ptp_stop(struct net_device *ndev)
> >  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> > -	struct ptp_clock *clock;
> > -
> > -	WRITE_ONCE(priv->ptp.phc_index, -1);
> > -	clock = xchg(&priv->ptp.clock, NULL);
> >  
> >  	ravb_ptp_disable(ndev);
> >  	ravb_ptp_sync_irqs(ndev);
> >  
> > -	if (clock)
> > -		ptp_clock_unregister(clock);
> > +	if (priv->ptp.clock)
> > +		ptp_clock_unregister(priv->ptp.clock);
> 
> [Severity: Critical]
> Does this leave priv->ptp.clock as a dangling pointer?
> 
> The original code used xchg() to set priv->ptp.clock to NULL before
> unregistering it. With this change, when the interface is brought down,
> ravb_ptp_stop() unregisters and frees the PTP clock but leaves the pointer
> pointing to freed memory.
> 
> If an unprivileged user or system daemon queries ethtool while the interface
> is down, ravb_get_ts_info() will pass this dangling pointer to
> ptp_clock_index(), potentially causing a use-after-free read. Should
> priv->ptp.clock be explicitly set to NULL here?
> 
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=5

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net-next v3 01/10] net: ethernet: ravb: Remove gPTP control from WoL setup and restore

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] The commit message's stated justification for the removal is
  factually wrong for the suspend leg, and the removal deletes the only…

--

net: ethernet: ravb: Remove gPTP control from WoL setup and restore

This drops the ccc_gac special case that called ravb_ptp_stop() from
ravb_wol_setup() and ravb_ptp_init() from ravb_wol_restore(), on the
grounds that ndo_open/ndo_close already start and stop the gPTP clock.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791e..36173a09e2fdb 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -3182,9 +3182,6 @@ static int ravb_wol_setup(struct net_device *ndev)
>  	/* Enable MagicPacket */
>  	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_stop(ndev);
> -
>  	return enable_irq_wake(priv->emac_irq);
>  }

[Severity: High]

Is the ndev really closed on the suspend leg when WoL is enabled?
ravb_suspend() returns immediately after ravb_wol_setup() without ever
calling ravb_close():

drivers/net/ethernet/renesas/ravb_main.c:ravb_suspend() {
	rtnl_lock();
	if (priv->wol_enabled) {
		ret = ravb_wol_setup(ndev);
		rtnl_unlock();
		return ret;
	}

	ret = ravb_close(ndev);
	...
}

The only ravb_close() in the WoL flow is at the end of ravb_wol_restore(),
i.e. on the resume leg. So the commit message statement:

  "As the ndev will always be closed and re-opened when suspending and
  resuming the system."

does not appear to hold for the suspend direction. Should the message be
adjusted, or is the intent to change behaviour here?

With ravb_ptp_stop() gone from ravb_wol_setup(), doesn't the WoL suspend
path lose the gPTP masking and IRQ draining that ravb_ptp_stop() performed?

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_stop() {
	ravb_ptp_disable(ndev);		/* GIC = 0, GIS = 0 */
	ravb_ptp_sync_irqs(ndev);	/* synchronize_irq(ndev->irq), err_irq, mgmt_irq */
	...
}

After this patch ravb_wol_setup() only clears RIC0/RIC2/TIC, narrows
ECSIPR, and synchronizes priv->emac_irq, so gPTP interrupt sources armed by
userspace (extts PTCE, perout PTME) stay enabled and the AVB-DMAC / err /
mgmt IRQ lines are never drained. Do the retained comments in
ravb_wol_setup() still describe the code?

	/* Disable interrupts by clearing the interrupt masks. */
	...
	/* Only allow ECI interrupts */
	synchronize_irq(priv->emac_irq);

Can a gPTP interrupt latched during the sleep then be replayed against a
block that ravb_wol_restore() is resetting? resume_device_irqs() runs in
the noirq phase, before the .resume callback, so:

ravb_interrupt()/ravb_multi_interrupt()
	if (iss & ISS_CGIS)
		ravb_ptp_interrupt(ndev);

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_interrupt() {
	gis &= ravb_read(ndev, GIC);
	...
	if (gis & GIS_PTMF) {
		...
		ravb_ptp_update_compare(priv, perout->target);
	}
	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
}

can run while ravb_wol_restore() is driving the block through
CCC_OPC_RESET and config mode:

drivers/net/ethernet/renesas/ravb_main.c:ravb_wol_restore() {
	/* Set reset mode to rearm the WoL logic. */
	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	...
	error = ravb_set_config_mode(ndev);
	...
}

Neither ravb_set_opmode() nor ravb_set_config_mode() takes priv->lock, so
is there anything serializing that window? Related, does the PHC stay
registered across a sleep in which the block is subsequently reset?

This also looks like it reverses commit b07bc55cbb1c ("net: ravb: Simplify
ravb_suspend()"), which moved ravb_ptp_stop() into ravb_wol_setup()
specifically because the WoL branch bypasses ravb_close(). Checking the end
of this series (up to "net: ethernet: ravb: Add gPTP support for Gen4"),
ravb_wol_setup() still has no gPTP stop, so no later patch restores it.

Would it work to keep the gPTP masking and IRQ draining in
ravb_wol_setup()? Dropping the ravb_ptp_init() from ravb_wol_restore()
below looks fine on its own, since ravb_close() there already stops gPTP.

> @@ -3204,9 +3201,6 @@ static int ravb_wol_restore(struct net_device *ndev)
>  	if (error)
>  		return error;
>  
> -	if (priv->info->ccc_gac)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	if (info->nc_queues)
>  		napi_enable(&priv->napi[RAVB_NC]);
>  	napi_enable(&priv->napi[RAVB_BE]);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 02/10] net: ethernet: ravb: Move programming of gPTP timer interval

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] In ravb_ptp_init() (drivers/net/ethernet/renesas/ravb_ptp.c),
  the newly moved gPTP timer-interval programming polls the wrong GCCR…
- [Medium] The commit is presented as relocating existing programming
  ("moving the programming to the gPTP initialization function"), but it…

--

net: ethernet: ravb: Move programming of gPTP timer interval

This moves the gPTP timer interval programming out of ravb_set_gti() in
ravb_main.c and into ravb_ptp_init(), where it is now done under
priv->lock, and takes priv->ptp.default_addend from priv->gti_tiv instead
of reading GTI back from the hardware.

A couple of questions below about the wait that follows the load request,
and about the callers of ravb_ptp_init() other than ravb_open().

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 36173a09e2fdb..a89f6fe7866fe 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1846,20 +1846,6 @@ static int ravb_set_config_mode(struct net_device *ndev)
>  	return error;
>  }
>  
> -static void ravb_set_gti(struct net_device *ndev)
> -{
> -	struct ravb_private *priv = netdev_priv(ndev);
> -	const struct ravb_hw_info *info = priv->info;
> -
> -	if (!(info->gptp || info->ccc_gac))
> -		return;
> -
> -	ravb_write(ndev, priv->gti_tiv, GTI);
> -
> -	/* Request GTI loading */
> -	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> -}
> -

[ ... ]

> @@ -1974,8 +1960,6 @@ static int ravb_open(struct net_device *ndev)
>  
>  	ravb_emac_init(ndev);
>  
> -	ravb_set_gti(ndev);
> -
>  	/* Initialise PTP Clock driver */
>  	if (info->gptp || info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 43218bc15b151..157fb78209c78 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -321,11 +321,20 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
> -	priv->ptp.default_addend = ravb_read(ndev, GTI);
> +	priv->ptp.default_addend = priv->gti_tiv;
>  	priv->ptp.current_addend = priv->ptp.default_addend;
>  
>  	spin_lock_irqsave(&priv->lock, flags);
> +
> +	/* Set gPTP Timer Increment Value. */
> +	ravb_write(ndev, priv->ptp.default_addend, GTI);

[Severity: Medium]

Before this patch ravb_set_gti() was called only from ravb_open(), so GTI
was programmed once per open.  ravb_ptp_init() has two other callers that
run on a live interface:

  ravb_set_ringparam()
      if (netif_running(ndev)) {
          ...
          if (info->gptp)
              ravb_ptp_init(ndev, priv->pdev);

  ravb_tx_timeout_work()
  out:
      /* Initialise PTP Clock driver */
      if (info->gptp)
          ravb_ptp_init(ndev, priv->pdev);

Both of those paths previously only read the live GTI back into
priv->ptp.default_addend and left the hardware increment alone.  With the
write moved here they now reprogram GTI with the nominal priv->gti_tiv and
assert GCCR_LTI.

Does that mean an ethtool -G ring resize, or a TX watchdog recovery, now
discards whatever frequency correction ravb_ptp_adjfine() had programmed?

  ravb_ptp_adjfine()
      addend = (u32)adjust_by_scaled_ppm(priv->ptp.default_addend,
                                        scaled_ppm);

The commit message describes this as moving the programming, so this extra
effect on the ringparam and tx-timeout paths is not visible from the
changelog.  Could it be mentioned there if it is intentional?

> +
> +	/* Request GTI loading. */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Wait for GIT loading to complete. */
>  	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
> +
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>  	spin_unlock_irqrestore(&priv->lock, flags);

[Severity: Medium]

Does this ravb_wait() actually observe the GTI load?  The fields in
ravb.h look disjoint:

  GCCR_TCR         = 0x00000003,
  GCCR_TCR_NOREQ   = 0x00000000, /* No request */
  GCCR_TCR_RESET   = 0x00000001, /* gPTP/AVTP presentation timer reset */
  GCCR_TCR_CAPTURE = 0x00000003, /* Capture value set in GCCR.TCSS */
  GCCR_LTO         = 0x00000004,
  GCCR_LTI         = 0x00000008,

GCCR_TCR is the timer capture/reset request field, while the increment load
request just asserted is GCCR_LTI.  So the wait returns immediately as long
as no capture or reset request is outstanding, and the load itself is never
confirmed.

The driver's own handling of the same register elsewhere treats LTI as the
load-pending bit:

  ravb_ptp_adjfine()
      gccr = ravb_read(ndev, GCCR);
      if (gccr & GCCR_LTI) {
          spin_unlock_irqrestore(&priv->lock, flags);
          return -EBUSY;
      }
      ravb_write(ndev, addend & GTI_TIV, GTI);
      ravb_write(ndev, gccr | GCCR_LTI, GCCR);

Would ravb_wait(ndev, GCCR, GCCR_LTI, 0) be the condition intended here?
As written, the following read-modify-write

  ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);

can read GCCR with LTI still set and write it back, re-arming a second load
while one is in flight, and the new GTI write is issued without the
LTI-pending check that ravb_ptp_adjfine() makes.

Note that the register sequence itself matches the pre-patch code, where
ravb_set_gti() wrote GTI plus GCCR_LTI and ravb_ptp_init() then did the same
TCR wait and TCSS read-modify-write, so the re-assertion window and the
ignored ravb_wait() return value are not new here.  What is new is the
comment, and the changelog sentence that the move is based on:

    Having split the two apart this can be improved further by moving the
    programming to the gPTP initialization function, as the first action of
    the gPTP init function is to wait for the timer interval programming to
    be acknowledge by the hardware.

Since the wait polls GCCR_TCR and not GCCR_LTI, is that rationale accurate?

Also in the same comment, GIT looks like a typo for GTI, and "acknowledge"
in the changelog should be "acknowledged".

This is still the same code at the end of the series
(941e688417745c4877c88108bd047381e5c6e533 keeps the identical
ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ) under the same comment), and
priv->ptp.default_addend = priv->gti_tiv followed by the GTI write and LTI
request is unchanged there too.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 03/10] net: ethernet: ravb: Simplify gPTP start and stop

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 0 · Low: 0

- [High] ravb_open() leaks the registered PTP clock for info->ccc_gac
  devices (R-Car Gen3/Gen4) when ravb_phy_start() fails.
- [High] Folding ravb_ptp_stop() into the tail of ravb_stop_dma()
  (ravb_main.c:1126-1128) makes gPTP teardown conditional on the AVB-…

--

net: ethernet: ravb: Simplify gPTP start and stop

This moves the info->gptp gPTP clock start and stop into ravb_dmac_init()
and ravb_stop_dma() so the gPTP clock follows the AVB-DMAC, and drops the
info->gptp part of the combined "gptp || ccc_gac" conditions in
ravb_open() and ravb_close().  The commit message states "There are no
functional change".  Two behaviour differences below look like they may
not hold up.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a89f6fe7866fe..2d0022a4f85c3 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -707,7 +707,15 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Setting the control will start the AVB-DMAC process. */
> -	return ravb_set_opmode(ndev, CCC_OPC_OPERATION);
> +	error = ravb_set_opmode(ndev, CCC_OPC_OPERATION);
> +	if (error)
> +		return error;
> +
> +	/* Initialise PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_init(ndev, priv->pdev);
> +
> +	return 0;
>  }
>  
>  static void ravb_get_tx_tstamp(struct net_device *ndev)
> @@ -1115,6 +1123,10 @@ static int ravb_stop_dma(struct net_device *ndev)
>  			netdev_err(ndev, "failed to stop AXI BUS\n");
>  	}
>  
> +	/* Stop PTP Clock driver */
> +	if (info->gptp)
> +		ravb_ptp_stop(ndev);
> +

[Severity: High]

Is the gPTP stop still guaranteed to run for info->gptp parts?  Placing it
here puts it behind the three ravb_wait() timeouts earlier in
ravb_stop_dma():

	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;

	error = ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3,
			  0);
	if (error)
		return error;
	...
	error = ravb_wait(ndev, CSR, CSR_RPO, 0);
	if (error)
		return error;

Before this patch every caller ran the stop unconditionally before
ravb_stop_dma(), so a wedged DMAC could not skip it.  Now ravb_close()
only logs the failure and keeps going:

	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");

so on a .gptp = 1 part (ravb_gen2_hw_info, ravb_rzv2m_hw_info) the PHC
stays registered across close.  A later open then goes
ravb_open() -> ravb_dmac_init() -> ravb_ptp_init(), which registers a
second clock and overwrites the pointer without checking:

	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
	...
	WRITE_ONCE(priv->ptp.clock, clock);

Does that permanently orphan the first registration?  ravb_remove() does
no PTP teardown before free_netdev(), and the orphaned clock's
ptp_clock_info lives in the netdev private area, so its ops would point
at freed memory after unbind.

Skipping ravb_ptp_stop() on those early returns also skips the interrupt
quiescing it performs:

	ravb_ptp_disable(ndev);
	ravb_ptp_sync_irqs(ndev);

ravb_close() then walks and frees priv->ts_skb_list without priv->lock,
while ravb_timestamp_interrupt() -> ravb_get_tx_tstamp() can still be
doing list_del() on the same list.  Can that corrupt the list?

There is also the reverse ordering.  If ravb_stop_dma() fails only in its
final ravb_set_opmode(ndev, CCC_OPC_CONFIG), the ravb_ptp_stop() above has
already unregistered the clock, and ravb_tx_timeout_work() takes the
"goto out" path whose ravb_ptp_init() is removed by this patch (hunk
below), leaving a running interface with no PHC until an administrative
down/up.  Same question for ravb_set_ringparam(): its

		error = ravb_stop_dma(ndev);
		if (error) {
			netdev_err(ndev,
				   "cannot set ringparam! Any AVB processes are still running?\n");
			return error;
		}

now returns with the gPTP clock still registered where previously it had
been stopped first.

>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
>  }
> @@ -1719,9 +1731,7 @@ static int ravb_set_ringparam(struct net_device *ndev,
>  
>  	if (netif_running(ndev)) {
>  		netif_device_detach(ndev);
> -		/* Stop PTP Clock driver */
> -		if (info->gptp)
> -			ravb_ptp_stop(ndev);
> +
>  		/* Wait for DMA stopping */
>  		error = ravb_stop_dma(ndev);
>  		if (error) {

[ ... ]

> @@ -1961,7 +1967,7 @@ static int ravb_open(struct net_device *ndev)
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> +	if (info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);
>  
>  	/* PHY control start */
> @@ -1974,9 +1980,6 @@ static int ravb_open(struct net_device *ndev)
>  	return 0;
>  
>  out_ptp_stop:
> -	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> -		ravb_ptp_stop(ndev);
>  	ravb_stop_dma(ndev);

[Severity: High]

Does this leak the registered PTP clock for info->ccc_gac parts?

ravb_open() still registers it just above:

	if (info->ccc_gac)
		ravb_ptp_init(ndev, priv->pdev);

and the next fallible step is ravb_phy_start(), which jumps here.  The
replacement stop folded into ravb_stop_dma() is gated on info->gptp, and
that flag is mutually exclusive with ccc_gac (ravb_gen2_hw_info and
ravb_rzv2m_hw_info set .gptp = 1, ravb_gen3_hw_info and ravb_gen4_hw_info
set .ccc_gac = 1), so nothing unregisters the clock on this path.

ravb_close() is the only remaining ccc_gac ravb_ptp_stop() site, and it is
not reached because ndo_open returned an error.  The device is then put in
CCC_OPC_RESET and runtime suspended while /dev/ptpN stays alive with
ptp_clock_info pointing into the netdev private area.  Each further
"ip link set ethX up" attempt registers another clock and orphans the
previous one via the WRITE_ONCE(priv->ptp.clock, clock) in
ravb_ptp_init(), and ravb_remove() frees the netdev without unregistering
anything.

>  out_set_reset:
>  	ravb_set_opmode(ndev, CCC_OPC_RESET);

[ ... ]

> @@ -2020,10 +2023,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  
>  	netif_tx_stop_all_queues(ndev);
>  
> -	/* Stop PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_stop(ndev);
> -
>  	/* Wait for DMA stopping */
>  	if (ravb_stop_dma(ndev)) {
>  		/* If ravb_stop_dma() fails, the hardware is still operating

[ ... ]

> @@ -2056,10 +2055,6 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	ravb_emac_init(ndev);
>  
>  out:
> -	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev, priv->pdev);
> -
>  	netif_tx_start_all_queues(ndev);
>  
>  out_unlock:
> @@ -2374,7 +2369,7 @@ static int ravb_close(struct net_device *ndev)
>  	}
>  
>  	/* Stop PTP Clock driver */
> -	if (info->gptp || info->ccc_gac)
> +	if (info->ccc_gac)
>  		ravb_ptp_stop(ndev);
>  
>  	/* Set the config mode to stop the AVB-DMAC's processes */

Both concerns come down to the same question: since ravb_stop_dma() can
return early, and its callers treat that failure differently, is the
folded start/stop still symmetric on every exit path, or would performing
the stop before the ravb_wait() loops (or leaving it at the call sites)
keep the previous behaviour?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 2 · High: 0 · Medium: 2 · Low: 1

- [Critical] NULL pointer dereference reachable by any unprivileged local
  user via `ethtool -T ethX`.
- [Critical] Dangling PTP clock pointer leading to use-after-free and
  double unregister.
- [Medium] Undisclosed revert of two in-tree stable-tagged fixes.
- [Medium] ravb_ptp_init() publishes the raw result of
  ptp_clock_register() into the shared, hardirq-visible priv->ptp.clock…
- [Low] Concern (baseline decision: false positive after series
  validation): ravb.h now declares `int ravb_ptp_init(struct net_device…

--

net: ethernet: ravb: Propagate error from ptp_clock_register()

ravb_ptp_init() is changed from void to int so that a failed
ptp_clock_register() can be reported to its callers. Along the way the
cached priv->ptp.phc_index field is removed and ravb_get_ts_info() reads
the index directly from priv->ptp.clock.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1

A few questions below.

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index aa45f5466001d..0c122a8158409 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1028,7 +1028,6 @@ struct ravb_ptp_perout {
>  struct ravb_ptp {
>  	struct ptp_clock *clock;
>  	struct ptp_clock_info info;
> -	int phc_index;
>  	u32 default_addend;
>  	u32 current_addend;
>  	int extts[N_EXT_TS];

[Severity: Medium]
The commit message says the change is about propagating the error from
ptp_clock_register(), but this hunk plus the ravb_ptp_stop() hunk below
also back out two in-tree fixes.  Could the changelog describe that?

The phc_index field being removed here was added by 1f77af0aaf27 ("net:
ravb: avoid dereferencing an invalid PTP clock"), and the
xchg(&priv->ptp.clock, NULL) removed from ravb_ptp_stop() came from
1cb9663789c5 ("net: ravb: serialize PTP clock teardown").  Both carry
Fixes: a0d2f20650e8 and Cc: stable.

The note "Rework to remove the caching of phc_index added by LLM without
access to hardware" is below the --- cut line, so it does not end up in
git history.

There is also a new side effect that is not mentioned: ravb_ptp_init()
now calls ravb_ptp_stop() itself on the registration failure path, so it
writes GIC=0/GIS=0 and runs synchronize_irq() before returning.

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index e44d065f77cc8..7c7b3ac81e5d6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = READ_ONCE(priv->ptp.phc_index);
> +		info->phc_index = ptp_clock_index(priv->ptp.clock);
>  	}
>  
>  	return 0;

[Severity: Critical]
Can this dereference NULL when the interface has never been opened?

The only guard in ravb_get_ts_info() is the static hardware test:

	if (hw_info->gptp || hw_info->ccc_gac) {
		...
		info->phc_index = ptp_clock_index(priv->ptp.clock);
	}

priv->ptp.clock lives in netdev_priv() and is zeroed at allocation.  It
is only assigned by ravb_ptp_init(), which runs from ravb_dmac_init()
(info->gptp) or ravb_open() (info->ccc_gac), i.e. not before the first
ndo_open.  With CONFIG_PTP_1588_CLOCK=y, ptp_clock_index() in
drivers/ptp/ptp_clock.c is an unconditional dereference:

	int ptp_clock_index(struct ptp_clock *ptp)
	{
		return ptp->index;
	}

ETHTOOL_GET_TS_INFO is in the "allow some commands to be done by anyone"
list in dev_ethtool(), and dev_ethtool() only requires
netif_device_present(), not netif_running(), so on a gen2/gen3/RZ-V2M
board "ethtool -T eth0" on a down interface looks reachable by any local
user.

The second way to get here with a NULL pointer is the failure this patch
is about: ravb_ptp_init() stores NULL on registration failure and both
call sites in this commit discard the return value, so the interface is
brought up with priv->ptp.clock == NULL.

The other readers in the driver, ravb_ptp_interrupt() and
ravb_ptp_stop(), still NULL-check this field, so the new site looks like
it breaks the in-driver invariant.  Should ravb_get_ts_info() keep a
sentinel of -1 for "no PHC" as before?

> @@ -2934,7 +2934,6 @@ static int ravb_probe(struct platform_device *pdev)
>  	priv->rstc = rstc;
>  	priv->ndev = ndev;
>  	priv->pdev = pdev;
> -	priv->ptp.phc_index = -1;
>  	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
>  	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
>  	if (info->nc_queues) {

diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index f70b616ac847c..19d039b9d24a5 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -313,11 +313,11 @@ void ravb_ptp_interrupt(struct net_device *ndev)
>  	ravb_write(ndev, ~(gis | GIS_RESERVED), GIS);
>  }
>  
> -void ravb_ptp_init(struct net_device *ndev)
> +int ravb_ptp_init(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
>  	unsigned long flags;
> +	int ret = 0;
>  
>  	priv->ptp.info = ravb_ptp_info;
>  
> @@ -338,15 +338,14 @@ void ravb_ptp_init(struct net_device *ndev)
>  	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> -	clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
> -	if (IS_ERR(clock)) {
> -		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
> -		clock = NULL;
> +	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
> +	if (IS_ERR(priv->ptp.clock)) {
> +		ret = PTR_ERR(priv->ptp.clock);
> +		priv->ptp.clock = NULL;
> +		ravb_ptp_stop(ndev);
>  	}
>  
> -	WRITE_ONCE(priv->ptp.clock, clock);
> -	if (clock)
> -		WRITE_ONCE(priv->ptp.phc_index, ptp_clock_index(clock));
> +	return ret;
>  }

[Severity: Medium]
Is it intentional that the registration result is published into the
shared field before the error pointer is normalised?

The store to priv->ptp.clock is a plain store outside priv->lock, and it
can briefly contain an ERR_PTR value until the next statements replace
it with NULL.  The lockless hardirq reader still expects "valid if
non-NULL" and still uses READ_ONCE():

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_interrupt() {
	struct ptp_clock *clock = READ_ONCE(priv->ptp.clock);
	...
	if ((gis & GIS_PTCF) && clock) {
		...
		ptp_clock_event(clock, &event);
	}
}

So the WRITE_ONCE()/READ_ONCE() pairing added by 1cb9663789c5 is now
one-sided.  Keeping the registration result in a local variable and
storing only NULL or a valid pointer, as the previous code did, would
avoid both the plain-store annotation mismatch and the transient
ERR_PTR.

[Severity: Low]
This isn't a regression in behaviour, but the netdev_err("failed to
register PTP clock: %pe") diagnostic is dropped here while both callers
in this commit still ignore the new return value:

drivers/net/ethernet/renesas/ravb_main.c:ravb_dmac_init() {
	/* Initialise PTP Clock driver */
	if (info->gptp)
		ravb_ptp_init(ndev);

	return 0;
}

drivers/net/ethernet/renesas/ravb_main.c:ravb_open() {
	/* Initialise PTP Clock driver */
	if (info->ccc_gac)
		ravb_ptp_init(ndev);
}

The later patch "net: ethernet: ravb: Replace gPTP flags with callbacks"
does consume the error at both sites (dmac_start() and ndev_open()), so
the ignored return is only an intermediate state.  In this intermediate
commit a registration failure is however quieter than before, since
nothing prints anything at all.  Should the message be kept until the
callers report the error?

>  
>  static void ravb_ptp_disable(struct net_device *ndev)
> @@ -369,14 +368,10 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
>  void ravb_ptp_stop(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
> -
> -	WRITE_ONCE(priv->ptp.phc_index, -1);
> -	clock = xchg(&priv->ptp.clock, NULL);
>  
>  	ravb_ptp_disable(ndev);
>  	ravb_ptp_sync_irqs(ndev);
>  
> -	if (clock)
> -		ptp_clock_unregister(clock);
> +	if (priv->ptp.clock)
> +		ptp_clock_unregister(priv->ptp.clock);
>  }

[Severity: Critical]
Does this leave priv->ptp.clock pointing at freed memory?

ptp_clock_unregister() ends with:

drivers/ptp/ptp_clock.c:ptp_clock_unregister() {
	...
	/* The final put, normally here, will invoke ptp_clock_release(). */
	put_device(&ptp->dev);
}

so the struct ptp_clock is released, but the field keeps the old value
because the xchg() to NULL is gone.  Two consequences look possible.

First, a use-after-free read.  After a normal close of a gen3 interface
(ravb_close() -> ravb_ptp_stop()), an unprivileged "ethtool -T eth0"
reaches the new ravb_get_ts_info() -> ptp_clock_index(priv->ptp.clock)
and reads ->index out of freed memory, returning it to userspace as
phc_index.

Second, a double unregister on gptp hardware, where ravb_stop_dma()
tears the clock down:

drivers/net/ethernet/renesas/ravb_main.c:ravb_stop_dma() {
	/* Stop PTP Clock driver */
	if (info->gptp)
		ravb_ptp_stop(ndev);
}

If the following ravb_dmac_init() fails, ravb_ptp_init() is not re-run:

drivers/net/ethernet/renesas/ravb_main.c:ravb_tx_timeout_work() {
	error = ravb_dmac_init(ndev);
	if (error) {
		...
		goto out_unlock;
	}
}

ravb_set_ringparam() has the same early-return shape.  A later
ravb_close() -> ravb_stop_dma() -> ravb_ptp_stop() then sees the stale
non-NULL pointer and calls ptp_clock_unregister() a second time on the
released object, where ptp_vclock_in_use(), get_device(&ptp->dev) and
kthread_destroy_worker() all touch freed memory.

Before this change the xchg() made ravb_ptp_stop() idempotent and made
both sequences impossible.  Would storing NULL after unregistering, or
restoring the xchg(), be preferable?

Neither of these appears to be addressed by the rest of the series;
ravb_ptp_stop() is unchanged at the end of the series and
ravb_gen2_ptp_clock_index() still does "return ptp_clock_index(priv->ptp.clock)".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 06/10] net: ethernet: ravb: Replace gPTP flags with callbacks

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 1 · High: 2 · Medium: 2 · Low: 0 (1)

- [Critical] ravb_get_ts_info() unconditionally dereferences
  priv->ptp.clock via ptp_clock_index() whenever the hardware table…
- [High] For Gen3 (and Gen4, which shares ravb_gen3_ptp_info in this
  commit) the PTP clock is registered by info->ptp->ndev_open()…
- [High] ravb_ptp_stop() does not clear priv->ptp.clock, so the stop
  callback is not idempotent: a second invocation without an intervening…
- [Medium] ravb_dmac_init() now returns the error from
  info->ptp->dmac_start() (ravb_ptp_init() for Gen2/RZ-V2M), creating a…
- [Medium] The commit message presents the change as a mechanical
  refactor ("replacing the flags for Gen2 behavior (info->gptp) and Gen3…

Pre-existing issues:
- [Low] In ravb_ptp_init() the GTI load sequence requests loading with
  GCCR_LTI (0x8) but the following wait — commented "Wait for GIT…

--

net: ethernet: ravb: Replace gPTP flags with callbacks

The gPTP behavior flags info->gptp (Gen2) and info->ccc_gac (Gen3) are
replaced by a new struct ravb_gptp_info holding callbacks for config mode
entry, DMAC start/stop and ndev open/close.  Each hardware info table now
points at a per-generation callback table.  This prepares for adding Gen4
support.

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 0c122a8158409..6603f90b19e15 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7c7b3ac81e5d6..5d5b9c0e2f450 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -712,8 +712,8 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->dmac_start)
> +		return info->ptp->dmac_start(ndev);
>  
>  	return 0;
>  }

[Severity: Medium]
Can this leak the descriptor rings and page pools?

For Gen2 and RZ/V2M, .dmac_start is ravb_ptp_init(), which can fail with
the PTR_ERR() from ptp_clock_register().  That error is now returned from
ravb_dmac_init() after info->dmac_init() already allocated the page pools,
rx_buffers, tx_skb arrays, aligned TX buffers and the coherent descriptor
rings, and after the DMAC was switched to CCC_OPC_OPERATION.

In ravb_open() that error goes to out_set_reset:

out_set_reset:
	ravb_set_opmode(ndev, CCC_OPC_RESET);

so ravb_stop_dma() is skipped and ravb_ring_free() is never called.  Since
ndo_open failed, ravb_close() will not run either, and the next open
re-runs ravb_ring_init() which overwrites priv->rx_pool[], priv->rx_buffers[],
priv->tx_skb[], priv->rx_ring[] and priv->tx_ring[] unconditionally, so the
previous allocations can never be freed.

The other two callers see the same new late failure.  ravb_set_ringparam()
returns the error with the netdev still detached and the DMAC in operation
mode, and ravb_tx_timeout_work() returns with all TX queues stopped, which
seems to contradict its own comment:

	/* If ravb_dmac_init() fails, descriptors are freed. So, this
	 * should return here to avoid re-enabling the TX and RX in
	 * ravb_emac_init().
	 */

Would it make sense to roll back inside ravb_dmac_init() when the gPTP start
callback fails (stop the DMA, return to CONFIG mode and free the rings) so
that the documented post-condition of a failed ravb_dmac_init() still holds
for every generation?

[Severity: Medium]
The commit message describes this as replacing "the flags for Gen2 behavior
(info->gptp) and Gen3 behavior (info->ccc_gac) with callbacks", which reads
as a mechanical refactor.

The patch also changes failure behaviour.  Before, both call sites discarded
the ravb_ptp_init() return value:

	if (info->gptp)
		ravb_ptp_init(ndev);

	return 0;

Now ravb_dmac_init() propagates it for Gen2/RZ-V2M, and ravb_open() aborts
when the Gen3/Gen4 .ndev_open callback fails, so a PTP clock registration
failure turns into a failed ndo_open (and a failed ethtool ring resize or
TX timeout recovery) where it previously left the interface fully usable.

Only the v2 changelog below the '---' line mentions this ("Handle error from
starting the PTP clock"), and that text is dropped when the patch is applied.
Could the commit message itself mention the behaviour change?

> @@ -1124,8 +1124,8 @@ static int ravb_stop_dma(struct net_device *ndev)
>  	}
>  
>  	/* Stop PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_stop(ndev);
> +	if (info->ptp && info->ptp->dmac_stop)
> +		info->ptp->dmac_stop(ndev);
>  
>  	/* Stop AVB-DMAC process */
>  	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);

[Severity: High]
Can this end up calling ptp_clock_unregister() twice on the same clock?

ravb_ptp_stop() no longer clears priv->ptp.clock:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_stop() {
	...
	if (priv->ptp.clock)
		ptp_clock_unregister(priv->ptp.clock);
}

so the callback is not idempotent.  With .dmac_stop = ravb_ptp_stop for Gen2
and RZ/V2M, ravb_stop_dma() is reachable from ravb_close(),
ravb_set_ringparam(), ravb_tx_timeout_work() and the ravb_open() error path,
and two stops with no ravb_ptp_init() in between look possible:

ethtool -G eth0 rx 4096
  ravb_set_ringparam()
    ravb_stop_dma()
      info->ptp->dmac_stop()   -> ravb_ptp_stop() -> ptp_clock_unregister()
                                  priv->ptp.clock left dangling
    ravb_dmac_init()           -> fails (ravb_ring_init() -ENOMEM, or
                                  ravb_set_opmode() timeout), so dmac_start
                                  and ravb_ptp_init() never run
    return error               -> netdev still running

ip link set eth0 down
  ravb_close()
    ravb_stop_dma()
      info->ptp->dmac_stop()   -> ravb_ptp_stop()
                                  if (priv->ptp.clock) still true
                                  ptp_clock_unregister() on freed memory

The same interleaving looks reachable through ravb_tx_timeout_work(), where a
successful ravb_stop_dma() is followed by a failing ravb_dmac_init() and
'goto out_unlock' with the interface still up.

The xchg(&priv->ptp.clock, NULL) that used to make this idempotent was
removed by the earlier series commit "net: ethernet: ravb: Propagate error
from ptp_clock_register()", and ravb_ptp_stop() is still non-idempotent at
the end of the series.  Should priv->ptp.clock be cleared in
ravb_ptp_stop()?

> @@ -1774,7 +1774,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *hw_info = priv->info;
>  
> -	if (hw_info->gptp || hw_info->ccc_gac) {
> +	if (hw_info->ptp) {
>  		info->so_timestamping =

[Severity: Critical]
Is this guard sufficient for the ptp_clock_index() call in the body?

hw_info->ptp is a static hardware capability, but the body still does:

	info->phc_index = ptp_clock_index(priv->ptp.clock);

and with CONFIG_PTP_1588_CLOCK enabled that is:

drivers/ptp/ptp_clock.c:ptp_clock_index() {
	return ptp->index;
}

priv->ptp.clock is NULL until the interface is opened (Gen3/Gen4 register in
the .ndev_open callback, Gen2/RZ-V2M in .dmac_start), and it is left dangling
after ravb_ptp_stop() because that function no longer clears it.  So:

ethtool -T eth0        # device probed, never brought up
  ravb_get_ts_info()
    hw_info->ptp != NULL   # all ravb tables set .ptp
    ptp_clock_index(NULL)  # NULL dereference

ip link set eth0 up; ip link set eth0 down; ethtool -T eth0
  ravb_get_ts_info()
    ptp_clock_index(<freed ptp_clock>)  # value returned in phc_index

ETHTOOL_GET_TS_INFO is listed in net/ethtool/ioctl.c among the commands that
fall through before the ns_capable(CAP_NET_ADMIN) check, so this is reachable
by an unprivileged local user.

This is still broken at the end of the series, where ravb_get_ts_info() calls
hw_info->ptp->clock_index() and ravb_gen2_ptp_clock_index() is just
'return ptp_clock_index(priv->ptp.clock);'.  The cached priv->ptp.phc_index
(reset to -1 in ravb_ptp_stop()) plus the xchg() clearing of the pointer that
used to make this safe were removed by the earlier series commit "net:
ethernet: ravb: Propagate error from ptp_clock_register()".  Should the
phc_index only be reported when a clock is actually registered?

[ ... ]

> @@ -1967,8 +1957,11 @@ static int ravb_open(struct net_device *ndev)
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->ccc_gac)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->ndev_open) {
> +		error = info->ptp->ndev_open(ndev);
> +		if (error)
> +			goto out_ptp_stop;
> +	}
>  
>  	/* PHY control start */
>  	error = ravb_phy_start(ndev);

[Severity: High]
Does the out_ptp_stop label still stop the PTP clock for Gen3 and Gen4?

For ravb_gen3_ptp_info (which ravb_gen4_hw_info also uses in this commit) the
clock is registered from .ndev_open = ravb_ptp_init, and the only teardown is
.ndev_close = ravb_ptp_stop.  The label reachable from here is:

out_ptp_stop:
	ravb_stop_dma(ndev);
out_set_reset:
	ravb_set_opmode(ndev, CCC_OPC_RESET);

and ravb_stop_dma() only calls dmac_stop:

	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);

.dmac_stop is NULL in ravb_gen3_ptp_info, so ravb_ptp_stop() never runs.  If
ravb_phy_start() fails after a successful ndev_open, ndo_open returns an error
with the PHC still registered while the DMAC is forced to CCC_OPC_RESET and
the device is runtime suspended.  ndo_stop is not called for a failed open, so
nothing ever calls ndev_close for that registration, and each retried open
re-registers over priv->ptp.clock.

ravb_remove() does no PTP teardown:

	of_node_put(priv->of_gptp);
	reset_control_assert(priv->rstc);
	free_netdev(ndev);

Since ptp_clock_register() keeps the caller's struct ptp_clock_info pointer
(priv->ptp.info, inside the netdev private data), can the orphaned /dev/ptpN
outlive free_netdev() and dereference freed memory on a later ioctl?

The symmetric teardown existed at the series baseline as
'if (info->gptp || info->ccc_gac) ravb_ptp_stop(ndev);' at this label and was
dropped by the earlier series commit "net: ethernet: ravb: Simplify gPTP start
and stop"; the label is still just ravb_stop_dma() at the end of the series.
Should the open error path call info->ptp->ndev_close()?  The failing
ndev_open case itself is already cleaned up inside ravb_ptp_init().

[ ... ]

One more observation, unrelated to the diff itself:

[Severity: Low]
This isn't a bug introduced by this patch, but the GTI load sequence in
ravb_ptp_init() waits on the wrong field:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_init() {
	...
	/* Request GTI loading. */
	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);

	/* Wait for GIT loading to complete. */
	ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
}

GCCR_TCR and GCCR_LTI are distinct fields:

drivers/net/ethernet/renesas/ravb.h:
	GCCR_TCR	= 0x00000003,
	...
	GCCR_LTI	= 0x00000008,

so the wait cannot observe LTI completion, and ravb_wait()'s return value is
discarded as well.  The worst effect I can see is a transient unloaded timer
increment or an -EBUSY from a very early ravb_ptp_adjfine(), which treats a
set GCCR_LTI as busy.

The pre-existing baseline had no LTI poll at all in ravb_set_gti(); the
misleading comment came from the earlier series commit "net: ethernet: ravb:
Move programming of gPTP timer interval", whose message also states that the
init function waits for the interval programming to be acknowledged.  Should
the wait poll GCCR_LTI and check the result?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 07/10] net: ethernet: ravb: Add callback for gPTP probe

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 1 · High: 2 · Medium: 1 · Low: 0

- [Critical] ravb_get_ts_info()
  (drivers/net/ethernet/renesas/ravb_main.c:1771-1790) gates on…
- [High] ravb_ptp_stop()
  (drivers/net/ethernet/renesas/ravb_ptp.c:368-377) unregisters…
- [High] For the ccc_gac-style generations the PTP clock is registered
  from ravb_open() via info->ptp->ndev_open (ravb_main.c:1953-1956,…
- [Medium] The newly propagated ptp_clock_register() error creates
  ravb_open() failure paths that leak all DMA ring resources.

--

net: ethernet: ravb: Add callback for gPTP probe

This adds a probe callback to struct ravb_gptp_info so each RAVB
generation can acquire its own gPTP timer clock, removes the last gPTP
specific flag (gptp_ref_clk), and moves the GTI computation into the new
per-generation probe callbacks.

A few questions below about the gPTP clock lifecycle that the ptp callback
structs in this patch select.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5d5b9c0e2f450..7e53eca5327f5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c

[ ... ]

> @@ -2668,6 +2668,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
>  }
>  
>  static const struct ravb_gptp_info ravb_gen2_ptp_info = {
> +	.probe = ravb_gen2_ptp_probe,
>  	.set_config_mode = ravb_gen2_ptp_set_config_mode,
>  	.dmac_start = ravb_ptp_init,
>  	.dmac_stop = ravb_ptp_stop,
>  };

[Severity: High]
With dmac_stop = ravb_ptp_stop wired up here, can priv->ptp.clock end up
pointing at a freed object after the interface goes down?

ravb_ptp_stop() in ravb_ptp.c unregisters the clock but never clears the
pointer:

	if (priv->ptp.clock)
		ptp_clock_unregister(priv->ptp.clock);

The baseline cleared it before unregistering:

	WRITE_ONCE(priv->ptp.phc_index, -1);
	clock = xchg(&priv->ptp.clock, NULL);

After ip link set <if> down on Gen2/RZ-V2M, ravb_close() -> ravb_stop_dma()
-> dmac_stop = ravb_ptp_stop() leaves the stale pointer, and
ravb_get_ts_info() then reads the freed memory:

	info->phc_index = ptp_clock_index(priv->ptp.clock);

drivers/ptp/ptp_clock.c:ptp_clock_index() is just:

	return ptp->index;

ETHTOOL_GET_TS_INFO is in the list in dev_ethtool() that skips the
ns_capable(CAP_NET_ADMIN) check, so this read is reachable by any local
user once the interface is down.

Can the same stale pointer also be unregistered twice? In
ravb_tx_timeout_work(), ravb_stop_dma() runs ravb_ptp_stop() (unregister),
and if the following ravb_dmac_init() fails (ravb_ring_init() returning
-ENOMEM, or the OPERATION opmode wait timing out) the function returns via
out_unlock without re-registering, so the later ravb_close() ->
ravb_stop_dma() -> ravb_ptp_stop() calls ptp_clock_unregister() on the
already freed clock.

This was introduced earlier in the series by "net: ethernet: ravb:
Propagate error from ptp_clock_register()" and ravb_ptp_stop() is still
unchanged at the end of the series (941e688), where
ravb_gen2_ptp_clock_index() likewise still does
return ptp_clock_index(priv->ptp.clock).

> @@ -2704,6 +2705,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
>  }
>  
>  static const struct ravb_gptp_info ravb_gen3_ptp_info = {
> +	.probe = ravb_gen2_ptp_probe,
>  	.set_config_mode = ravb_gen3_ptp_set_config_mode,
>  	.ndev_open = ravb_ptp_init,
>  	.ndev_close = ravb_ptp_stop,
>  };

[Severity: High]
Since ravb_gen3_ptp_info registers the clock from ndev_open and only tears
it down from ndev_close, is the clock leaked when ravb_open() fails after
that registration?

ravb_open() calls info->ptp->ndev_open() and then ravb_phy_start(), whose
failure path is:

out_ptp_stop:
	ravb_stop_dma(ndev);

ravb_stop_dma() only stops PTP through info->ptp->dmac_stop, and
ravb_gen3_ptp_info has no dmac_stop, so nothing unregisters the clock.
ravb_gen4_hw_info uses ravb_gen3_ptp_info at this commit as well.

ravb_phy_start() can fail concretely: ravb_phy_init() returns -ENOENT when
of_phy_connect() fails, and it also propagates
of_phy_register_fixed_link() errors. As ndo_open returned an error the
netdev never goes IFF_UP, so ndo_stop and therefore ndev_close =
ravb_ptp_stop() are never called.

Consequences of that: /dev/ptpN stays registered; a retried open
overwrites priv->ptp.clock in ravb_ptp_init(), losing the old registration
for good; and a later unbind runs ravb_remove() -> free_netdev(ndev)
while the PTP core still has ptp->info pointing into the freed netdev
private area.

The baseline out_ptp_stop label did:

	if (info->gptp || info->ccc_gac)
		ravb_ptp_stop(ndev);

That was dropped by the series commit "net: ethernet: ravb: Simplify gPTP
start and stop", which added the stop only inside ravb_stop_dma(), so the
"no functional change" claim there does not seem to hold for the ccc_gac
(Gen3/Gen4) case. The gap is still present at 941e688, where out_ptp_stop
only calls ravb_stop_dma() and ravb_gen3_ptp_info still has no dmac_stop.

> @@ -2763,6 +2765,24 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.magic_pkt = 1,
>  };
>  
> +static int ravb_rzv2m_ptp_probe(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	priv->gptp_clk = devm_clk_get(&priv->pdev->dev, "gptp");
> +	if (IS_ERR(priv->gptp_clk))
> +		return PTR_ERR(priv->gptp_clk);
> +
> +	return ravb_compute_gti(ndev, priv->gptp_clk);
> +}
> +
> +static const struct ravb_gptp_info ravb_rzv2m_ptp_info = {
> +	.probe = ravb_rzv2m_ptp_probe,
> +	.set_config_mode = ravb_gen2_ptp_set_config_mode,
> +	.dmac_start = ravb_ptp_init,

[Severity: Medium]
With dmac_start = ravb_ptp_init, does a failing ptp_clock_register() leak
the DMA rings?

ravb_dmac_init() first runs info->dmac_init = ravb_dmac_init_rcar(), which
allocates both queues in ravb_ring_init():

	priv->rx_pool[q] = page_pool_create(&params);
	...
	priv->tx_ring[q] = dma_alloc_coherent(ndev->dev.parent, ring_size,
					      &priv->tx_desc_dma[q], GFP_KERNEL);

and only afterwards does:

	/* Initialise PTP Clock driver */
	if (info->ptp && info->ptp->dmac_start)
		return info->ptp->dmac_start(ndev);

If ptp_clock_register() in ravb_ptp_init() fails (for example -ENOMEM under
memory pressure), that error reaches ravb_open(), which jumps to
out_set_reset. Neither out_set_reset nor out_ptp_stop calls
ravb_ring_free(), and since ndo_open returned an error, ravb_close() (the
only ring freeing path) never runs; ravb_remove() does not free them
either.

A later open re-enters ravb_ring_init() and overwrites priv->rx_pool[q],
priv->rx_buffers[q], priv->tx_skb[q], priv->rx_ring[q] and
priv->tx_ring[q], so the previous page pools with their DMA mappings and
the coherent descriptor memory are lost.

For Gen3/Gen4 the equivalent ndev_open failure goes to out_ptp_stop, which
also does not call ravb_ring_free(). The same leak shape already existed
for a failing ravb_set_opmode(CCC_OPC_OPERATION); the new trigger comes
from the series commit "net: ethernet: ravb: Propagate error from
ptp_clock_register()", and the ravb_open() error labels are unchanged at
941e688.

> +	.dmac_stop = ravb_ptp_stop,
> +};
> +
>  static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.receive = ravb_rx_rcar,
>  	.set_rate = ravb_set_rate_rcar,
> @@ -2783,8 +2803,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.dbat_entry_num = 22,
>  	.multi_irqs = 1,
>  	.err_mgmt_irqs = 1,
> -	.ptp = &ravb_gen2_ptp_info,
> -	.gptp_ref_clk = 1,
> +	.ptp = &ravb_rzv2m_ptp_info,

[Severity: Critical]
hw_info->ptp is what ravb_get_ts_info() tests, and it only says the IP
supports gPTP. Can that path dereference a NULL priv->ptp.clock?

ravb_get_ts_info() does:

	if (hw_info->ptp) {
		...
		info->phc_index = ptp_clock_index(priv->ptp.clock);
	}

priv->ptp.clock is the zero initialised NULL from alloc_etherdev_mqs()
until the clock is registered, which only happens in the open lifecycle
(Gen2/RZ-V2M via ravb_dmac_init() -> dmac_start = ravb_ptp_init(),
Gen3/Gen4 via ravb_open() -> ndev_open = ravb_ptp_init()), and
ravb_ptp_init() explicitly stores NULL on registration failure:

	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &priv->pdev->dev);
	if (IS_ERR(priv->ptp.clock)) {
		ret = PTR_ERR(priv->ptp.clock);
		priv->ptp.clock = NULL;

With CONFIG_PTP_1588_CLOCK=y, ptp_clock_index() is
return ptp->index;, so ethtool -T on an interface that has never been
opened (the state right after probe and register_netdev) oopses.
ETHTOOL_GET_TS_INFO does not require CAP_NET_ADMIN in dev_ethtool(), so an
unprivileged user can reach it; only CONFIG_PTP_1588_CLOCK=n is safe
because of the inline stub returning -1.

The baseline kept a separate priv->ptp.phc_index initialised to -1 in
ravb_probe() and read it with READ_ONCE(); the series commit "net:
ethernet: ravb: Propagate error from ptp_clock_register()" removed that
field and its -1 initialisation. At 941e688 the callback
ravb_gen2_ptp_clock_index() still does
return ptp_clock_index(priv->ptp.clock) and is installed for gen2, gen3
and rzv2m.

>  	.nc_queues = 1,
>  	.magic_pkt = 1,
>  };

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 08/10] net: ethernet: ravb: Add callback for gPTP clock index

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 1 · High: 0 · Medium: 2 · Low: 0

- [Critical] NULL pointer dereference (oops) and post-close use-after-
  free read via the new `.clock_index` callback.
- [Medium] All timestamping capabilities, including the purely software
  SOF_TIMESTAMPING_TX_SOFTWARE bit and the tx_types/rx_filters masks,…
- [Medium] On Gen2 and RZ/V2M the gPTP shutdown is now reached only
  through `.dmac_stop = ravb_ptp_stop` at the tail of `ravb_stop_dma()`…

--

net: ethernet: ravb: Add callback for gPTP clock index

This adds a .clock_index callback to struct ravb_gptp_info and uses its
return value in ravb_get_ts_info() to decide whether the driver reports
gPTP support. Gen2, Gen3 and RZ/V2M all get ravb_gen2_ptp_clock_index()
wired up.

A few questions below.

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index b788e8f5260dd..27e2b31955ba9 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1042,6 +1042,7 @@ struct ravb_ptp {
>   * critical points in the RAVB driver.
>   *
>   * @probe:		Probe the gPTP clock
> + * @clock_index:	Get the PTP clock index, if any
>   * @set_config_mode:	Enter config mode
>   * @dmac_start:		Called when the DMAC starts
>   * @dmac_stop:		Called when the DMAC stops
> @@ -1050,6 +1051,7 @@ struct ravb_ptp {
>   */
>  struct ravb_gptp_info {
>  	int (*probe)(struct net_device *ndev);
> +	int (*clock_index)(struct net_device *ndev);
>  	int (*set_config_mode)(struct net_device *ndev);
>  	int (*dmac_start)(struct net_device *ndev);
>  	void (*dmac_stop)(struct net_device *ndev);

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7e53eca5327f5..402370e1be634 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1773,8 +1773,13 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *hw_info = priv->info;
> +	int index = -1;
>  
> -	if (hw_info->ptp) {
> +	if (hw_info->ptp && hw_info->ptp->clock_index)
> +		index = hw_info->ptp->clock_index(ndev);
> +
> +	/* Only advertise ptp clock if present. */
> +	if (index >= 0) {
>  		info->so_timestamping =
>  			SOF_TIMESTAMPING_TX_SOFTWARE |

[Severity: Medium]
Is it intended that the whole capability block, and not just phc_index, is
now gated on the clock index?

RAVB has "depends on PTP_1588_CLOCK_OPTIONAL" in
drivers/net/ethernet/renesas/Kconfig, so CONFIG_PTP_1588_CLOCK=n is a
supported build. In that configuration ptp_clock_index() is the stub in
include/linux/ptp_clock_kernel.h:

  static inline int ptp_clock_index(struct ptp_clock *ptp)
  { return -1; }

so ravb_gen2_ptp_clock_index() always returns -1, "if (index >= 0)" never
fires, and ravb_get_ts_info() leaves so_timestamping, tx_types and
rx_filters all at 0.

Before this change the gate was "if (hw_info->ptp)", which advertised
SOF_TIMESTAMPING_TX_SOFTWARE together with the hardware bits and the
tx_types/rx_filters masks while leaving phc_index at -1. Commit
1f77af0aaf27 ("net: ravb: avoid dereferencing an invalid PTP clock")
described that as preserving "the static timestamping capabilities".

ravb_start_xmit() still calls skb_tx_timestamp(), and
__ethtool_get_ts_info() in net/ethtool/common.c only adds the RX and
generic software bits:

  info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE |
			   SOF_TIMESTAMPING_SOFTWARE;

so SOF_TIMESTAMPING_TX_SOFTWARE disappears from ethtool -T entirely. Should
the software capability bits and the filter masks be reported
unconditionally, with only phc_index depending on the callback?

>  			SOF_TIMESTAMPING_TX_HARDWARE |
> @@ -1785,7 +1790,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = ptp_clock_index(priv->ptp.clock);
> +		info->phc_index = index;
>  	}
>  
>  	return 0;
> @@ -2653,6 +2658,13 @@ static int ravb_gen2_ptp_probe(struct net_device *ndev)
>  	return ravb_compute_gti(ndev, priv->clk);
>  }
>  
> +static int ravb_gen2_ptp_clock_index(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	return ptp_clock_index(priv->ptp.clock);
> +}
> +

[Severity: Critical]
Can this dereference priv->ptp.clock when no clock is registered?

With CONFIG_PTP_1588_CLOCK=y, ptp_clock_index() in drivers/ptp/ptp_clock.c
is an unconditional dereference:

  int ptp_clock_index(struct ptp_clock *ptp)
  {
	  return ptp->index;
  }

The PHC is only registered at open time (.dmac_start / .ndev_open =
ravb_ptp_init) and unregistered at close (.dmac_stop / .ndev_close =
ravb_ptp_stop), so priv->ptp.clock is NULL before the first open and stale
after close, since ravb_ptp_stop() no longer clears it:

drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_stop() {
	...
	if (priv->ptp.clock)
		ptp_clock_unregister(priv->ptp.clock);
}

ravb_get_ts_info() is reached from ethtool -T / ETHTOOL_MSG_TSINFO_GET, and
__ethtool_get_ts_info() in net/ethtool/common.c has no netif_running() or
CAP_NET_ADMIN gate. So a query on a never-opened interface would call
ptp_clock_index(NULL), and a query after "ip link set eth0 down" would read
memory freed by ptp_clock_unregister() and hand the result back to userspace
as phc_index. Is that reachable as written?

The "if (index >= 0)" test and the "@clock_index: Get the PTP clock index,
if any" kernel-doc suggest the callback is expected to be able to report
absence, but this implementation cannot return a negative value on
Gen2/Gen3/RZ/V2M when CONFIG_PTP_1588_CLOCK=y. The sibling helper in the
same driver family does carry the guard:

drivers/net/ethernet/renesas/rcar_gen4_ptp.c:rcar_gen4_ptp_clock_index() {
	if (!priv->initialized)
		return -1;

	return ptp_clock_index(priv->clock);
}

Note the baseline before this series cached the value instead
(info->phc_index = READ_ONCE(priv->ptp.phc_index), initialised to -1 in
ravb_probe() and reset to -1 in ravb_ptp_stop()) and cleared the pointer
with clock = xchg(&priv->ptp.clock, NULL). Those came from commit
1f77af0aaf27 ("net: ravb: avoid dereferencing an invalid PTP clock", Cc:
stable, Fixes: a0d2f20650e8) and commit 1cb9663789c5 ("net: ravb: serialize
PTP clock teardown", Cc: stable). The earlier patch in this series ("net:
ethernet: ravb: Propagate error from ptp_clock_register()") removed both,
and the later "net: ethernet: ravb: Add gPTP support for Gen4" only adds a
registration-state guard for Gen4, so Gen2/Gen3/RZ/V2M still look exposed at
the end of the series. Would a registration-state check here, plus clearing
priv->ptp.clock in ravb_ptp_stop(), restore the previous behaviour?

>  static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
>  {
>  	int ret;
> @@ -2669,6 +2681,7 @@ static int ravb_gen2_ptp_set_config_mode(struct net_device *ndev)
>  
>  static const struct ravb_gptp_info ravb_gen2_ptp_info = {
>  	.probe = ravb_gen2_ptp_probe,
> +	.clock_index = ravb_gen2_ptp_clock_index,
>  	.set_config_mode = ravb_gen2_ptp_set_config_mode,
>  	.dmac_start = ravb_ptp_init,
>  	.dmac_stop = ravb_ptp_stop,

[Severity: Medium]
Since the clock index now depends on whether the PHC is registered, is the
Gen2/RZ/V2M teardown path via .dmac_stop always taken?

On these variants ravb_ptp_stop() is only reached from the tail of
ravb_stop_dma(), after three earlier error returns:

drivers/net/ethernet/renesas/ravb_main.c:ravb_stop_dma() {
	...
	error = ravb_wait(ndev, TCCR, info->tccr_mask, 0);
	if (error)
		return error;
	...
	/* Stop PTP Clock driver */
	if (info->ptp && info->ptp->dmac_stop)
		info->ptp->dmac_stop(ndev);
	...
}

and ravb_close() continues regardless:

drivers/net/ethernet/renesas/ravb_main.c:ravb_close() {
	...
	/* Set the config mode to stop the AVB-DMAC's processes */
	if (ravb_stop_dma(ndev) < 0)
		netdev_err(ndev,
			   "device will be stopped after h/w processes are done.\n");
	...
}

If any of the TCCR / CSR(TPO*) / CSR(RPO) polls time out, the PHC stays
registered while ravb_close() goes on to free the rings, set CCC_OPC_RESET
and call pm_runtime_put_autosuspend(). Can /dev/ptpN then still drive
ravb_ptp_gettime64()/settime64()/adjfine() and issue ravb_read()/
ravb_write() against a device in reset with clocks gated?

A following "ip link set eth0 up" also runs ravb_dmac_init() ->
.dmac_start = ravb_ptp_init -> ptp_clock_register() and overwrites
priv->ptp.clock, which looks like it leaks the previously registered
ptp_clock whose ptp_clock_info points into the netdev private data.

The baseline called ravb_ptp_stop() before ravb_stop_dma() in ravb_close(),
so a DMA-stop timeout could not skip the gPTP teardown. The commit that
moved it ("net: ethernet: ravb: Simplify gPTP start and stop") states "There
are no functional change" — does that hold for this error path?

> @@ -2706,6 +2719,7 @@ static int ravb_gen3_ptp_set_config_mode(struct net_device *ndev)
>  
>  static const struct ravb_gptp_info ravb_gen3_ptp_info = {
>  	.probe = ravb_gen2_ptp_probe,
> +	.clock_index = ravb_gen2_ptp_clock_index,
>  	.set_config_mode = ravb_gen3_ptp_set_config_mode,
>  	.ndev_open = ravb_ptp_init,
>  	.ndev_close = ravb_ptp_stop,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 09/10] dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Concern (invalidated): the binding documents `ptp-timer` for Gen4
  EtherAVB while the RAVB driver allegedly never reads it and keeps…

--

dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

This adds an optional ptp-timer phandle to the EtherAVB binding so that
Gen4 SoCs can point at the external system wide gPTP clock, and adds a
conditional block rejecting the property on non-Gen4 compatibles.

> diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> index 1e00ef5b3acd..fad002492421 100644
> --- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> +++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> @@ -97,6 +97,8 @@ properties:
>  
>    phy-handle: true
>  
> +  ptp-timer: true
> +

[Severity: Low]
This isn't a bug, just a note for the record after checking the rest of
the series.

At this patch alone there is no consumer of ptp-timer in
drivers/net/ethernet/renesas/, which initially looked like the binding
was documenting a property the driver would never read.

The later patch in the series, "net: ethernet: ravb: Add gPTP support
for Gen4", does add the consumer in ravb_gen4_ptp_probe():

	priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);

and switches ravb_gen4_hw_info.ptp from &ravb_gen3_ptp_info to
&ravb_gen4_ptp_info, with the reference released in ravb_remove() via
of_node_put(priv->of_gptp).

So no change is needed here; the property is consumed by the end of the
series.

>    '#address-cells':
>      description: Number of address cells for the MDIO bus.
>      const: 1

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se

Re: [PATCH net-next v3 10/10] net: ethernet: ravb: Add gPTP support for Gen4

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 1

- [High] The new Gen4 gPTP support can never resolve a PHC index, so the
  feature is inert.
- [Medium] Gen4 hardware-timestamping gating is incomplete: only
  ravb_get_ts_info() consults the new clock_index callback, while every…
- [Medium] OF node reference leak on all probe failure paths.
- [Low] The commit fixes a user-visible defect — it states "While driver
  advertises gPTP support on Gen4 platforms it is in fact completely…

--

net: ethernet: ravb: Add gPTP support for Gen4

This adds a Gen4-specific ravb_gptp_info that resolves a system-wide gPTP
timer through a new optional "ptp-timer" phandle, points the Gen4 hardware
info at it instead of the Gen3 one, and only advertises gPTP to user space
when the referenced timer node is present and enabled.

A few questions below.

> While driver advertises gPTP support on Gen4 platforms it is in fact
> completely broken.

[Severity: Low]
Since this changes user-visible behaviour on Gen4 (ravb_get_ts_info() no
longer reports so_timestamping/tx_types/rx_filters/phc_index there), should
the commit carry a Fixes: tag so the affected range is clear for stable
backports?

One note on attribution: 6e0713cc8206 ("ravb: Add MII support for R-Car
V4M") only split ravb_gen4_hw_info out of ravb_gen3_hw_info, copying
.ccc_gac = 1 along with it, so the bogus gPTP advertisement on Gen4 is
older than that commit.  The tag would instead point at the commit that
first bound the "renesas,etheravb-rcar-gen4" compatible to the Gen3
(ccc_gac) hardware description.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 402370e1be63..66c9cd15a19e 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2752,6 +2752,57 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.magic_pkt = 1,
>  };
>  
> +static int ravb_gen4_ptp_probe(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);
> +	if (!priv->of_gptp)
> +		return 0;

[Severity: Medium]
Does this leak the "ptp-timer" device_node when probe fails later on?

of_parse_phandle() returns the node with an incremented reference count,
and ravb_probe() calls info->ptp->probe() early:

	if (info->ptp && info->ptp->probe) {
		error = info->ptp->probe(ndev);
		if (error)
			goto out_reset_assert;
	}

Everything after that can still fail: devm_clk_get_optional() for refclk,
pm_runtime_resume_and_get(), devm_platform_get_and_ioremap_resource(),
of_get_phy_mode(), the descriptor BAT dma_alloc_coherent(),
ravb_set_opmode(), ravb_mdio_init() and register_netdev().  The unwind
labels out_napi_del, out_mdio_release, out_reset_mode, out_rpm_put,
out_rpm_disable, out_reset_assert and out_free_netdev release napi, mdio,
the opmode, the BAT, runtime PM, refclk, the reset control and the netdev,
but none of them calls of_node_put(priv->of_gptp), and free_netdev(ndev) at
out_free_netdev then destroys priv.

The only put added here is in ravb_remove(), which is never reached on a
failed probe.  ravb_remove() also returns early:

	error = pm_runtime_resume_and_get(dev);
	if (error < 0)
		return;

so the reference is skipped on that path too.  With EPROBE_DEFER retry
cycles this accumulates one node reference per attempt.

> +
> +	if (!of_device_is_available(priv->of_gptp)) {
> +		of_node_put(priv->of_gptp);
> +		priv->of_gptp = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ravb_gen4_ptp_clock_index(struct net_device *ndev)
> +{
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
> +	/* If no clock, mimic ptp_clock_index_by_of_node() fail and return -1 */
> +	if (!priv->of_gptp)
> +		return -1;
> +
> +	return ptp_clock_index_by_of_node(priv->of_gptp);
> +}

[Severity: High]
Can this lookup ever succeed with the in-tree Gen4 gPTP provider?

ptp_clock_index_by_of_node() matches through class_find_device() using:

drivers/ptp/ptp_clock.c:ptp_clock_of_node_match() {
	...
	return (dev->parent && dev_of_node(dev->parent) == parent_np);
}

so the PHC must have been registered with a parent device whose OF node is
the phandle target.  The R-Car Gen4 system-wide timer is provided by
rcar_gen4_ptp.c (used by rswitch and rtsn, i.e. the two blocks the binding
patch names as sharing the Gen4 gPTP clock), and it registers with no
parent:

drivers/net/ethernet/renesas/rcar_gen4_ptp.c:rcar_gen4_ptp_register() {
	...
	ptp_priv->clock = ptp_clock_register(&ptp_priv->info, NULL);
	...
}

ptp_clock_register() then stores ptp->dev.parent = parent, so the PTP class
device has no parent and therefore no OF node, and rcar_gen4_ptp_alloc()
never keeps the pdev/device around either.

With that, ravb_gen4_ptp_clock_index() returns -1 even for a correctly
described board, and ravb_get_ts_info() takes the "index < 0" path:

	/* Only advertise ptp clock if present. */
	if (index >= 0) {

leaving so_timestamping, tx_types, rx_filters and phc_index unset, so
ethtool -T and ptp4l see nothing on Gen4.

For comparison, the same "ptp-timer" consumer pattern works for enetc
because the provider passes an owning device:

drivers/ptp/ptp_qoriq.c:ptp_qoriq_init() {
	...
	ptp_qoriq->clock = ptp_clock_register(&ptp_qoriq->caps, ptp_qoriq->dev);
	...
}

Should rcar_gen4_ptp.c pass the owning device to ptp_clock_register(), or
should a different lookup key be used here?

> +
> +static int ravb_gen4_ptp_set_config_mode(struct net_device *ndev)
> +{

[ ... ]

> +
> +static const struct ravb_gptp_info ravb_gen4_ptp_info = {
> +	.probe = ravb_gen4_ptp_probe,
> +	.clock_index = ravb_gen4_ptp_clock_index,
> +	.set_config_mode = ravb_gen4_ptp_set_config_mode,
> +};
> +

[Severity: Medium]
The commit message says that without "ptp-timer" the driver "will no longer
advertise gPTP support to user-space", but is only the advertisement
silenced here?

ravb_get_ts_info() is the single site that consults ->clock_index().
ravb_gen4_hw_info.ptp stays non-NULL, and nothing else looks at
priv->of_gptp, so the remaining timestamping paths stay armed:

ravb_hwtstamp_set() still accepts the request and returns 0:

	switch (config->tx_type) {
	case HWTSTAMP_TX_OFF:
	case HWTSTAMP_TX_ON:
		tstamp_tx_ctrl = config->tx_type;

and net_hwtstamp_validate() in net/core/dev_ioctl.c only range-checks
tx_type/rx_filter, it never consults the ethtool ts_info.  So SIOCSHWTSTAMP
succeeds on an interface that reports no hardware timestamping at all, and
ravb_hwtstamp_get() afterwards reports it as enabled.

The TX timestamp bookkeeping in ravb_start_xmit() is keyed only off
info->ptp and the queue:

	/* TX timestamp required */
	if (info->ptp) {
		if (q == RAVB_NC) {
			ts_skb = kmalloc_obj(*ts_skb, GFP_ATOMIC);

Each such skb gets an extra skb_get(), SKBTX_IN_PROGRESS and an entry on
priv->ts_skb_list.  The only drain is ravb_get_tx_tstamp(), which is driven
by the TX timestamp FIFO:

	count = (ravb_read(ndev, TSR) & TSR_TFFL) >> 8;
	while (count--) {

and that FIFO is fed by the timestamp counter this change intentionally
leaves without a source when "ptp-timer" is absent (and, given the PHC
lookup above, also when it is present).

An unprivileged SO_TIMESTAMPING socket sets SKBTX_HW_TSTAMP and
ravb_select_queue() steers those skbs to RAVB_NC, so if the FIFO never
produces entries, does priv->ts_skb_list keep growing with pinned skbs
until ravb_close() flushes it, with the senders never getting a timestamp?

Would it be cleaner to gate the hwtstamp_set/get and the RAVB_NC TX
timestamp path on the presence of a usable gPTP source as well?

> @@ -2774,7 +2825,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
>  	.irq_en_dis = 1,
> -	.ptp = &ravb_gen3_ptp_info,
> +	.ptp = &ravb_gen4_ptp_info,
>  	.nc_queues = 1,
>  	.magic_pkt = 1,
>  };
> @@ -3188,6 +3239,7 @@ static void ravb_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(dev);
>  	clk_unprepare(priv->refclk);
> +	of_node_put(priv->of_gptp);
>  	reset_control_assert(priv->rstc);
>  	free_netdev(ndev);
>  	platform_set_drvdata(pdev, NULL);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902084703.3228504-1-niklas.soderlund%2Brenesas%40ragnatech.se