[PATCH net-next v3 00/10] net: dsa: microchip: add PTP support for KSZ8463

[PATCH net-next v3 00/10] net: dsa: microchip: add PTP support for KSZ8463

[Bastien Curutchet (Schneider Electric)]

Hi all,

This series adds PTP support for the KSZ8463.

The KSZ8463 differs quite a lot from other KSZ switches supporting PTP:
it has a different interrupt logic and a different 'PTP engine'.

This third iteration addresses two more Sashiko comments from v2. I
didn't address some other Sashiko comments, I said why in the v2 thread.

Patches 1 to 4 add interrupt support for the KSZ8463
Patches 5 to 10 add the PTP support for the KSZ8463

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
Changes in v3:
- PATCH 8:
  - Initialize KSZ_SKB_CB(skb)->tstamp to 0 to prevent from exposing
    uninitialized memory in case of failure
  - Remove the skb->ip_summed line as this series only support L2 PTP
- PATCH 10:
  - Add a check on the register's address before applying the offset in
    ksz_read_ts()
- Link to v2: https://lore.kernel.org/r/20260713-ksz-new-ptp-v2-0-7209c05058ef@bootlin.com

Changes in v2:
- PATCH 1:
  - Modify the commit log to explain why IRQ setup isn't done on KSZ8463
- PATCH 4:
  - Fix the error code path in ksz8463_ptp_port_irq_setup().
  - Change the IRQ request/free orders to register/unregister the top level
    handler first/last
- PATCH 8:
  - Free the skb if its linearization fails
  - Downgrade ip_summed to CHECKSUM_NONE when clearing the reserved
    field of the PTP header
- PATCH 10:
  - Ensure the skb is linearized before accessing the PTP header
- Link to v1: https://lore.kernel.org/r/20260709-ksz-new-ptp-v1-0-344f02fe739e@bootlin.com

---
Bastien Curutchet (Schneider Electric) (10):
      net: dsa: microchip: implement ksz8463_setup()
      net: dsa: microchip: split ksz8_config_cpu_port()
      net: dsa: microchip: allow the use of other IRQ operations.
      net: dsa: microchip: add PTP interrupt handling for KSZ8463
      net: dsa: microchip: adapt port offset for KSZ8463's PTP register
      net: dsa: tag_ksz: move the KSZ8795 tag handling below ksz_xmit_timestamp()
      net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations
      net: dsa: microchip: add KSZ8463 tail tag handling
      net: dsa: microchip: explicitly enable detection of L2 PTP frames
      net: dsa: microchip: add two-steps PTP support for KSZ8463

 drivers/net/dsa/microchip/ksz8.c        | 311 +++++++++++++++++++++++++++-----
 drivers/net/dsa/microchip/ksz8_reg.h    |   1 +
 drivers/net/dsa/microchip/ksz_common.c  |  15 +-
 drivers/net/dsa/microchip/ksz_common.h  |   6 +
 drivers/net/dsa/microchip/ksz_ptp.c     | 271 +++++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_ptp.h     |  16 ++
 drivers/net/dsa/microchip/ksz_ptp_reg.h |  10 +
 include/net/dsa.h                       |   2 +
 net/dsa/tag_ksz.c                       | 233 +++++++++++++++---------
 9 files changed, 727 insertions(+), 138 deletions(-)
---
base-commit: a6cfd2762eb18cde4eb34906599ffa7b07c2ed60
change-id: 20260708-ksz-new-ptp-f98db30a8a4f

Best regards,
-- 
Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

[PATCH net-next v3 01/10] net: dsa: microchip: implement ksz8463_setup()

[Bastien Curutchet (Schneider Electric)]

KSZ8463 uses the ksz8_setup() as setup() callback for its DSA
operations. Its behavior is quite different than other KSZ8 switches,
especially its interrupt scheme.

Remove from the ksz8_setup()/ksz8_reset_switch() everything that is
ksz8463-related.
Create a dedicated ksz8463_setup() and a ksz8463_reset_switch() function.
This new ksz8463_setup() is widely inspired from ksz8_setup, it has
following differences:
- it doesn't configure drive strength (not supported on KSZ8463)
- it uses the ksz8463_reset_switch()
- it doesn't call ksz8_handle_global_errata() (the handled errata only
  affects the KSZ87xx variant)
- it doesn't configure IRQs. Note that ksz8_setup()'s IRQ initialization
  doesn't work for the KSZ8463 anyway. Proper support for it comes in
  upcoming patches.

Remove the teardown implementation from the KSZ8463 operations. Since
PTP and interrupts aren't setup, the common ksz_teardown() wouldn't do
anything anyway.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c | 126 +++++++++++++++++++++++++++++++++++----
 1 file changed, 114 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index c4c769028a20..5ed63f425013 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -181,6 +181,14 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
 	return ksz8_ind_write8(dev, table, (u8)(offset), data);
 }
 
+static int ksz8463_reset_switch(struct ksz_device *dev)
+{
+	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
+	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET,
+		false);
+	return 0;
+}
+
 static int ksz8_reset_switch(struct ksz_device *dev)
 {
 	if (ksz_is_ksz88x3(dev)) {
@@ -189,11 +197,6 @@ static int ksz8_reset_switch(struct ksz_device *dev)
 			KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, true);
 		ksz_cfg(dev, KSZ8863_REG_SW_RESET,
 			KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, false);
-	} else if (ksz_is_ksz8463(dev)) {
-		ksz_cfg(dev, KSZ8463_REG_SW_RESET,
-			KSZ8463_GLOBAL_SOFTWARE_RESET, true);
-		ksz_cfg(dev, KSZ8463_REG_SW_RESET,
-			KSZ8463_GLOBAL_SOFTWARE_RESET, false);
 	} else {
 		/* reset switch */
 		ksz_write8(dev, REG_POWER_MANAGEMENT_1,
@@ -2300,6 +2303,110 @@ static void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
 	spin_unlock(&mib->stats64_lock);
 }
 
+static int ksz8463_setup(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	u16 storm_mask, storm_rate;
+	struct ksz_port *p;
+	const u16 *regs;
+	int i, ret;
+
+	regs = dev->info->regs;
+
+	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
+				       dev->info->num_vlans, GFP_KERNEL);
+	if (!dev->vlan_cache)
+		return -ENOMEM;
+
+	ret = ksz8463_reset_switch(dev);
+	if (ret) {
+		dev_err(ds->dev, "failed to reset switch\n");
+		return ret;
+	}
+
+	/* set broadcast storm protection 10% rate */
+	storm_mask = BROADCAST_STORM_RATE;
+	storm_rate = (BROADCAST_STORM_VALUE * BROADCAST_STORM_PROT_RATE) / 100;
+	storm_mask = swab16(storm_mask);
+	storm_rate = swab16(storm_rate);
+	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
+			   storm_mask, storm_rate);
+
+	ksz8_config_cpu_port(ds);
+
+	ksz8_enable_stp_addr(dev);
+
+	ds->num_tx_queues = dev->info->num_tx_queues;
+
+	regmap_update_bits(ksz_regmap_8(dev), regs[S_MULTICAST_CTRL],
+			   MULTICAST_STORM_DISABLE, MULTICAST_STORM_DISABLE);
+
+	ksz_init_mib_timer(dev);
+
+	ds->configure_vlan_while_not_filtering = false;
+	ds->dscp_prio_mapping_is_global = true;
+	ds->mtu_enforcement_ingress = true;
+
+	/* We rely on software untagging on the CPU port, so that we
+	 * can support both tagged and untagged VLANs
+	 */
+	ds->untag_bridge_pvid = true;
+
+	/* VLAN filtering is partly controlled by the global VLAN
+	 * Enable flag
+	 */
+	ds->vlan_filtering_is_global = true;
+
+	/* Enable automatic fast aging when link changed detected. */
+	ksz_cfg(dev, S_LINK_AGING_CTRL, SW_LINK_AUTO_AGING, true);
+
+	/* Enable aggressive back off algorithm in half duplex mode. */
+	ret = ksz_rmw8(dev, REG_SW_CTRL_1, SW_AGGR_BACKOFF, SW_AGGR_BACKOFF);
+	if (ret)
+		return ret;
+
+	/*
+	 * Make sure unicast VLAN boundary is set as default and
+	 * enable no excessive collision drop.
+	 */
+	ret = ksz_rmw8(dev, REG_SW_CTRL_2,
+		       UNICAST_VLAN_BOUNDARY | NO_EXC_COLLISION_DROP,
+		       UNICAST_VLAN_BOUNDARY | NO_EXC_COLLISION_DROP);
+	if (ret)
+		return ret;
+
+	ksz_cfg(dev, S_REPLACE_VID_CTRL, SW_REPLACE_VID, false);
+
+	ksz_cfg(dev, S_MIRROR_CTRL, SW_MIRROR_RX_TX, false);
+
+	for (i = 0; i < (dev->info->num_vlans / 4); i++)
+		ksz8_r_vlan_entries(dev, i);
+
+	/* Start with learning disabled on standalone user ports, and enabled
+	 * on the CPU port. In lack of other finer mechanisms, learning on the
+	 * CPU port will avoid flooding bridge local addresses on the network
+	 * in some cases.
+	 */
+	p = &dev->ports[dev->cpu_port];
+	p->learning = true;
+
+	ret = ksz_mdio_register(dev);
+	if (ret < 0) {
+		dev_err(dev->dev, "failed to register the mdio");
+		return ret;
+	}
+
+	ret = ksz_dcb_init(dev);
+	if (ret)
+		return ret;
+
+	/* start switch */
+	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
+			   SW_START, SW_START);
+
+	return 0;
+}
+
 /**
  * ksz88x3_drive_strength_write() - Set the drive strength configuration for
  *				    KSZ8863 compatible chip variants.
@@ -2445,10 +2552,6 @@ static int ksz8_setup(struct dsa_switch *ds)
 	/* set broadcast storm protection 10% rate */
 	storm_mask = BROADCAST_STORM_RATE;
 	storm_rate = (BROADCAST_STORM_VALUE * BROADCAST_STORM_PROT_RATE) / 100;
-	if (ksz_is_ksz8463(dev)) {
-		storm_mask = swab16(storm_mask);
-		storm_rate = swab16(storm_rate);
-	}
 	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   storm_mask, storm_rate);
 
@@ -2499,7 +2602,7 @@ static int ksz8_setup(struct dsa_switch *ds)
 
 	ksz_cfg(dev, S_MIRROR_CTRL, SW_MIRROR_RX_TX, false);
 
-	if (!ksz_is_ksz88x3(dev) && !ksz_is_ksz8463(dev))
+	if (!ksz_is_ksz88x3(dev))
 		ksz_cfg(dev, REG_SW_CTRL_19, SW_INS_TAG_ENABLE, true);
 
 	for (i = 0; i < (dev->info->num_vlans / 4); i++)
@@ -2889,8 +2992,7 @@ const struct ksz_dev_ops ksz88xx_dev_ops = {
 const struct dsa_switch_ops ksz8463_switch_ops = {
 	.get_tag_protocol	= ksz8463_get_tag_protocol,
 	.connect_tag_protocol   = ksz8463_connect_tag_protocol,
-	.setup			= ksz8_setup,
-	.teardown		= ksz_teardown,
+	.setup			= ksz8463_setup,
 	.phy_read		= ksz8463_phy_read16,
 	.phy_write		= ksz8463_phy_write16,
 	.phylink_get_caps	= ksz8_phylink_get_caps,

-- 
2.55.0

Re: [PATCH net-next v3 01/10] net: dsa: microchip: implement ksz8463_setup()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net-next v3 01/10] net: dsa: microchip: implement ksz8463_setup() ---
commit 095ca74374f60de415fb4e79da7940e5e376827a
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

net: dsa: microchip: implement ksz8463_setup()

This splits the KSZ8463 out of ksz8_setup()/ksz8_reset_switch() into a
dedicated ksz8463_setup() and ksz8463_reset_switch(), dropping drive
strength configuration, the KSZ87xx global errata workaround and the IRQ
initialization for that chip.  It also removes .teardown from
ksz8463_switch_ops.

A few questions below.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index c4c769028a20c..5ed63f4250136 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -181,6 +181,14 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>  	return ksz8_ind_write8(dev, table, (u8)(offset), data);
>  }
>  
> +static int ksz8463_reset_switch(struct ksz_device *dev)
> +{
> +	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
> +	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET,
> +		false);
> +	return 0;
> +}
> +

[Severity: Low]
Can ksz8463_reset_switch() ever return anything but 0?  ksz_cfg() throws the
register write status away:

drivers/net/dsa/microchip/ksz8.c:ksz_cfg() {
	ksz_rmw8(dev, addr, bits, set ? bits : 0);
}

So the error handling added in ksz8463_setup() is dead code:

	ret = ksz8463_reset_switch(dev);
	if (ret) {
		dev_err(ds->dev, "failed to reset switch\n");
		return ret;
	}

If the SPI/regmap access to KSZ8463_REG_SW_RESET fails, ksz8463_setup()
continues on to the storm/aging/VLAN-boundary programming and finally sets
SW_START on a switch that was never reset, without any message.

Since this helper is new, would it make sense to either declare it void, or
use ksz_rmw8() directly and propagate the error the way the REG_SW_CTRL_1
and REG_SW_CTRL_2 writes in ksz8463_setup() already do?

The same always-return-0 shape exists in ksz8_reset_switch(), so this is not
a new behaviour, only newly duplicated.

>  static int ksz8_reset_switch(struct ksz_device *dev)
>  {
>  	if (ksz_is_ksz88x3(dev)) {

[ ... ]

> @@ -2300,6 +2303,110 @@ static void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
>  	spin_unlock(&mib->stats64_lock);
>  }
>  
> +static int ksz8463_setup(struct dsa_switch *ds)
> +{

[Severity: Low]
This isn't a bug, but ksz8463_setup() is a near-verbatim copy of
ksz8_setup(): the vlan_cache allocation, broadcast storm programming,
ksz8_config_cpu_port(), ksz8_enable_stp_addr(), ds->num_tx_queues, the
multicast storm disable, ksz_init_mib_timer(), all the ds->* flag
assignments, the S_LINK_AGING_CTRL / REG_SW_CTRL_1 / REG_SW_CTRL_2 /
S_REPLACE_VID_CTRL / S_MIRROR_CTRL writes, the ksz8_r_vlan_entries() loop,
the CPU port learning setting, ksz_mdio_register(), ksz_dcb_init() and the
final SW_START write are identical.  Only four things differ.

The rest of this family uses one setup function with small chip predicates
(ksz_is_ksz88x3()/ksz_is_ksz8463()).  Could the shared body be factored into
a common helper instead, so a later fix or a new DSA flag does not have to be
applied in two places?

> +	struct ksz_device *dev = ds->priv;
> +	u16 storm_mask, storm_rate;
> +	struct ksz_port *p;
> +	const u16 *regs;
> +	int i, ret;
> +
> +	regs = dev->info->regs;
> +
> +	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
> +				       dev->info->num_vlans, GFP_KERNEL);

[Severity: Low]
The count and size arguments look exchanged here.  The prototype is
devm_kcalloc(dev, n, size, gfp), so this reads as
sizeof(struct vlan_table) elements of dev->info->num_vlans bytes.  The
resulting byte count is the same, so nothing misbehaves, but would
devm_kcalloc(dev->dev, dev->info->num_vlans, sizeof(struct vlan_table),
GFP_KERNEL) be the intended order?

The same swap is present in ksz8_setup(), and this patch copies it into the
new function, which seems like a good moment to correct it.

> +	if (!dev->vlan_cache)
> +		return -ENOMEM;
> +
> +	ret = ksz8463_reset_switch(dev);
> +	if (ret) {
> +		dev_err(ds->dev, "failed to reset switch\n");
> +		return ret;
> +	}
> +

[ ... ]

> +	/* Start with learning disabled on standalone user ports, and enabled
> +	 * on the CPU port. In lack of other finer mechanisms, learning on the
> +	 * CPU port will avoid flooding bridge local addresses on the network
> +	 * in some cases.
> +	 */
> +	p = &dev->ports[dev->cpu_port];
> +	p->learning = true;
> +
> +	ret = ksz_mdio_register(dev);

[Severity: Medium]
ksz8463_setup() deliberately creates no interrupt domains (no
ksz_girq_setup()/ksz_pirq_setup()), but ksz_mdio_register() still runs the
PHY interrupt setup whenever dev->irq is set:

drivers/net/dsa/microchip/ksz_common.c:ksz_mdio_register() {
	...
	if (dev->irq > 0) {
		ret = ksz_irq_phy_setup(dev);
		if (ret)
			goto put_mdio_node;
	}
	...
}

and ksz_irq_phy_setup() resolves the PHY interrupt through the per-port
domain that ksz_pirq_setup() used to create:

			irq = irq_find_mapping(dev->ports[port].pirq.domain,
					       PORT_SRC_PHY_INT);
			if (!irq) {
				ret = -EINVAL;

For KSZ8463 dev->ports[port].pirq.domain is now always NULL, and a NULL
domain is not rejected:

kernel/irq/irqdomain.c:__irq_resolve_mapping() {
	/* Look for default domain if necessary */
	if (domain == NULL)
		domain = irq_default_domain;

On platforms that install an irq_default_domain, can this hand a virq owned
by an unrelated interrupt controller (hwirq 1 of the default domain) to
phylib via ds->user_mii_bus->irq[phy]?

And if devm_of_mdiobus_register() then fails, does ksz_irq_phy_free() call
irq_dispose_mapping() on that foreign virq, tearing down a mapping this
driver never created?

This needs a KSZ8463 node with both an 'interrupts' property (dev->irq =
spi->irq) and an 'mdio' child node with PHY sub-nodes.  On platforms without
a default domain the lookup returns 0 and probe fails with -EINVAL instead;
that outcome matches the pre-patch behaviour, since the KSZ8463 chip entry
sets no .port_nirqs and no per-port mappings were ever created.

Would gating ksz_irq_phy_setup() on the per-port domain actually existing be
preferable to relying on dev->irq alone?

> +	if (ret < 0) {
> +		dev_err(dev->dev, "failed to register the mdio");
> +		return ret;
> +	}
> +
> +	ret = ksz_dcb_init(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* start switch */
> +	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
> +			   SW_START, SW_START);
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -2889,8 +2992,7 @@ const struct ksz_dev_ops ksz88xx_dev_ops = {
>  const struct dsa_switch_ops ksz8463_switch_ops = {
>  	.get_tag_protocol	= ksz8463_get_tag_protocol,
>  	.connect_tag_protocol   = ksz8463_connect_tag_protocol,
> -	.setup			= ksz8_setup,
> -	.teardown		= ksz_teardown,
> +	.setup			= ksz8463_setup,

[Severity: High]
This isn't a bug introduced by this patch, but since the commit message says
"PTP and interrupts aren't setup" and this hunk is rewriting exactly this ops
table, it seems worth raising: ksz8463_switch_ops keeps advertising the PTP
callbacks

	.get_ts_info		= ksz_get_ts_info,
	.port_hwtstamp_get	= ksz_hwtstamp_get,
	.port_hwtstamp_set	= ksz_hwtstamp_set,
	.port_txtstamp		= ksz_port_txtstamp,
	.port_rxtstamp		= ksz_port_rxtstamp,

while no PTP clock is ever registered for this chip: the KSZ8463 entry in
ksz_switch_chips[] does not set .ptp_capable, and ksz8463_setup() calls
neither ksz_ptp_clock_register() nor ksz_ptp_irq_setup().  Those are the only
places that run mutex_init(&ptp_data->lock), spin_lock_init(&ptp_data->
clock_lock) and init_completion(&port->tstamp_msg_comp), so dev->ptp_data.
clock stays NULL and those objects stay all-zero.

ksz_get_ts_info() checks for this, but ksz_hwtstamp_set() does not:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_set_hwtstamp_config() {
	...
	return ksz_ptp_enable_mode(dev);
}

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_enable_mode() {
	...
	if (tag_en) {
		ptp_schedule_worker(ptp_data->clock, 0);
	} else {
		ptp_cancel_worker_sync(ptp_data->clock);
	}

	tagger_data->hwtstamp_set_state(dev->ds, tag_en);
}

With CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y, does a SIOCSHWTSTAMP on a KSZ8463
user port reach ptp_cancel_worker_sync(NULL) here, even for the
HWTSTAMP_TX_OFF / FILTER_NONE combination, followed by an indirect call
through the NULL tagger_data->hwtstamp_set_state (ksz8463_connect_tag_
protocol() only assigns xmit_work_fn)?

Similarly, does the RX path take spin_lock_bh(&ptp_data->clock_lock) in
ksz_tstamp_reconstruct() on a spinlock that was never initialized for this
chip?

Would it make sense to drop these callbacks from ksz8463_switch_ops, or gate
them on dev->info->ptp_capable, while this table is being restructured?  The
same stale callbacks are present in ksz87xx_switch_ops.

>  	.phy_read		= ksz8463_phy_read16,
>  	.phy_write		= ksz8463_phy_write16,
>  	.phylink_get_caps	= ksz8_phylink_get_caps,

Re: [PATCH net-next v3 01/10] net: dsa: microchip: implement ksz8463_setup()

[Bastien Curutchet]

Hi,

On 7/31/26 4:23 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> --- Patch [1]: [PATCH net-next v3 01/10] net: dsa: microchip: implement ksz8463_setup() ---
> commit 095ca74374f60de415fb4e79da7940e5e376827a
> Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
> 
> net: dsa: microchip: implement ksz8463_setup()
> 
> This splits the KSZ8463 out of ksz8_setup()/ksz8_reset_switch() into a
> dedicated ksz8463_setup() and ksz8463_reset_switch(), dropping drive
> strength configuration, the KSZ87xx global errata workaround and the IRQ
> initialization for that chip.  It also removes .teardown from
> ksz8463_switch_ops.
> 
> A few questions below.
> 
>> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
>> index c4c769028a20c..5ed63f4250136 100644
>> --- a/drivers/net/dsa/microchip/ksz8.c
>> +++ b/drivers/net/dsa/microchip/ksz8.c
>> @@ -181,6 +181,14 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>>   	return ksz8_ind_write8(dev, table, (u8)(offset), data);
>>   }
>>   
>> +static int ksz8463_reset_switch(struct ksz_device *dev)
>> +{
>> +	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
>> +	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET,
>> +		false);
>> +	return 0;
>> +}
>> +
> 
> [Severity: Low]
> Can ksz8463_reset_switch() ever return anything but 0?  ksz_cfg() throws the
> register write status away:
> 
> drivers/net/dsa/microchip/ksz8.c:ksz_cfg() {
> 	ksz_rmw8(dev, addr, bits, set ? bits : 0);
> }
> 
> So the error handling added in ksz8463_setup() is dead code:
> 
> 	ret = ksz8463_reset_switch(dev);
> 	if (ret) {
> 		dev_err(ds->dev, "failed to reset switch\n");
> 		return ret;
> 	}
> 
> If the SPI/regmap access to KSZ8463_REG_SW_RESET fails, ksz8463_setup()
> continues on to the storm/aging/VLAN-boundary programming and finally sets
> SW_START on a switch that was never reset, without any message.
> 
> Since this helper is new, would it make sense to either declare it void, or
> use ksz_rmw8() directly and propagate the error the way the REG_SW_CTRL_1
> and REG_SW_CTRL_2 writes in ksz8463_setup() already do?
> 
> The same always-return-0 shape exists in ksz8_reset_switch(), so this is not
> a new behaviour, only newly duplicated.
> 

As it says, the behaviour is the same in ksz8_reset_switch(), I only 
extracted the functions from it to have a better split between KSZ8463 
and others KSZ8.
IMO, the real issue here is that the ksz_cfg() helper ignores the 
ksz_rmw8() return value.

>>   static int ksz8_reset_switch(struct ksz_device *dev)
>>   {
>>   	if (ksz_is_ksz88x3(dev)) {
> 
> [ ... ]
> 
>> @@ -2300,6 +2303,110 @@ static void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
>>   	spin_unlock(&mib->stats64_lock);
>>   }
>>   
>> +static int ksz8463_setup(struct dsa_switch *ds)
>> +{
> 
> [Severity: Low]
> This isn't a bug, but ksz8463_setup() is a near-verbatim copy of
> ksz8_setup(): the vlan_cache allocation, broadcast storm programming,
> ksz8_config_cpu_port(), ksz8_enable_stp_addr(), ds->num_tx_queues, the
> multicast storm disable, ksz_init_mib_timer(), all the ds->* flag
> assignments, the S_LINK_AGING_CTRL / REG_SW_CTRL_1 / REG_SW_CTRL_2 /
> S_REPLACE_VID_CTRL / S_MIRROR_CTRL writes, the ksz8_r_vlan_entries() loop,
> the CPU port learning setting, ksz_mdio_register(), ksz_dcb_init() and the
> final SW_START write are identical.  Only four things differ.
> 
> The rest of this family uses one setup function with small chip predicates
> (ksz_is_ksz88x3()/ksz_is_ksz8463()).  Could the shared body be factored into
> a common helper instead, so a later fix or a new DSA flag does not have to be
> applied in two places?
> 

The goal of this patch is to introduce a dedicated ksz8463_setup() 
function so that its specific IRQ configuration (and, in an upcoming 
series, its specific periodic output configuration) can be handled 
without adding ksz8463-specific branches to the common ksz8_setup() 
function.
Indeed it comes with some code duplication. I considered adding some 
'sub-setup()' helpers shared between the ksz8xxx_setup() to avoid the 
copies. However, the differences are spread accross the setups(). At the 
beginning, the reset() and the storm configuration differ; in the 
middle, the ksz87xx()/ksz_is_ksz88x3() belong only to ksz8_setup(); at 
the end, the IRQ setup (added in PATCH 4) is completely different. So I 
came to the conclusion that the ksz8463_setup() could live a bit of code 
duplication.

>> +	struct ksz_device *dev = ds->priv;
>> +	u16 storm_mask, storm_rate;
>> +	struct ksz_port *p;
>> +	const u16 *regs;
>> +	int i, ret;
>> +
>> +	regs = dev->info->regs;
>> +
>> +	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
>> +				       dev->info->num_vlans, GFP_KERNEL);
> 
> [Severity: Low]
> The count and size arguments look exchanged here.  The prototype is
> devm_kcalloc(dev, n, size, gfp), so this reads as
> sizeof(struct vlan_table) elements of dev->info->num_vlans bytes.  The
> resulting byte count is the same, so nothing misbehaves, but would
> devm_kcalloc(dev->dev, dev->info->num_vlans, sizeof(struct vlan_table),
> GFP_KERNEL) be the intended order?
> 
> The same swap is present in ksz8_setup(), and this patch copies it into the
> new function, which seems like a good moment to correct it.
>

Yes once again, I only copied what was needed from ksz8_setup() so this 
kind of tiny mistakes are copied.
>> +	if (!dev->vlan_cache)
>> +		return -ENOMEM;
>> +
>> +	ret = ksz8463_reset_switch(dev);
>> +	if (ret) {
>> +		dev_err(ds->dev, "failed to reset switch\n");
>> +		return ret;
>> +	}
>> +
> 
> [ ... ]
> 
>> +	/* Start with learning disabled on standalone user ports, and enabled
>> +	 * on the CPU port. In lack of other finer mechanisms, learning on the
>> +	 * CPU port will avoid flooding bridge local addresses on the network
>> +	 * in some cases.
>> +	 */
>> +	p = &dev->ports[dev->cpu_port];
>> +	p->learning = true;
>> +
>> +	ret = ksz_mdio_register(dev);
> 
> [Severity: Medium]
> ksz8463_setup() deliberately creates no interrupt domains (no
> ksz_girq_setup()/ksz_pirq_setup()), but ksz_mdio_register() still runs the
> PHY interrupt setup whenever dev->irq is set:
> 
> drivers/net/dsa/microchip/ksz_common.c:ksz_mdio_register() {
> 	...
> 	if (dev->irq > 0) {
> 		ret = ksz_irq_phy_setup(dev);
> 		if (ret)
> 			goto put_mdio_node;
> 	}
> 	...
> }
> 
> and ksz_irq_phy_setup() resolves the PHY interrupt through the per-port
> domain that ksz_pirq_setup() used to create:
> 
> 			irq = irq_find_mapping(dev->ports[port].pirq.domain,
> 					       PORT_SRC_PHY_INT);
> 			if (!irq) {
> 				ret = -EINVAL;
> 
> For KSZ8463 dev->ports[port].pirq.domain is now always NULL, and a NULL
> domain is not rejected:
> 
> kernel/irq/irqdomain.c:__irq_resolve_mapping() {
> 	/* Look for default domain if necessary */
> 	if (domain == NULL)
> 		domain = irq_default_domain;
> 
> On platforms that install an irq_default_domain, can this hand a virq owned
> by an unrelated interrupt controller (hwirq 1 of the default domain) to
> phylib via ds->user_mii_bus->irq[phy]?
> 
> And if devm_of_mdiobus_register() then fails, does ksz_irq_phy_free() call
> irq_dispose_mapping() on that foreign virq, tearing down a mapping this
> driver never created?
> 
> This needs a KSZ8463 node with both an 'interrupts' property (dev->irq =
> spi->irq) and an 'mdio' child node with PHY sub-nodes.  On platforms without
> a default domain the lookup returns 0 and probe fails with -EINVAL instead;
> that outcome matches the pre-patch behaviour, since the KSZ8463 chip entry
> sets no .port_nirqs and no per-port mappings were ever created.
> 
> Would gating ksz_irq_phy_setup() on the per-port domain actually existing be
> preferable to relying on dev->irq alone?
> 

Proper IRQ handling for the KSZ8463 is added in PATCH 4. Current IRQ 
framework doesn't work for the KSZ8463 anyway, so IMO this patch doesn't 
introduce any regression.

>> +	if (ret < 0) {
>> +		dev_err(dev->dev, "failed to register the mdio");
>> +		return ret;
>> +	}
>> +
>> +	ret = ksz_dcb_init(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* start switch */
>> +	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
>> +			   SW_START, SW_START);
>> +
>> +	return 0;
>> +}
>> +
> 
> [ ... ]
> 
>> @@ -2889,8 +2992,7 @@ const struct ksz_dev_ops ksz88xx_dev_ops = {
>>   const struct dsa_switch_ops ksz8463_switch_ops = {
>>   	.get_tag_protocol	= ksz8463_get_tag_protocol,
>>   	.connect_tag_protocol   = ksz8463_connect_tag_protocol,
>> -	.setup			= ksz8_setup,
>> -	.teardown		= ksz_teardown,
>> +	.setup			= ksz8463_setup,
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but since the commit message says
> "PTP and interrupts aren't setup" and this hunk is rewriting exactly this ops
> table, it seems worth raising: ksz8463_switch_ops keeps advertising the PTP
> callbacks
> 
> 	.get_ts_info		= ksz_get_ts_info,
> 	.port_hwtstamp_get	= ksz_hwtstamp_get,
> 	.port_hwtstamp_set	= ksz_hwtstamp_set,
> 	.port_txtstamp		= ksz_port_txtstamp,
> 	.port_rxtstamp		= ksz_port_rxtstamp,
> 
> while no PTP clock is ever registered for this chip: the KSZ8463 entry in
> ksz_switch_chips[] does not set .ptp_capable, and ksz8463_setup() calls
> neither ksz_ptp_clock_register() nor ksz_ptp_irq_setup().  Those are the only
> places that run mutex_init(&ptp_data->lock), spin_lock_init(&ptp_data->
> clock_lock) and init_completion(&port->tstamp_msg_comp), so dev->ptp_data.
> clock stays NULL and those objects stay all-zero.
> 
> ksz_get_ts_info() checks for this, but ksz_hwtstamp_set() does not:
> 
> drivers/net/dsa/microchip/ksz_ptp.c:ksz_set_hwtstamp_config() {
> 	...
> 	return ksz_ptp_enable_mode(dev);
> }
> 
> drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_enable_mode() {
> 	...
> 	if (tag_en) {
> 		ptp_schedule_worker(ptp_data->clock, 0);
> 	} else {
> 		ptp_cancel_worker_sync(ptp_data->clock);
> 	}
> 
> 	tagger_data->hwtstamp_set_state(dev->ds, tag_en);
> }
> 
> With CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y, does a SIOCSHWTSTAMP on a KSZ8463
> user port reach ptp_cancel_worker_sync(NULL) here, even for the
> HWTSTAMP_TX_OFF / FILTER_NONE combination, followed by an indirect call
> through the NULL tagger_data->hwtstamp_set_state (ksz8463_connect_tag_
> protocol() only assigns xmit_work_fn)?
> 
> Similarly, does the RX path take spin_lock_bh(&ptp_data->clock_lock) in
> ksz_tstamp_reconstruct() on a spinlock that was never initialized for this
> chip?
> 
> Would it make sense to drop these callbacks from ksz8463_switch_ops, or gate
> them on dev->info->ptp_capable, while this table is being restructured?  The
> same stale callbacks are present in ksz87xx_switch_ops.
> 
As it says, this isn't introduced by this patch and it will be fixed on 
PTP support is added on PATCH 10.


Best regards,
Bastien

[PATCH net-next v3 02/10] net: dsa: microchip: split ksz8_config_cpu_port()

[Bastien Curutchet (Schneider Electric)]

ksz8_config_cpu_port() is only called twice, once by ksz8_setup() and
once by ksz8463_setup(). It contains a ksz8463 branch that could be
avoided in the ksz8_setup() case and a ksz87xx/ksz88xx branches that
could be avoided in ksz8463_setup() case.

Create ksz8463_config_cpu_port() that only handles the ksz8463 case and
remove the ksz8463 specificities from the common ksz8_config_cpu_port().

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c | 73 +++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index 5ed63f425013..3bbca6f9cfc5 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -2029,6 +2029,50 @@ static void ksz88x3_config_rmii_clk(struct ksz_device *dev)
 		KSZ88X3_PORT3_RMII_CLK_INTERNAL, rmii_clk_internal);
 }
 
+static void ksz8463_config_cpu_port(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
+	u8 fiber_ports = 0;
+	const u32 *masks;
+	const u16 *regs;
+	int i;
+
+	masks = dev->info->masks;
+	regs = dev->info->regs;
+
+	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true);
+
+	ksz8_port_setup(dev, dev->cpu_port, true);
+
+	for (i = 0; i < dev->phy_port_cnt; i++)
+		ksz_port_stp_state_set(ds, i, BR_STATE_DISABLED);
+
+	for (i = 0; i < dev->phy_port_cnt; i++) {
+		p = &dev->ports[i];
+		ksz_port_cfg(dev, i, regs[P_STP_CTRL], PORT_FORCE_FLOW_CTRL,
+			     p->fiber);
+		if (p->fiber)
+			fiber_ports |= (1 << i);
+	}
+
+	/* Setup fiber ports. */
+	if (fiber_ports) {
+		fiber_ports &= 3;
+		regmap_update_bits(ksz_regmap_16(dev), KSZ8463_REG_CFG_CTRL,
+				   fiber_ports << PORT_COPPER_MODE_S,
+				   0);
+		regmap_update_bits(ksz_regmap_16(dev), KSZ8463_REG_DSP_CTRL_6,
+				   COPPER_RECEIVE_ADJUSTMENT, 0);
+	}
+
+	/* Turn off PTP function as the switch enables it by default */
+	regmap_update_bits(ksz_regmap_16(dev), KSZ8463_PTP_MSG_CONF1,
+			   PTP_ENABLE, 0);
+	regmap_update_bits(ksz_regmap_16(dev), KSZ8463_PTP_CLK_CTRL,
+			   PTP_CLK_ENABLE, 0);
+}
+
 static void ksz8_config_cpu_port(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
@@ -2036,7 +2080,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
 	const u32 *masks;
 	const u16 *regs;
 	u8 remote;
-	u8 fiber_ports = 0;
 	int i;
 
 	masks = dev->info->masks;
@@ -2067,32 +2110,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
 		else
 			ksz_port_cfg(dev, i, regs[P_STP_CTRL],
 				     PORT_FORCE_FLOW_CTRL, false);
-		if (p->fiber)
-			fiber_ports |= (1 << i);
-	}
-	if (ksz_is_ksz8463(dev)) {
-		/* Setup fiber ports. */
-		if (fiber_ports) {
-			fiber_ports &= 3;
-			regmap_update_bits(ksz_regmap_16(dev),
-					   KSZ8463_REG_CFG_CTRL,
-					   fiber_ports << PORT_COPPER_MODE_S,
-					   0);
-			regmap_update_bits(ksz_regmap_16(dev),
-					   KSZ8463_REG_DSP_CTRL_6,
-					   COPPER_RECEIVE_ADJUSTMENT, 0);
-		}
-
-		/* Turn off PTP function as the switch's proprietary way of
-		 * handling timestamp is not supported in current Linux PTP
-		 * stack implementation.
-		 */
-		regmap_update_bits(ksz_regmap_16(dev),
-				   KSZ8463_PTP_MSG_CONF1,
-				   PTP_ENABLE, 0);
-		regmap_update_bits(ksz_regmap_16(dev),
-				   KSZ8463_PTP_CLK_CTRL,
-				   PTP_CLK_ENABLE, 0);
 	}
 }
 
@@ -2332,7 +2349,7 @@ static int ksz8463_setup(struct dsa_switch *ds)
 	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   storm_mask, storm_rate);
 
-	ksz8_config_cpu_port(ds);
+	ksz8463_config_cpu_port(ds);
 
 	ksz8_enable_stp_addr(dev);
 

-- 
2.55.0

[PATCH net-next v3 03/10] net: dsa: microchip: allow the use of other IRQ operations.

[Bastien Curutchet (Schneider Electric)]

The IRQ setup uses an hardcoded set of IRQ operations. These operations
don't fit with the KSZ8463 which has an inverted bit logic (it uses an
'enable irq' register instead of a 'mask irq' one) and 16-bits registers.

Take the IRQ domain operations as input of ksz_irq_common_setup() to
allow KSZ8463 to use the already existing setup with its own set of IRQ
operations.
Expose ksz_irq_common_setup() and ksz_irq_bus_lock/unlock() so they can
be used by ksz8.c.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 15 ++++++++-------
 drivers/net/dsa/microchip/ksz_common.h |  5 +++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 67ab6ddb9e53..ff1be540b546 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2419,14 +2419,14 @@ static void ksz_irq_unmask(struct irq_data *d)
 	kirq->masked &= ~BIT(d->hwirq);
 }
 
-static void ksz_irq_bus_lock(struct irq_data *d)
+void ksz_irq_bus_lock(struct irq_data *d)
 {
 	struct ksz_irq *kirq  = irq_data_get_irq_chip_data(d);
 
 	mutex_lock(&kirq->dev->lock_irq);
 }
 
-static void ksz_irq_bus_sync_unlock(struct irq_data *d)
+void ksz_irq_bus_sync_unlock(struct irq_data *d)
 {
 	struct ksz_irq *kirq  = irq_data_get_irq_chip_data(d);
 	struct ksz_device *dev = kirq->dev;
@@ -2504,14 +2504,15 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)
 	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
 }
 
-static int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq)
+int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq,
+			 const struct irq_domain_ops *ops)
 {
 	int ret, n;
 
 	kirq->dev = dev;
 
-	kirq->domain = irq_domain_create_simple(dev_fwnode(dev->dev), kirq->nirqs, 0,
-						&ksz_irq_domain_ops, kirq);
+	kirq->domain = irq_domain_create_simple(dev_fwnode(dev->dev),
+						kirq->nirqs, 0, ops, kirq);
 	if (!kirq->domain)
 		return -ENOMEM;
 
@@ -2543,7 +2544,7 @@ int ksz_girq_setup(struct ksz_device *dev)
 
 	girq->irq_num = dev->irq;
 
-	return ksz_irq_common_setup(dev, girq);
+	return ksz_irq_common_setup(dev, girq, &ksz_irq_domain_ops);
 }
 
 int ksz_pirq_setup(struct ksz_device *dev, u8 p)
@@ -2560,7 +2561,7 @@ int ksz_pirq_setup(struct ksz_device *dev, u8 p)
 	if (!pirq->irq_num)
 		return -EINVAL;
 
-	return ksz_irq_common_setup(dev, pirq);
+	return ksz_irq_common_setup(dev, pirq, &ksz_irq_domain_ops);
 }
 
 void ksz_teardown(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index acaf70e6f393..0f2abb22ca91 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -8,6 +8,7 @@
 #define __KSZ_COMMON_H
 
 #include <linux/etherdevice.h>
+#include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/pcs/pcs-xpcs.h>
@@ -508,6 +509,10 @@ int ksz_sw_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val);
 int ksz_parent_mdio_read(struct mii_bus *bus, int addr, int regnum);
 int ksz_parent_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val);
 int ksz_mdio_register(struct ksz_device *dev);
+void ksz_irq_bus_lock(struct irq_data *d);
+void ksz_irq_bus_sync_unlock(struct irq_data *d);
+int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq,
+			 const struct irq_domain_ops *ops);
 int ksz_pirq_setup(struct ksz_device *dev, u8 p);
 int ksz_girq_setup(struct ksz_device *dev);
 void ksz_irq_free(struct ksz_irq *kirq);

-- 
2.55.0

[PATCH net-next v3 04/10] net: dsa: microchip: add PTP interrupt handling for KSZ8463

[Bastien Curutchet (Schneider Electric)]

KSZ8463 PTP interrupts aren't handled by the driver.
The interrupt layout in KSZ8463 has nothing to do with the other
switches:
- Its global interrupt enable register is 16-bits long and follow an
  'enable' logic, instead of a 'mask' one
- all the interrupts of all ports are grouped into one status register
  while others have one interrupt register per port
- xdelay_req and pdresp timestamps share one single interrupt bit on the
  KSZ8463 while each of them has its own interrupt bit on other switches

Create a KSZ8463-specific set of interrupt domain operations to handle
the global IRQ layer. To limit code duplication, it uses the same
interrupt handler than the other switches. Since other switches have
8-bits registers, only the high-byte of the interrupt status/enable
registers are used. This high-byte is where the PTP interrupts are
located. The low-byte contains the wake-up detection interrupts so if at
some points these interrupts are needed we'll need a bit of rework here.

Create KSZ8463-specific functions to setup the PTP interrupts. The
created IRQ domain is tied to the first port of the KSZ8463. Again,
the same PTP interrupt handler than the others switches is used.

Implement the teardown callback to release the interrupts.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c        |  93 ++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_ptp.c     | 131 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_ptp.h     |   9 +++
 drivers/net/dsa/microchip/ksz_ptp_reg.h |   6 ++
 4 files changed, 237 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index 3bbca6f9cfc5..c099a7005808 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -36,6 +36,13 @@
 #include "ksz8_reg.h"
 #include "ksz8.h"
 
+/*
+ * We use only the high-byte (so odd addresses) of the 16-bits registers to fit
+ * in the common IRQ framework
+ */
+#define KSZ8463_REG_ISR			0x191
+#define KSZ8463_REG_IER			0x193
+
 /* ksz88x3_drive_strengths - Drive strength mapping for KSZ8863, KSZ8873, ..
  *			     variants.
  * This values are documented in KSZ8873 and KSZ8863 datasheets.
@@ -181,6 +188,58 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
 	return ksz8_ind_write8(dev, table, (u8)(offset), data);
 }
 
+static void ksz8463_irq_mask(struct irq_data *d)
+{
+	struct ksz_irq *kirq = irq_data_get_irq_chip_data(d);
+
+	kirq->masked &= ~BIT(d->hwirq);
+}
+
+static void ksz8463_irq_unmask(struct irq_data *d)
+{
+	struct ksz_irq *kirq = irq_data_get_irq_chip_data(d);
+
+	kirq->masked |= BIT(d->hwirq);
+}
+
+static const struct irq_chip ksz8463_irq_chip = {
+	.name			= "ksz8463-irq",
+	.irq_mask		= ksz8463_irq_mask,
+	.irq_unmask		= ksz8463_irq_unmask,
+	.irq_bus_lock		= ksz_irq_bus_lock,
+	.irq_bus_sync_unlock	= ksz_irq_bus_sync_unlock,
+};
+
+static int ksz8463_irq_domain_map(struct irq_domain *d,
+				  unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(irq, d->host_data);
+	irq_set_chip_and_handler(irq, &ksz8463_irq_chip, handle_level_irq);
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops ksz8463_irq_domain_ops = {
+	.map	= ksz8463_irq_domain_map,
+	.xlate	= irq_domain_xlate_twocell,
+};
+
+static int ksz8463_girq_setup(struct ksz_device *dev)
+{
+	struct ksz_irq *girq = &dev->girq;
+
+	girq->nirqs = 8;
+	girq->reg_mask = KSZ8463_REG_IER;
+	girq->reg_status = KSZ8463_REG_ISR;
+	girq->masked = 0;
+	snprintf(girq->name, sizeof(girq->name), "ksz8463-girq");
+
+	girq->irq_num = dev->irq;
+
+	return ksz_irq_common_setup(dev, girq, &ksz8463_irq_domain_ops);
+}
+
 static int ksz8463_reset_switch(struct ksz_device *dev)
 {
 	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
@@ -2407,21 +2466,50 @@ static int ksz8463_setup(struct dsa_switch *ds)
 	p = &dev->ports[dev->cpu_port];
 	p->learning = true;
 
+	if (dev->irq > 0) {
+		ret = ksz8463_girq_setup(dev);
+		if (ret)
+			return ret;
+
+		ret = ksz8463_ptp_irq_setup(ds);
+		if (ret)
+			goto free_girq;
+	}
+
 	ret = ksz_mdio_register(dev);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to register the mdio");
-		return ret;
+		goto free_ptp_irq;
 	}
 
 	ret = ksz_dcb_init(dev);
 	if (ret)
-		return ret;
+		goto free_ptp_irq;
 
 	/* start switch */
 	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
 			   SW_START, SW_START);
 
 	return 0;
+
+free_ptp_irq:
+	if (dev->irq > 0)
+		ksz8463_ptp_irq_free(ds);
+free_girq:
+	if (dev->irq > 0)
+		ksz_irq_free(&dev->girq);
+
+	return ret;
+}
+
+static void ksz8463_teardown(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+
+	if (dev->irq > 0) {
+		ksz8463_ptp_irq_free(ds);
+		ksz_irq_free(&dev->girq);
+	}
 }
 
 /**
@@ -3010,6 +3098,7 @@ const struct dsa_switch_ops ksz8463_switch_ops = {
 	.get_tag_protocol	= ksz8463_get_tag_protocol,
 	.connect_tag_protocol   = ksz8463_connect_tag_protocol,
 	.setup			= ksz8463_setup,
+	.teardown		= ksz8463_teardown,
 	.phy_read		= ksz8463_phy_read16,
 	.phy_write		= ksz8463_phy_write16,
 	.phylink_get_caps	= ksz8_phylink_get_caps,
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 8b98039320ad..89fbc1253f25 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -32,6 +32,15 @@
 
 #define KSZ_PTP_INT_START 13
 
+/*
+ * PTP interrupt bit is the bit 12 of the 16-bits ISR/IER. But ksz_common.c only
+ * accesses the high-byte of these registers so the PTP interrupt bit becomes 4.
+ */
+#define KSZ8463_SRC_PTP_INT		4
+#define KSZ8463_PTP_PORT1_INT_START	12
+#define KSZ8463_PTP_PORT2_INT_START	14
+#define KSZ8463_PTP_INT_START		KSZ8463_PTP_PORT1_INT_START
+
 static int ksz_ptp_tou_gpio(struct ksz_device *dev)
 {
 	int ret;
@@ -1129,6 +1138,128 @@ static int ksz_ptp_msg_irq_setup(struct ksz_port *port, u8 n)
 	return ret;
 }
 
+static int ksz8463_ptp_port_irq_setup(struct ksz_irq *ptpirq,
+				      struct ksz_port *port, int hw_irq)
+{
+	u16 ts_reg[] = {KSZ8463_REG_PORT_SYNC_TS, KSZ8463_REG_PORT_DREQ_TS};
+	static const char * const name[] = {"sync-msg", "delay-msg"};
+	const struct ksz_dev_ops *ops = port->ksz_dev->dev_ops;
+	struct ksz_ptp_irq *ptpmsg_irq;
+	int ret;
+	int i;
+
+	init_completion(&port->tstamp_msg_comp);
+
+	for (i = 0; i < 2; i++) {
+		ptpmsg_irq = &port->ptpmsg_irq[i];
+		ptpmsg_irq->num = irq_create_mapping(ptpirq->domain,
+						     hw_irq + i);
+		if (!ptpmsg_irq->num) {
+			ret = -EINVAL;
+			goto release_msg_irq;
+		}
+
+		ptpmsg_irq->port = port;
+		ptpmsg_irq->ts_reg = ops->get_port_addr(port->num, ts_reg[i]);
+
+		strscpy(ptpmsg_irq->name, name[i]);
+
+		ret = request_threaded_irq(ptpmsg_irq->num, NULL,
+					   ksz_ptp_msg_thread_fn, IRQF_ONESHOT,
+					   ptpmsg_irq->name, ptpmsg_irq);
+		if (ret) {
+			irq_dispose_mapping(ptpmsg_irq->num);
+			goto release_msg_irq;
+		}
+	}
+
+	return 0;
+
+release_msg_irq:
+	while (i--)
+		ksz_ptp_msg_irq_free(port, i);
+
+	return ret;
+}
+
+static void ksz8463_ptp_port_irq_teardown(struct ksz_port *port)
+{
+	int i;
+
+	for (i = 0; i < 2; i++)
+		ksz_ptp_msg_irq_free(port, i);
+}
+
+int ksz8463_ptp_irq_setup(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *port1, *port2;
+	struct ksz_irq *ptpirq;
+	int ret;
+
+	port1 = &dev->ports[0];
+	port2 = &dev->ports[1];
+	ptpirq = &port1->ptpirq;
+
+	ptpirq->irq_num = irq_find_mapping(dev->girq.domain,
+					   KSZ8463_SRC_PTP_INT);
+	if (!ptpirq->irq_num)
+		return -EINVAL;
+
+	ptpirq->dev = dev;
+	ptpirq->nirqs = 4;
+	ptpirq->reg_mask = KSZ8463_PTP_TS_IER;
+	ptpirq->reg_status = KSZ8463_PTP_TS_ISR;
+	ptpirq->irq0_offset = KSZ8463_PTP_INT_START;
+	snprintf(ptpirq->name, sizeof(ptpirq->name), "ptp-irq");
+
+	ptpirq->domain = irq_domain_create_linear(dev_fwnode(dev->dev),
+						  ptpirq->nirqs,
+						  &ksz_ptp_irq_domain_ops,
+						  ptpirq);
+	if (!ptpirq->domain)
+		return -ENOMEM;
+
+	ret = ksz8463_ptp_port_irq_setup(ptpirq, port1,
+					 KSZ8463_PTP_PORT1_INT_START - KSZ8463_PTP_INT_START);
+	if (ret)
+		goto release_domain;
+
+	ret = ksz8463_ptp_port_irq_setup(ptpirq, port2,
+					 KSZ8463_PTP_PORT2_INT_START - KSZ8463_PTP_INT_START);
+	if (ret)
+		goto free_port1;
+
+	ret = request_threaded_irq(ptpirq->irq_num, NULL, ksz_ptp_irq_thread_fn,
+				   IRQF_ONESHOT, ptpirq->name, ptpirq);
+	if (ret)
+		goto free_port2;
+
+	return 0;
+
+free_port2:
+	ksz8463_ptp_port_irq_teardown(port2);
+free_port1:
+	ksz8463_ptp_port_irq_teardown(port1);
+release_domain:
+	irq_domain_remove(ptpirq->domain);
+
+	return ret;
+}
+
+void ksz8463_ptp_irq_free(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *port1 = &dev->ports[0];
+	struct ksz_port *port2 = &dev->ports[1];
+	struct ksz_irq *ptpirq = &port1->ptpirq;
+
+	free_irq(ptpirq->irq_num, ptpirq);
+	ksz8463_ptp_port_irq_teardown(port2);
+	ksz8463_ptp_port_irq_teardown(port1);
+	irq_domain_remove(ptpirq->domain);
+}
+
 int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 {
 	struct ksz_device *dev = ds->priv;
diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
index 3086e519b1b6..11408580031d 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.h
+++ b/drivers/net/dsa/microchip/ksz_ptp.h
@@ -50,6 +50,8 @@ bool ksz_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
 		       unsigned int type);
 int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p);
 void ksz_ptp_irq_free(struct dsa_switch *ds, u8 p);
+int ksz8463_ptp_irq_setup(struct dsa_switch *ds);
+void ksz8463_ptp_irq_free(struct dsa_switch *ds);
 
 #else
 
@@ -72,6 +74,13 @@ static inline int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 
 static inline void ksz_ptp_irq_free(struct dsa_switch *ds, u8 p) {}
 
+static inline int ksz8463_ptp_irq_setup(struct dsa_switch *ds)
+{
+	return 0;
+}
+
+static inline void ksz8463_ptp_irq_free(struct dsa_switch *ds) {}
+
 #define ksz_get_ts_info NULL
 
 #define ksz_hwtstamp_get NULL
diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h b/drivers/net/dsa/microchip/ksz_ptp_reg.h
index eab9aecb7fa8..1a669d6ee889 100644
--- a/drivers/net/dsa/microchip/ksz_ptp_reg.h
+++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
@@ -121,6 +121,12 @@
 #define REG_PTP_PORT_SYNC_TS		0x0C0C
 #define REG_PTP_PORT_PDRESP_TS		0x0C10
 
+#define KSZ8463_REG_PORT_DREQ_TS	0x0648
+#define KSZ8463_REG_PORT_SYNC_TS	0x064C
+#define KSZ8463_REG_PORT_DRESP_TS	0x0650
+#define KSZ8463_PTP_TS_ISR		0x068C
+#define KSZ8463_PTP_TS_IER		0x068E
+
 #define REG_PTP_PORT_TX_INT_STATUS__2	0x0C14
 #define REG_PTP_PORT_TX_INT_ENABLE__2	0x0C16
 

-- 
2.55.0

Re: [PATCH net-next v3 04/10] net: dsa: microchip: add PTP interrupt handling for KSZ8463

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [4]: [PATCH net-next v3 04/10] net: dsa: microchip: add PTP interrupt handling for KSZ8463 ---
commit 0135c84c1b18f8d3665107436a050b472916ca2f
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

net: dsa: microchip: add PTP interrupt handling for KSZ8463

This adds a KSZ8463-specific global interrupt domain (16-bit enable
register, one shared status register, only the high byte used) and
KSZ8463-specific PTP interrupt setup/teardown that reuse the existing
interrupt handlers. A .teardown callback is added to release the
interrupts.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 3bbca6f9cfc5a..c099a7005808b 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -36,6 +36,13 @@
>  #include "ksz8_reg.h"
>  #include "ksz8.h"
>  
> +/*
> + * We use only the high-byte (so odd addresses) of the 16-bits registers to fit
> + * in the common IRQ framework
> + */
> +#define KSZ8463_REG_ISR			0x191
> +#define KSZ8463_REG_IER			0x193
> +
>  /* ksz88x3_drive_strengths - Drive strength mapping for KSZ8863, KSZ8873, ..
>   *			     variants.
>   * This values are documented in KSZ8873 and KSZ8863 datasheets.
> @@ -181,6 +188,58 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>  	return ksz8_ind_write8(dev, table, (u8)(offset), data);
>  }
>  

[ ... ]

> +static int ksz8463_girq_setup(struct ksz_device *dev)
> +{
> +	struct ksz_irq *girq = &dev->girq;
> +
> +	girq->nirqs = 8;
> +	girq->reg_mask = KSZ8463_REG_IER;
> +	girq->reg_status = KSZ8463_REG_ISR;
> +	girq->masked = 0;
> +	snprintf(girq->name, sizeof(girq->name), "ksz8463-girq");
> +
> +	girq->irq_num = dev->irq;
> +
> +	return ksz_irq_common_setup(dev, girq, &ksz8463_irq_domain_ops);
> +}

[Severity: Medium]
ksz_irq_common_setup() requests the generic ksz_irq_thread_fn(), which only
reads the status register and dispatches nested IRQs:

	ret = ksz_read8(dev, kirq->reg_status, &data);
	...
	for (n = 0; n < kirq->nirqs; ++n) {
		if (data & BIT(n)) {
			sub_irq = irq_find_mapping(kirq->domain, n);
			handle_nested_irq(sub_irq);
			++nhandled;

It never writes reg_status back. That is fine for KSZ9477/LAN937x, where
REG_SW_PORT_INT_STATUS__1 is a read-only aggregation of the per-port status
registers. Is the KSZ8463 global status register latched write-1-to-clear
instead? The driver's own handler for the sibling register on this chip
treats it that way, in ksz_ptp_irq_thread_fn():

	/* Clear the interrupts W1C */
	ret = ksz_write16(dev, ptpirq->reg_status, data);

If the global bit is latched, is there anything in the new path that
acknowledges it? The level-triggered IRQF_ONESHOT parent would stay asserted
and the threaded handler re-entered indefinitely over SPI.

Related to that: girq->nirqs = 8 creates mappings for all eight high-byte
bits while only bit 4 ever gets an action. handle_nested_irq() on a virq
without an action returns immediately, yet nhandled is still incremented, so
the parent always returns IRQ_HANDLED and the "nobody cared" spurious
interrupt detection can never fire. Should the mapping count be limited to
the bits that are actually used?

>  static int ksz8463_reset_switch(struct ksz_device *dev)
>  {
>  	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
> @@ -2407,21 +2466,50 @@ static int ksz8463_setup(struct dsa_switch *ds)
>  	p = &dev->ports[dev->cpu_port];
>  	p->learning = true;
>  
> +	if (dev->irq > 0) {
> +		ret = ksz8463_girq_setup(dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = ksz8463_ptp_irq_setup(ds);
> +		if (ret)
> +			goto free_girq;
> +	}
> +

[Severity: High]
The PTP interrupt path is enabled here, but the KSZ8463 entry in
ksz_switch_chips[] has no .ptp_capable and ksz8463_setup() never calls
ksz_ptp_clock_register(). Is the PTP state these handlers consume ever
initialized?

ksz_ptp_clock_register() is the only place that runs:

	mutex_init(&ptp_data->lock);
	spin_lock_init(&ptp_data->clock_lock);
	...
	ret = ksz_ptp_start_clock(dev);
	...
	ptp_data->clock = ptp_clock_register(&ptp_data->caps, dev->dev);

The handler requested below, ksz_ptp_msg_thread_fn(), calls
ksz_tstamp_reconstruct(), which does:

	spin_lock_bh(&ptp_data->clock_lock);
	ptp_clock_time = ptp_data->clock_time;
	spin_unlock_bh(&ptp_data->clock_lock);

For KSZ8463 that spinlock is never initialized (spinlock bad magic with
CONFIG_DEBUG_SPINLOCK, "trying to register non-static key" with lockdep) and
clock_time is never advanced by an aux worker, so timestamps produced by the
new interrupt path come from an all-zero clock time.

In the same configuration ptp_data->clock stays NULL while
ksz8463_switch_ops still exports .port_hwtstamp_set = ksz_hwtstamp_set, and
ksz_set_hwtstamp_config() ends in ksz_ptp_enable_mode():

	if (tag_en) {
		ptp_schedule_worker(ptp_data->clock, 0);
	} else {
		ptp_cancel_worker_sync(ptp_data->clock);
	}

ptp_schedule_worker() dereferences ptp->kworker unconditionally, so does a
SIOCSHWTSTAMP on a KSZ8463 user port oops here? Should this hunk also set
.ptp_capable for KSZ8463 and call ksz_ptp_clock_register(), as
ksz8_setup()/ksz9477_setup() do?

>  	ret = ksz_mdio_register(dev);
>  	if (ret < 0) {
>  		dev_err(dev->dev, "failed to register the mdio");
> -		return ret;
> +		goto free_ptp_irq;
>  	}
>  
>  	ret = ksz_dcb_init(dev);
>  	if (ret)
> -		return ret;
> +		goto free_ptp_irq;

[Severity: Medium]
Making dev->irq > 0 a supported configuration for KSZ8463 also makes
ksz_mdio_register() -> ksz_irq_phy_setup() reachable, and that function does:

	irq = irq_find_mapping(dev->ports[port].pirq.domain,
			       PORT_SRC_PHY_INT);
	if (!irq) {
		ret = -EINVAL;

ksz8463_setup() never calls ksz_pirq_setup(), so dev->ports[].pirq.domain
stays NULL. __irq_resolve_mapping() substitutes irq_default_domain when the
domain pointer is NULL. On platforms without a default domain the lookup
returns 0 and KSZ8463 setup fails with -EINVAL for every device tree that has
both an interrupts property and an mdio node with internal PHY handles. On
platforms that install a default host, doesn't the returned virq belong to an
unrelated interrupt controller, which is then stored in
ds->user_mii_bus->irq[phy], request_irq()'ed by phylib and later disposed by
ksz_irq_phy_free()?

Separately, the goto free_ptp_irq taken when ksz_dcb_init() fails runs after
ksz_mdio_register() has succeeded. Does that path leak the PHY IRQ mappings
acquired inside ksz_mdio_register(), since ksz_irq_phy_free() is not called?

>  
>  	/* start switch */
>  	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
>  			   SW_START, SW_START);
>  
>  	return 0;
> +
> +free_ptp_irq:
> +	if (dev->irq > 0)
> +		ksz8463_ptp_irq_free(ds);
> +free_girq:
> +	if (dev->irq > 0)
> +		ksz_irq_free(&dev->girq);
> +
> +	return ret;
> +}

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 8b98039320add..89fbc1253f252 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c

[ ... ]

> @@ -1129,6 +1138,128 @@ static int ksz_ptp_msg_irq_setup(struct ksz_port *port, u8 n)
>  	return ret;
>  }
>  
> +static int ksz8463_ptp_port_irq_setup(struct ksz_irq *ptpirq,
> +				      struct ksz_port *port, int hw_irq)
> +{
> +	u16 ts_reg[] = {KSZ8463_REG_PORT_SYNC_TS, KSZ8463_REG_PORT_DREQ_TS};
> +	static const char * const name[] = {"sync-msg", "delay-msg"};

[Severity: Medium]
This gives ptpmsg_irq[] different index semantics than the rest of the
driver: slot 0 is sync-msg and slot 1 is delay-msg here, and only slots 0 and
1 are mapped. But ksz_ptp_reg.h defines:

	#define KSZ_SYNC_MSG			2
	#define KSZ_XDREQ_MSG			1
	#define KSZ_PDRES_MSG			0

and the shared ksz_set_hwtstamp_config(), reachable for KSZ8463 because
ksz8463_switch_ops sets .port_hwtstamp_set = ksz_hwtstamp_set, indexes the
same array with those constants:

	prt->ptpmsg_irq[KSZ_SYNC_MSG].ts_en  = false;
	prt->ptpmsg_irq[KSZ_XDREQ_MSG].ts_en = true;
	prt->ptpmsg_irq[KSZ_PDRES_MSG].ts_en = false;

So on KSZ8463 the Sync handler in slot 0 is gated by the Pdelay_Resp flag,
and every write to ptpmsg_irq[KSZ_SYNC_MSG] lands in slot 2, which has no
mapping, no handler and no port back-pointer.

In the two TX modes currently accepted for this chip both of those flags end
up false, so no wrong timestamp is observable today. Should KSZ8463-specific
index constants be added, or the shared code taught about this layout, so the
gating does not silently go to the wrong handler once two-step TX
timestamping is supported?

> +	const struct ksz_dev_ops *ops = port->ksz_dev->dev_ops;
> +	struct ksz_ptp_irq *ptpmsg_irq;
> +	int ret;
> +	int i;
> +
> +	init_completion(&port->tstamp_msg_comp);

[Severity: High]
This isn't a bug introduced by this patch, but the initialization of
port->tstamp_msg_comp now happens only on the dev->irq > 0 path. Before this
series it was never initialized for KSZ8463 at all, and a KSZ8463 without an
interrupt keeps that behaviour while ksz8463_switch_ops exports
.port_hwtstamp_set/.port_txtstamp and ksz_set_hwtstamp_config() checks
neither ptp_capable, nor the clock, nor dev->irq.

After HWTSTAMP_TX_ONESTEP_P2P is accepted, the transmit path runs:

ksz_port_deferred_xmit()
	reinit_completion(&prt->tstamp_msg_comp);
	dsa_enqueue_skb(skb, skb->dev);
	ksz_ptp_txtstamp_skb(dev, prt, clone);

ksz_ptp_txtstamp_skb()
	ret = wait_for_completion_timeout(&prt->tstamp_msg_comp,
					  msecs_to_jiffies(100));

on an all-zero struct completion, where x->wait.head has NULL next/prev, so
__add_wait_queue_entry_tail() inside do_wait_for_common() dereferences NULL.

Could the completion be initialized unconditionally, or the hwtstamp
configuration be rejected when the PTP interrupt infrastructure was not set
up?

> +
> +	for (i = 0; i < 2; i++) {
> +		ptpmsg_irq = &port->ptpmsg_irq[i];
> +		ptpmsg_irq->num = irq_create_mapping(ptpirq->domain,
> +						     hw_irq + i);
> +		if (!ptpmsg_irq->num) {
> +			ret = -EINVAL;
> +			goto release_msg_irq;
> +		}
> +
> +		ptpmsg_irq->port = port;
> +		ptpmsg_irq->ts_reg = ops->get_port_addr(port->num, ts_reg[i]);

[Severity: Medium]
The commit message says "xdelay_req and pdresp timestamps share one single
interrupt bit on the KSZ8463", but the single delay-msg handler bound to that
shared bit has its ts_reg hard-wired to KSZ8463_REG_PORT_DREQ_TS, and the
newly added KSZ8463_REG_PORT_DRESP_TS has no user anywhere in the tree.

ksz_ptp_msg_thread_fn() cannot distinguish the two events:

	if (ptpmsg_irq->ts_en) {
		ret = ksz_read_ts(port, ptpmsg_irq->ts_reg, &tstamp_raw);
		...
		complete(&port->tstamp_msg_comp);

In HWTSTAMP_TX_ONESTEP_P2P, ksz_port_txtstamp() returns early for
PTP_MSGTYPE_PDELAY_RESP (correction field update, no clone, no waiter), while
ptpmsg_irq[KSZ_XDREQ_MSG].ts_en stays true. If a transmitted Pdelay_Resp
raises the shared bit inside the window opened by reinit_completion() in
ksz_port_deferred_xmit() and the wait in ksz_ptp_txtstamp_skb(), does the
Pdelay_Req waiter then return early with the value read from DREQ_TS for an
unrelated event?

Since ts_en has to stay true for Delay_Req in the only supported TX mode, is
there any way to filter the pdresp event in software here?

> +
> +		strscpy(ptpmsg_irq->name, name[i]);
> +
> +		ret = request_threaded_irq(ptpmsg_irq->num, NULL,
> +					   ksz_ptp_msg_thread_fn, IRQF_ONESHOT,
> +					   ptpmsg_irq->name, ptpmsg_irq);
> +		if (ret) {
> +			irq_dispose_mapping(ptpmsg_irq->num);
> +			goto release_msg_irq;
> +		}
> +	}
> +
> +	return 0;
> +
> +release_msg_irq:
> +	while (i--)
> +		ksz_ptp_msg_irq_free(port, i);
> +
> +	return ret;
> +}
> +

[ ... ]

> +int ksz8463_ptp_irq_setup(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *port1, *port2;
> +	struct ksz_irq *ptpirq;
> +	int ret;
> +
> +	port1 = &dev->ports[0];
> +	port2 = &dev->ports[1];
> +	ptpirq = &port1->ptpirq;

[Severity: Low]
This isn't a bug, but the domain and all of its bookkeeping (irq_num, nirqs,
reg_mask, reg_status, irq0_offset, name) are switch-global for the KSZ8463
while being stored in dev->ports[0].ptpirq, and both this function and
ksz8463_ptp_irq_free() hardcode port indices 0 and 1 instead of iterating
with dsa_switch_for_each_user_port(), which is what
ksz8_setup()/ksz9477_setup()/lan937x_setup() use.

dev->ports[1].ptpirq stays all-zero while ports[1].ptpmsg_irq[] is populated
from a domain owned by port 0, so struct ksz_port::ptpirq means something
different depending on the chip, and generic code that assumed it was valid
per port would end up calling irq_domain_remove(NULL) or free_irq(0).

Would placing the shared domain in struct ksz_device next to girq, and
iterating user ports for the per-port message IRQs, be preferable?

> +
> +	ptpirq->irq_num = irq_find_mapping(dev->girq.domain,
> +					   KSZ8463_SRC_PTP_INT);
> +	if (!ptpirq->irq_num)
> +		return -EINVAL;
> +

[ ... ]

[PATCH net-next v3 05/10] net: dsa: microchip: adapt port offset for KSZ8463's PTP register

[Bastien Curutchet (Schneider Electric)]

In KSZ8463 register's layout, the offset between port 1 and port 2
registers isn't the same in the generic control register area than in
the PTP register area. The get_port_addr() always uses the same offset
so it doesn't work when it's used to access PTP registers.

Adapt the port offset in get_port_addr() when the accessed register is
in the PTP area.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index c099a7005808..5e5bfc5cae2d 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -2831,6 +2831,9 @@ static u32 ksz8_get_port_addr(int port, int offset)
 
 static u32 ksz8463_get_port_addr(int port, int offset)
 {
+	if (offset >= KSZ8463_PTP_CLK_CTRL)
+		return offset + 0x20 * port;
+
 	return offset + 0x18 * port;
 }
 

-- 
2.55.0

[PATCH net-next v3 06/10] net: dsa: tag_ksz: move the KSZ8795 tag handling below ksz_xmit_timestamp()

[Bastien Curutchet (Schneider Electric)]

Upcoming patch reduces code duplication between KSZ8795 and KSZ9893 by
introducing a common xmit() function. This rework needs the KSZ8795
handlers to be implemented below ksz_defer_xmit().

Do the move now to reduce the noise in next patch.
No functionnal change is intended in this patch.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 net/dsa/tag_ksz.c | 132 +++++++++++++++++++++++++++---------------------------
 1 file changed, 66 insertions(+), 66 deletions(-)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 67fa89f102e0..f58ce0f0e9e4 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -103,72 +103,6 @@ static struct sk_buff *ksz_common_rcv(struct sk_buff *skb,
 	return skb;
 }
 
-/*
- * For Ingress (Host -> KSZ8795), 1 byte is added before FCS.
- * ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag(1byte)|FCS(4bytes)
- * ---------------------------------------------------------------------------
- * tag : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
- *
- * For Egress (KSZ8795 -> Host), 1 byte is added before FCS.
- * ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
- * ---------------------------------------------------------------------------
- * tag0 : zero-based value represents port
- *	  (eg, 0x0=port1, 0x2=port3, 0x3=port4)
- */
-
-#define KSZ8795_TAIL_TAG_EG_PORT_M	GENMASK(1, 0)
-#define KSZ8795_TAIL_TAG_OVERRIDE	BIT(6)
-#define KSZ8795_TAIL_TAG_LOOKUP		BIT(7)
-
-static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
-{
-	struct ethhdr *hdr;
-	u8 *tag;
-
-	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
-		kfree_skb(skb);
-		return NULL;
-	}
-
-	/* Tag encoding */
-	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
-	hdr = skb_eth_hdr(skb);
-
-	*tag = dsa_xmit_port_mask(skb, dev);
-	if (is_link_local_ether_addr(hdr->h_dest))
-		*tag |= KSZ8795_TAIL_TAG_OVERRIDE;
-
-	return skb;
-}
-
-static struct sk_buff *ksz8795_rcv(struct sk_buff *skb, struct net_device *dev)
-{
-	u8 *tag;
-
-	if (skb_linearize(skb)) {
-		kfree_skb(skb);
-		return NULL;
-	}
-
-	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
-
-	return ksz_common_rcv(skb, dev, tag[0] & KSZ8795_TAIL_TAG_EG_PORT_M,
-			      KSZ_EGRESS_TAG_LEN);
-}
-
-static const struct dsa_device_ops ksz8795_netdev_ops = {
-	.name	= KSZ8795_NAME,
-	.proto	= DSA_TAG_PROTO_KSZ8795,
-	.xmit	= ksz8795_xmit,
-	.rcv	= ksz8795_rcv,
-	.needed_tailroom = KSZ_INGRESS_TAG_LEN,
-};
-
-DSA_TAG_DRIVER(ksz8795_netdev_ops);
-MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
-
 /*
  * For Ingress (Host -> KSZ9477), 2/6 bytes are added before FCS.
  * ---------------------------------------------------------------------------
@@ -353,6 +287,72 @@ static const struct dsa_device_ops ksz9477_netdev_ops = {
 DSA_TAG_DRIVER(ksz9477_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477, KSZ9477_NAME);
 
+/*
+ * For Ingress (Host -> KSZ8795), 1 byte is added before FCS.
+ * ---------------------------------------------------------------------------
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag(1byte)|FCS(4bytes)
+ * ---------------------------------------------------------------------------
+ * tag : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
+ *
+ * For Egress (KSZ8795 -> Host), 1 byte is added before FCS.
+ * ---------------------------------------------------------------------------
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
+ * ---------------------------------------------------------------------------
+ * tag0 : zero-based value represents port
+ *	  (eg, 0x0=port1, 0x2=port3, 0x3=port4)
+ */
+
+#define KSZ8795_TAIL_TAG_EG_PORT_M	GENMASK(1, 0)
+#define KSZ8795_TAIL_TAG_OVERRIDE	BIT(6)
+#define KSZ8795_TAIL_TAG_LOOKUP		BIT(7)
+
+static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct ethhdr *hdr;
+	u8 *tag;
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	/* Tag encoding */
+	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
+	hdr = skb_eth_hdr(skb);
+
+	*tag = dsa_xmit_port_mask(skb, dev);
+	if (is_link_local_ether_addr(hdr->h_dest))
+		*tag |= KSZ8795_TAIL_TAG_OVERRIDE;
+
+	return skb;
+}
+
+static struct sk_buff *ksz8795_rcv(struct sk_buff *skb, struct net_device *dev)
+{
+	u8 *tag;
+
+	if (skb_linearize(skb)) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+
+	return ksz_common_rcv(skb, dev, tag[0] & KSZ8795_TAIL_TAG_EG_PORT_M,
+			      KSZ_EGRESS_TAG_LEN);
+}
+
+static const struct dsa_device_ops ksz8795_netdev_ops = {
+	.name	= KSZ8795_NAME,
+	.proto	= DSA_TAG_PROTO_KSZ8795,
+	.xmit	= ksz8795_xmit,
+	.rcv	= ksz8795_rcv,
+	.needed_tailroom = KSZ_INGRESS_TAG_LEN,
+};
+
+DSA_TAG_DRIVER(ksz8795_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795, KSZ8795_NAME);
+
 #define KSZ9893_TAIL_TAG_PRIO		GENMASK(4, 3)
 #define KSZ9893_TAIL_TAG_OVERRIDE	BIT(5)
 #define KSZ9893_TAIL_TAG_LOOKUP		BIT(6)

-- 
2.55.0

[PATCH net-next v3 07/10] net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations

[Bastien Curutchet (Schneider Electric)]

KSZ8795 and KSZ9893 have very similar tag handling in the xmit path,
leading to code duplication.

There are only two differences between the two ksz*_xmit():
- the KSZ8795 doesn't handle priorities between frames
- ksz8795_xmit() directly returns the SKB instead of calling
ksz_defer_xmit(). Yet, ksz_defer_xmit() also returns directly the SKB
if no clone is present inside the SKB. Clones are only created by the KSZ
driver when the PTP feature is enabled. Since KSZ8795 doesn't support
PTP, returning the SKB directly or ksz_defer_xmit() is the same.

The upcoming support for the KSZ8463 also requires a similar xmit().

Gather the common code from ksz8795_xmit() and ksz9893_xmit() into a new
ksz_common_xmit() function that takes three input arguments:
- do_tstamp to tell whether ksz_xmit_timestamp() should be called
- prio to give the priority tag (if any)
- override_mask to give the location of the override bit (if any)

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 net/dsa/tag_ksz.c | 73 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index f58ce0f0e9e4..f8b40437c5fa 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -218,6 +218,37 @@ static struct sk_buff *ksz_defer_xmit(struct dsa_port *dp, struct sk_buff *skb)
 	return NULL;
 }
 
+static struct sk_buff *ksz_common_xmit(struct sk_buff *skb,
+				       struct net_device *dev,
+				       bool do_tstamp,
+				       u8 prio,
+				       u8 override_mask)
+{
+	struct dsa_port *dp = dsa_user_to_port(dev);
+	struct ethhdr *hdr;
+	u8 *tag;
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	/* Tag encoding */
+	if (do_tstamp)
+		ksz_xmit_timestamp(dp, skb);
+
+	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
+	hdr = skb_eth_hdr(skb);
+
+	*tag = dsa_xmit_port_mask(skb, dev);
+	*tag |= prio;
+
+	if (is_link_local_ether_addr(hdr->h_dest))
+		*tag |= override_mask;
+
+	return ksz_defer_xmit(dp, skb);
+}
+
 static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
@@ -308,23 +339,7 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477, KSZ9477_NAME);
 
 static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
 {
-	struct ethhdr *hdr;
-	u8 *tag;
-
-	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
-		kfree_skb(skb);
-		return NULL;
-	}
-
-	/* Tag encoding */
-	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
-	hdr = skb_eth_hdr(skb);
-
-	*tag = dsa_xmit_port_mask(skb, dev);
-	if (is_link_local_ether_addr(hdr->h_dest))
-		*tag |= KSZ8795_TAIL_TAG_OVERRIDE;
-
-	return skb;
+	return ksz_common_xmit(skb, dev, false, 0, KSZ8795_TAIL_TAG_OVERRIDE);
 }
 
 static struct sk_buff *ksz8795_rcv(struct sk_buff *skb, struct net_device *dev)
@@ -362,28 +377,10 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 {
 	u16 queue_mapping = skb_get_queue_mapping(skb);
 	u8 prio = netdev_txq_to_tc(dev, queue_mapping);
-	struct dsa_port *dp = dsa_user_to_port(dev);
-	struct ethhdr *hdr;
-	u8 *tag;
-
-	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
-		kfree_skb(skb);
-		return NULL;
-	}
-
-	/* Tag encoding */
-	ksz_xmit_timestamp(dp, skb);
-
-	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
-	hdr = skb_eth_hdr(skb);
-
-	*tag = dsa_xmit_port_mask(skb, dev);
-	*tag |= FIELD_PREP(KSZ9893_TAIL_TAG_PRIO, prio);
 
-	if (is_link_local_ether_addr(hdr->h_dest))
-		*tag |= KSZ9893_TAIL_TAG_OVERRIDE;
-
-	return ksz_defer_xmit(dp, skb);
+	return ksz_common_xmit(skb, dev, true,
+			       FIELD_PREP(KSZ9893_TAIL_TAG_PRIO, prio),
+			       KSZ9893_TAIL_TAG_OVERRIDE);
 }
 
 static const struct dsa_device_ops ksz9893_netdev_ops = {

-- 
2.55.0

Re: [PATCH net-next v3 07/10] net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [7]: [PATCH net-next v3 07/10] net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations ---
commit 801d0cdc8ef18f0fbd60b1d666b46b36437fd06e
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations

This factors the duplicated tail-tag encoding of ksz8795_xmit() and
ksz9893_xmit() into a new ksz_common_xmit() helper parameterized by
do_tstamp, prio and override_mask.  As a side effect ksz8795_xmit() now
returns through ksz_defer_xmit() instead of returning the skb directly.

> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index f58ce0f0e9e42..f8b40437c5fa4 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -218,6 +218,37 @@ static struct sk_buff *ksz_defer_xmit(struct dsa_port *dp, struct sk_buff *skb)
>  	return NULL;
>  }
>  
> +static struct sk_buff *ksz_common_xmit(struct sk_buff *skb,
> +				       struct net_device *dev,
> +				       bool do_tstamp,
> +				       u8 prio,
> +				       u8 override_mask)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(dev);
> +	struct ethhdr *hdr;
> +	u8 *tag;
> +
> +	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
> +	/* Tag encoding */
> +	if (do_tstamp)
> +		ksz_xmit_timestamp(dp, skb);
> +
> +	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
> +	hdr = skb_eth_hdr(skb);
> +
> +	*tag = dsa_xmit_port_mask(skb, dev);
> +	*tag |= prio;
> +
> +	if (is_link_local_ether_addr(hdr->h_dest))
> +		*tag |= override_mask;
> +
> +	return ksz_defer_xmit(dp, skb);
> +}
> +

[Severity: Medium]
Can this dereference NULL for the KSZ8795 tagger?  ksz_defer_xmit() reads
ds->tagger_data and the tagger private data before using them:

net/dsa/tag_ksz.c:ksz_defer_xmit() {
	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dp->ds);
	struct ksz_tagger_private *priv = ksz_tagger_private(dp->ds);
	...
	if (!clone)
		return skb;  /* no deferred xmit for this packet */

	xmit_work_fn = tagger_data->xmit_work_fn;
	xmit_worker = priv->xmit_worker;
	...
}

Both pointers are only populated by ksz_connect(), and ksz8795_netdev_ops
does not install it:

static const struct dsa_device_ops ksz8795_netdev_ops = {
	.name	= KSZ8795_NAME,
	.proto	= DSA_TAG_PROTO_KSZ8795,
	.xmit	= ksz8795_xmit,
	.rcv	= ksz8795_rcv,
	.needed_tailroom = KSZ_INGRESS_TAG_LEN,
};

compared with ksz9893_netdev_ops which has .connect = ksz_connect and
.disconnect = ksz_disconnect.  ksz87xx_connect_tag_protocol() and
ksz88xx_connect_tag_protocol() in drivers/net/dsa/microchip/ksz8.c also
just return 0 for DSA_TAG_PROTO_KSZ8795, so ds->tagger_data stays NULL for
these switches.

So after this change the only thing keeping ksz8795_xmit() from a NULL
dereference is KSZ_SKB_CB(skb)->clone always being NULL.

Is that invariant actually enforced anywhere?  The ksz87xx/ksz88xx
dsa_switch_ops export .port_hwtstamp_set = ksz_hwtstamp_set and
.port_txtstamp = ksz_port_txtstamp unconditionally, dsa_user_hwtstamp_set()
adds no capability gate, and ksz_set_hwtstamp_config() sets the flag before
the register write and returns the error without reverting it, with no
dev->info->ptp_capable check:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_set_hwtstamp_config() {
	...
		prt->hwts_tx_en = true;

		ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_1STEP, PTP_1STEP);
		if (ret)
			return ret;
	...
}

For these chips regs[] is ksz8795_regs, whose last initialized index is
REG_PORT_PME_CTRL (23) while PTP_MSG_CONF1 is 28, so regs[PTP_MSG_CONF1] is
an out-of-bounds read and the write can well be rejected, which is exactly
the path that leaves hwts_tx_en latched.

Once hwts_tx_en is set on such a port, ksz_port_txtstamp() attaches a clone
based on that flag alone:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_port_txtstamp() {
	...
	KSZ_SKB_CB(skb)->clone = clone;
	...
}

and then dsa_user_xmit() -> ksz8795_xmit() -> ksz_common_xmit() ->
ksz_defer_xmit() gets past the "if (!clone)" early exit and loads
tagger_data->xmit_work_fn from a NULL pointer.  Before this patch
ksz8795_xmit() returned the skb directly and never touched ds->tagger_data,
so the same misconfiguration only leaked the clone.

The commit message states:

    Clones are only created by the KSZ driver when the PTP feature is
    enabled. Since KSZ8795 doesn't support PTP, returning the SKB directly
    or ksz_defer_xmit() is the same.

Could this assumption be expressed in the code, for instance by keying the
ksz_defer_xmit() call off do_tstamp, or by adding a DEBUG_NET_WARN_ON()
for a clone on a tagger without tagger_data?  Note also that
ksz8795_netdev_ops uses .needed_tailroom = KSZ_INGRESS_TAG_LEN only, so
there is no room for the 4 byte PTP tag if a future user of this helper
passes do_tstamp = true with that tailroom.

>  static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
>  				    struct net_device *dev)
>  {
> @@ -308,23 +339,7 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477, KSZ9477_NAME);
>  
>  static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
> -	struct ethhdr *hdr;
> -	u8 *tag;
> -
> -	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
> -		kfree_skb(skb);
> -		return NULL;
> -	}
> -
> -	/* Tag encoding */
> -	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
> -	hdr = skb_eth_hdr(skb);
> -
> -	*tag = dsa_xmit_port_mask(skb, dev);
> -	if (is_link_local_ether_addr(hdr->h_dest))
> -		*tag |= KSZ8795_TAIL_TAG_OVERRIDE;
> -
> -	return skb;
> +	return ksz_common_xmit(skb, dev, false, 0, KSZ8795_TAIL_TAG_OVERRIDE);
>  }

[ ... ]

[PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling

[Bastien Curutchet (Schneider Electric)]

KSZ8463 uses the KSZ9893 DSA TAG driver. However, the KSZ8463 doesn't
use the tail tag to convey timestamps to the host as KSZ9893 does. It
uses the reserved fields in the PTP header instead.

Add a KSZ8463-specific DSA_TAG driver to handle KSZ8463 timestamps.
There is no information in the tail tag to distinguish PTP packets from
others so use the ptp_classify_raw() helper to find the PTP packets and
extract the timestamp from their PTP headers.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c |  4 +--
 include/net/dsa.h                |  2 ++
 net/dsa/tag_ksz.c                | 66 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index 5e5bfc5cae2d..ac9e8ef5774a 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -2966,7 +2966,7 @@ static enum dsa_tag_protocol ksz8463_get_tag_protocol(struct dsa_switch *ds,
 						      int port,
 						      enum dsa_tag_protocol mp)
 {
-	return DSA_TAG_PROTO_KSZ9893;
+	return DSA_TAG_PROTO_KSZ8463;
 }
 
 static int ksz8463_connect_tag_protocol(struct dsa_switch *ds,
@@ -2974,7 +2974,7 @@ static int ksz8463_connect_tag_protocol(struct dsa_switch *ds,
 {
 	struct ksz_tagger_data *tagger_data;
 
-	if (proto != DSA_TAG_PROTO_KSZ9893)
+	if (proto != DSA_TAG_PROTO_KSZ8463)
 		return -EPROTONOSUPPORT;
 
 	tagger_data = ksz_tagger_data(ds);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8c16ef23cc10..6f7f5c17b532 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -59,6 +59,7 @@ struct tc_action;
 #define DSA_TAG_PROTO_MXL_GSW1XX_VALUE		31
 #define DSA_TAG_PROTO_MXL862_VALUE		32
 #define DSA_TAG_PROTO_NETC_VALUE		33
+#define DSA_TAG_PROTO_KSZ8463_VALUE		34
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -95,6 +96,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_MXL_GSW1XX	= DSA_TAG_PROTO_MXL_GSW1XX_VALUE,
 	DSA_TAG_PROTO_MXL862		= DSA_TAG_PROTO_MXL862_VALUE,
 	DSA_TAG_PROTO_NETC		= DSA_TAG_PROTO_NETC_VALUE,
+	DSA_TAG_PROTO_KSZ8463		= DSA_TAG_PROTO_KSZ8463_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index f8b40437c5fa..b4d70ba930dc 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -12,6 +12,7 @@
 
 #include "tag.h"
 
+#define KSZ8463_NAME "ksz8463"
 #define KSZ8795_NAME "ksz8795"
 #define KSZ9477_NAME "ksz9477"
 #define KSZ9893_NAME "ksz9893"
@@ -396,6 +397,70 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
 DSA_TAG_DRIVER(ksz9893_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893, KSZ9893_NAME);
 
+#define KSZ8463_TAIL_TAG_PRIO		GENMASK(4, 3)
+#define KSZ8463_TAIL_TAG_EG_PORT_M	GENMASK(2, 0)
+
+static struct sk_buff *ksz8463_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	u16 queue_mapping = skb_get_queue_mapping(skb);
+	u8 prio = netdev_txq_to_tc(dev, queue_mapping);
+
+	return ksz_common_xmit(skb, dev, false,
+			       FIELD_PREP(KSZ8463_TAIL_TAG_PRIO, prio),
+			       0);
+}
+
+static struct sk_buff *ksz8463_rcv(struct sk_buff *skb, struct net_device *dev)
+{
+	unsigned int len = KSZ_EGRESS_TAG_LEN;
+	struct ptp_header *ptp_hdr;
+	unsigned int ptp_class;
+	unsigned int port;
+	ktime_t ts;
+	u8 *tag;
+
+	if (skb_linearize(skb)) {
+		kfree_skb(skb);
+		return NULL;
+	}
+
+	KSZ_SKB_CB(skb)->tstamp = 0;
+
+	/* Tag decoding */
+	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+	port = tag[0] & KSZ8463_TAIL_TAG_EG_PORT_M;
+
+	__skb_push(skb, ETH_HLEN);
+	ptp_class = ptp_classify_raw(skb);
+	__skb_pull(skb, ETH_HLEN);
+	if (ptp_class == PTP_CLASS_NONE)
+		goto common_rcv;
+
+	ptp_hdr = ptp_parse_header(skb, ptp_class);
+	if (ptp_hdr) {
+		ts = ksz_decode_tstamp(get_unaligned_be32(&ptp_hdr->reserved2));
+		KSZ_SKB_CB(skb)->tstamp = ts;
+		ptp_hdr->reserved2 = 0;
+	}
+
+common_rcv:
+	return ksz_common_rcv(skb, dev, port, len);
+}
+
+static const struct dsa_device_ops ksz8463_netdev_ops = {
+	.name	= KSZ8463_NAME,
+	.proto	= DSA_TAG_PROTO_KSZ8463,
+	.xmit	= ksz8463_xmit,
+	.rcv	= ksz8463_rcv,
+	.connect = ksz_connect,
+	.disconnect = ksz_disconnect,
+	.needed_tailroom = KSZ_INGRESS_TAG_LEN,
+};
+
+DSA_TAG_DRIVER(ksz8463_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8463, KSZ8463_NAME);
+
 /* For xmit, 2/6 bytes are added before FCS.
  * ---------------------------------------------------------------------------
  * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1(1byte)|
@@ -468,6 +533,7 @@ DSA_TAG_DRIVER(lan937x_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_LAN937X, LAN937X_NAME);
 
 static struct dsa_tag_driver *dsa_tag_driver_array[] = {
+	&DSA_TAG_DRIVER_NAME(ksz8463_netdev_ops),
 	&DSA_TAG_DRIVER_NAME(ksz8795_netdev_ops),
 	&DSA_TAG_DRIVER_NAME(ksz9477_netdev_ops),
 	&DSA_TAG_DRIVER_NAME(ksz9893_netdev_ops),

-- 
2.55.0

Re: [PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling

[Jakub Kicinski]

On Mon, 27 Jul 2026 12:20:50 +0200 Bastien Curutchet (Schneider
Electric) wrote:
> KSZ8463 uses the KSZ9893 DSA TAG driver. However, the KSZ8463 doesn't
> use the tail tag to convey timestamps to the host as KSZ9893 does. It
> uses the reserved fields in the PTP header instead.

Sigh, some of the marvell switches seem to be doing the same thing.
Is this the _only_ option this switch supports?

Reserved fields in protocols are generally reserved, not free to use.

Re: [PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling

[Bastien Curutchet]

Hi Jakub,

On 7/31/26 4:23 AM, Jakub Kicinski wrote:
> On Mon, 27 Jul 2026 12:20:50 +0200 Bastien Curutchet (Schneider
> Electric) wrote:
>> KSZ8463 uses the KSZ9893 DSA TAG driver. However, the KSZ8463 doesn't
>> use the tail tag to convey timestamps to the host as KSZ9893 does. It
>> uses the reserved fields in the PTP header instead.
> 
> Sigh, some of the marvell switches seem to be doing the same thing.
> Is this the _only_ option this switch supports?
> 
> Reserved fields in protocols are generally reserved, not free to use.

No sadly, it's the only option this switch supports.


Best regards,
Bastien

Re: [PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [8]: [PATCH net-next v3 08/10] net: dsa: microchip: add KSZ8463 tail tag handling ---
commit 20a43ca1297a3640a458d9df2837f23e6b9191ea
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

net: dsa: microchip: add KSZ8463 tail tag handling

This adds a KSZ8463-specific DSA tag driver so that RX timestamps can be
taken from the reserved2 field of the PTP header instead of from the tail
tag, and switches ksz8463_get_tag_protocol() over to the new
DSA_TAG_PROTO_KSZ8463.  A few questions below about the new receive and
transmit paths.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 5e5bfc5cae2d..ac9e8ef5774a 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -2966,7 +2966,7 @@ static enum dsa_tag_protocol ksz8463_get_tag_protocol(struct dsa_switch *ds,
>  						      int port,
>  						      enum dsa_tag_protocol mp)
>  {
> -	return DSA_TAG_PROTO_KSZ9893;
> +	return DSA_TAG_PROTO_KSZ8463;
>  }
>  
>  static int ksz8463_connect_tag_protocol(struct dsa_switch *ds,
> @@ -2974,7 +2974,7 @@ static int ksz8463_connect_tag_protocol(struct dsa_switch *ds,
>  {
>  	struct ksz_tagger_data *tagger_data;
>  
> -	if (proto != DSA_TAG_PROTO_KSZ9893)
> +	if (proto != DSA_TAG_PROTO_KSZ8463)
>  		return -EPROTONOSUPPORT;

[Severity: Low]
This changes the user-visible tagging protocol name for KSZ8463 boards,
which is reachable both through the conduit's sysfs "tagging" attribute and
through the "dsa-tag-protocol" device tree property.

A board that currently pins the previous default:

	dsa-tag-protocol = "ksz9893";

now hits the -EPROTONOSUPPORT return in ksz8463_connect_tag_protocol()
instead of degrading to the old behaviour.  Would it be worth mentioning
this compatibility aspect in the commit message?

>  
>  	tagger_data = ksz_tagger_data(ds);

[ ... include/net/dsa.h hunks snipped ... ]

> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index f8b40437c5fa..b4d70ba930dc 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c

[ ... ]

> @@ -396,6 +397,70 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
>  DSA_TAG_DRIVER(ksz9893_netdev_ops);
>  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893, KSZ9893_NAME);
>  
> +#define KSZ8463_TAIL_TAG_PRIO		GENMASK(4, 3)
> +#define KSZ8463_TAIL_TAG_EG_PORT_M	GENMASK(2, 0)
> +
> +static struct sk_buff *ksz8463_xmit(struct sk_buff *skb,
> +				    struct net_device *dev)
> +{
> +	u16 queue_mapping = skb_get_queue_mapping(skb);
> +	u8 prio = netdev_txq_to_tc(dev, queue_mapping);
> +
> +	return ksz_common_xmit(skb, dev, false,
> +			       FIELD_PREP(KSZ8463_TAIL_TAG_PRIO, prio),
> +			       0);
> +}

[Severity: Medium]
Is the loss of the tail tag override bit intentional here?

Until this patch KSZ8463 used ksz9893_xmit(), which passes
KSZ9893_TAIL_TAG_OVERRIDE (BIT(5)) as the override_mask argument, and
ksz8795_xmit() likewise passes KSZ8795_TAIL_TAG_OVERRIDE.  ksz8463_xmit()
passes 0, which makes this branch of ksz_common_xmit() dead for the new
tagger:

	if (is_link_local_ether_addr(hdr->h_dest))
		*tag |= override_mask;

Does that mean CPU-injected link-local frames (STP BPDUs, peer delay
multicast, LLDP) are now subject to normal port state filtering, so a port
in BLOCKING or LISTENING state drops them?

KSZ8463_TAIL_TAG_PRIO is GENMASK(4, 3), i.e. the same layout as the KSZ9893
tail tag, which suggests the override bit does exist at BIT(5) on this part.
The KSZ8463 tail tag layout is not documented anywhere in the driver, so if
BIT(5) is in fact reserved on this chip, could the change (and the now
constant override_mask argument) get an explicit note?

[Severity: Medium]
The do_tstamp = false argument means ksz_xmit_timestamp() never runs for
KSZ8463, and that is the only consumer of
KSZ_SKB_CB(skb)->update_correction:

net/dsa/tag_ksz.c:ksz_xmit_timestamp() {
	...
	/* Set correction field to 0 and update UDP checksum */
	ptp_header_update_correction(skb, ptp_type, ptp_hdr, 0);
	...
}

ksz_port_txtstamp() in drivers/net/dsa/microchip/ksz_ptp.c still sets that
field and returns without cloning, expecting the tagger to do the work:

	case PTP_MSGTYPE_PDELAY_RESP:
		if (prt->tstamp_config.tx_type == HWTSTAMP_TX_ONESTEP_P2P) {
			KSZ_SKB_CB(skb)->ptp_type = type;
			KSZ_SKB_CB(skb)->update_correction = true;
			return;
		}

ksz_get_ts_info() also keeps advertising the mode for this chip:

	ts->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ONESTEP_P2P);

	if (is_lan937x(dev))
		ts->tx_types |= BIT(HWTSTAMP_TX_ON);

So user space can select HWTSTAMP_TX_ONESTEP_P2P on a KSZ8463 port while the
tag driver performs no correction field handling at all, and
ksz_port_rxtstamp() still subtracts the ingress timestamp from the
PDelay_Req correction in that mode.  Is the correction field expected to be
handled entirely in hardware here?  If so, could the commit message say so,
since the xmit side behaviour change is not mentioned?

> +
> +static struct sk_buff *ksz8463_rcv(struct sk_buff *skb, struct net_device *dev)
> +{
> +	unsigned int len = KSZ_EGRESS_TAG_LEN;
> +	struct ptp_header *ptp_hdr;
> +	unsigned int ptp_class;
> +	unsigned int port;
> +	ktime_t ts;
> +	u8 *tag;
> +
> +	if (skb_linearize(skb)) {
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
> +	KSZ_SKB_CB(skb)->tstamp = 0;
> +
> +	/* Tag decoding */
> +	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> +	port = tag[0] & KSZ8463_TAIL_TAG_EG_PORT_M;
> +
> +	__skb_push(skb, ETH_HLEN);
> +	ptp_class = ptp_classify_raw(skb);
> +	__skb_pull(skb, ETH_HLEN);

[Severity: Low]
This isn't a bug, but the DSA core runs the same classifier a few
instructions later and hands the result to the driver:

net/dsa/tag.c:dsa_skb_defer_rx_timestamp() {
	...
	__skb_push(skb, ETH_HLEN);
	type = ptp_classify_raw(skb);
	__skb_pull(skb, ETH_HLEN);
	if (type == PTP_CLASS_NONE)
		return false;
	return ds->ops->port_rxtstamp(ds, p->dp->index, skb, type);
}

and ksz_port_rxtstamp() already calls ptp_parse_header() itself.  So every
received frame on a KSZ8463 conduit now runs the PTP BPF classifier in the
tagger, even with hardware timestamping disabled, and PTP frames are
classified twice.

Unlike KSZ9477/KSZ9893, where the timestamp sits in the tail tag and has to
be consumed before ksz_common_rcv() trims it, ptp_hdr->reserved2 survives
into ksz_port_rxtstamp() untouched.  Could the reserved2 extraction live in
ksz_port_rxtstamp() instead, where the PTP class is already available?

> +	if (ptp_class == PTP_CLASS_NONE)
> +		goto common_rcv;
> +
> +	ptp_hdr = ptp_parse_header(skb, ptp_class);

[Severity: Medium]
Only PTP_CLASS_NONE is filtered here, but the code below interprets the
frame with the PTPv2 struct ptp_header layout.  ptp_classify_raw() also
returns PTP_CLASS_V1_IPV4/PTP_CLASS_V1_IPV6, as documented in
include/linux/ptp_classify.h:

 * PTP_CLASS_V1_IPV{4,6}, PTP_CLASS_V2_IPV{4,6} or
 * PTP_CLASS_V2_{L2,VLAN}, depending on the packet content.

and the classifier program simply masks the version nibble, so any value
1..15 comes back ORed with the encapsulation bits:

net/core/ptp_classifier.c:
	{ 0x48, 0, 0, 0x0016 },	/* ldh [22] */
	{ 0x54, 0, 0, 0x000f },	/* and #0xf */
	{ 0x44, 0, 0, 0x0010 },	/* or #0x10 */

ptp_parse_header() does no version filtering either, it only skips
VLAN/IP/UDP headers and bounds-checks sizeof(struct ptp_header).  For a
PTPv1 frame, offsets 8..11 are inside the 16-byte subdomain field rather
than a reserved field, so real header content would be read as a timestamp
and then zeroed.  Should there be a "if (!(ptp_class & PTP_CLASS_V2))" gate
before the reserved2 access?

> +	if (ptp_hdr) {
> +		ts = ksz_decode_tstamp(get_unaligned_be32(&ptp_hdr->reserved2));
> +		KSZ_SKB_CB(skb)->tstamp = ts;
> +		ptp_hdr->reserved2 = 0;
> +	}

[Severity: Medium]
Is there anything that keeps this from running while the switch PTP unit is
disabled?  The tagger never looks at KSZ_HWTS_EN here, and does not even
fetch ksz_tagger_private(), while the transmit side in the same file does
gate on it:

net/dsa/tag_ksz.c:ksz_xmit_timestamp() {
	...
	if (!test_bit(KSZ_HWTS_EN, &priv->state))
		return;
	...
}

That bit is set together with PTP_ENABLE in ksz_ptp_enable_mode():

	tagger_data->hwtstamp_set_state(dev->ds, tag_en);

	return ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_ENABLE,
			 tag_en ? PTP_ENABLE : 0);

so "PTP disabled" is the default state, in which the hardware never wrote
anything into reserved2.  In that state, does this code destroy 4 bytes of a
genuine remote frame (messageTypeSpecific in IEEE 1588-2019) and build
KSZ_SKB_CB(skb)->tstamp out of remote bytes?  ksz_port_rxtstamp() then
publishes it with no prt->hwts_rx_en test:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_port_rxtstamp() {
	...
	tstamp = KSZ_SKB_CB(skb)->tstamp;
	memset(hwtstamps, 0, sizeof(*hwtstamps));
	hwtstamps->hwtstamp = ksz_tstamp_reconstruct(dev, tstamp);
	...
}

ksz9477_rcv() only trusts the timestamp when the hardware sets
KSZ9477_PTP_TAG_INDICATION, and the commit message says KSZ8463 provides no
such indication.  For comparison, hellcreek gates on the enable bit before
touching reserved2, in hellcreek_should_tstamp():

	if (!test_bit(HELLCREEK_HWTSTAMP_ENABLED, &ps->state))
		return NULL;

Would the same gating be appropriate here?

[Severity: Medium]
Does the "ptp_hdr->reserved2 = 0" store need checksum bookkeeping?  For PTP
over UDP (advertised by ksz_get_ts_info() through
HWTSTAMP_FILTER_PTP_V2_L4_EVENT) these 4 bytes are covered by the UDP
checksum, but neither the checksum field nor skb->csum / skb->ip_summed are
updated.

On a conduit reporting CHECKSUM_COMPLETE, skb->csum then still describes the
unmodified bytes and is carried forward by ksz_common_rcv():

	if (pskb_trim_rcsum(skb, skb->len - len)) {

In HWTSTAMP_TX_ONESTEP_P2P mode, ksz_port_rxtstamp() additionally calls
ptp_header_update_correction() on the same frame, which does:

include/linux/ptp_classify.h:ptp_header_update_correction() {
	...
	uhdr->check = csum_fold(ptp_check_diff8(correction_old, hdr->correction,
						~csum_unfold(uhdr->check)));
	...
	skb->ip_summed = CHECKSUM_NONE;
}

so the stack recomputes the UDP checksum over data whose reserved2 delta was
never accounted for.  If the switch updates the UDP checksum when it inserts
the timestamp, would PDelay_Req frames then be dropped in udp_rcv()?  The
same file already uses ptp_header_update_correction() from
ksz_xmit_timestamp() for in-place PTP header edits.

[ ... remaining tag_ksz.c hunks snipped ... ]

[PATCH net-next v3 09/10] net: dsa: microchip: explicitly enable detection of L2 PTP frames

[Bastien Curutchet (Schneider Electric)]

Detection of L2 PTP frames needs to be enabled for PTP to work at the L2
layer. The bit enabling this detection is set by default on the switches
currently supported by the driver, but it is unset by default on the
KSZ8463 for which support will be added in upcoming patches.

Explicitly enable the detection of L2 PTP frames for all switches when
PTP is enabled.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 89fbc1253f25..be6b8240ac03 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -953,8 +953,9 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
 	/* Currently only P2P mode is supported. When 802_1AS bit is set, it
 	 * forwards all PTP packets to host port and none to other ports.
 	 */
-	ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_TC_P2P | PTP_802_1AS,
-			PTP_TC_P2P | PTP_802_1AS);
+	ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1],
+			PTP_TC_P2P | PTP_802_1AS | PTP_ETH_ENABLE,
+			PTP_TC_P2P | PTP_802_1AS | PTP_ETH_ENABLE);
 	if (ret)
 		return ret;
 

-- 
2.55.0

[PATCH net-next v3 10/10] net: dsa: microchip: add two-steps PTP support for KSZ8463

[Bastien Curutchet (Schneider Electric)]

The KSZ8463 switch supports PTP but it's not supported by the driver.

Add L2 two-step PTP support for the KSZ8463. IPv4 and IPv6 layers aren't
supported. Neither is one-step PTP. Use KSZ8463-specific implementations
of the .get_ts_info and .port_hwtstamp_set callbacks.

The pdelay_req and pdelay_resp timestamps share one interrupt bit status
while they're located in two different registers. So introduce
last_tx_is_pdelayresp to keep track of the last sent event type. This
flag is set by the xmit worker right before sending the packet and then
used in the interrupt handler to retrieve the timestamp location.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c        |  26 ++++--
 drivers/net/dsa/microchip/ksz8_reg.h    |   1 +
 drivers/net/dsa/microchip/ksz_common.h  |   1 +
 drivers/net/dsa/microchip/ksz_ptp.c     | 135 +++++++++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_ptp.h     |   7 ++
 drivers/net/dsa/microchip/ksz_ptp_reg.h |   4 +
 6 files changed, 167 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index ac9e8ef5774a..941ae9f66f70 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -242,8 +242,11 @@ static int ksz8463_girq_setup(struct ksz_device *dev)
 
 static int ksz8463_reset_switch(struct ksz_device *dev)
 {
-	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
-	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET,
+	ksz_cfg(dev, KSZ8463_REG_SW_RESET,
+		KSZ8463_GLOBAL_SOFTWARE_RESET | KSZ8463_PTP_SOFTWARE_RESET,
+		true);
+	ksz_cfg(dev, KSZ8463_REG_SW_RESET,
+		KSZ8463_GLOBAL_SOFTWARE_RESET | KSZ8463_PTP_SOFTWARE_RESET,
 		false);
 	return 0;
 }
@@ -2474,17 +2477,24 @@ static int ksz8463_setup(struct dsa_switch *ds)
 		ret = ksz8463_ptp_irq_setup(ds);
 		if (ret)
 			goto free_girq;
+
+		ret = ksz_ptp_clock_register(ds);
+		if (ret) {
+			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
+				ret);
+			goto free_ptp_irq;
+		}
 	}
 
 	ret = ksz_mdio_register(dev);
 	if (ret < 0) {
 		dev_err(dev->dev, "failed to register the mdio");
-		goto free_ptp_irq;
+		goto ptp_clock_unregister;
 	}
 
 	ret = ksz_dcb_init(dev);
 	if (ret)
-		goto free_ptp_irq;
+		goto ptp_clock_unregister;
 
 	/* start switch */
 	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
@@ -2492,6 +2502,9 @@ static int ksz8463_setup(struct dsa_switch *ds)
 
 	return 0;
 
+ptp_clock_unregister:
+	if (dev->irq > 0)
+		ksz_ptp_clock_unregister(ds);
 free_ptp_irq:
 	if (dev->irq > 0)
 		ksz8463_ptp_irq_free(ds);
@@ -2507,6 +2520,7 @@ static void ksz8463_teardown(struct dsa_switch *ds)
 	struct ksz_device *dev = ds->priv;
 
 	if (dev->irq > 0) {
+		ksz_ptp_clock_unregister(ds);
 		ksz8463_ptp_irq_free(ds);
 		ksz_irq_free(&dev->girq);
 	}
@@ -3129,9 +3143,9 @@ const struct dsa_switch_ops ksz8463_switch_ops = {
 	.port_max_mtu		= ksz88xx_max_mtu,
 	.suspend		= ksz_suspend,
 	.resume			= ksz_resume,
-	.get_ts_info		= ksz_get_ts_info,
+	.get_ts_info		= ksz8463_get_ts_info,
 	.port_hwtstamp_get	= ksz_hwtstamp_get,
-	.port_hwtstamp_set	= ksz_hwtstamp_set,
+	.port_hwtstamp_set	= ksz8463_hwtstamp_set,
 	.port_txtstamp		= ksz_port_txtstamp,
 	.port_rxtstamp		= ksz_port_rxtstamp,
 	.port_setup_tc		= ksz8_setup_tc,
diff --git a/drivers/net/dsa/microchip/ksz8_reg.h b/drivers/net/dsa/microchip/ksz8_reg.h
index 981ab441d9b7..6bc511da1f7d 100644
--- a/drivers/net/dsa/microchip/ksz8_reg.h
+++ b/drivers/net/dsa/microchip/ksz8_reg.h
@@ -786,6 +786,7 @@
 #define KSZ8463_REG_SW_RESET		0x126
 
 #define KSZ8463_GLOBAL_SOFTWARE_RESET	BIT(0)
+#define KSZ8463_PTP_SOFTWARE_RESET	BIT(2)
 
 #define KSZ8463_PTP_CLK_CTRL		0x600
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 0f2abb22ca91..cbe98494578c 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -194,6 +194,7 @@ struct ksz_port {
 	struct kernel_hwtstamp_config tstamp_config;
 	bool hwts_tx_en;
 	bool hwts_rx_en;
+	bool last_tx_is_pdelayresp;
 	struct ksz_irq ptpirq;
 	struct ksz_ptp_irq ptpmsg_irq[3];
 	ktime_t tstamp_msg;
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index be6b8240ac03..9d8b78687f53 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -297,6 +297,31 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
 			 tag_en ? PTP_ENABLE : 0);
 }
 
+int ksz8463_get_ts_info(struct dsa_switch *ds, int port,
+			struct kernel_ethtool_ts_info *ts)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_ptp_data *ptp_data;
+
+	ptp_data = &dev->ptp_data;
+
+	if (!ptp_data->clock)
+		return -ENODEV;
+
+	ts->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
+			      SOF_TIMESTAMPING_RX_HARDWARE |
+			      SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	ts->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
+
+	ts->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
+			 BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT);
+
+	ts->phc_index = ptp_clock_index(ptp_data->clock);
+
+	return 0;
+}
+
 /* The function is return back the capability of timestamping feature when
  * requested through ethtool -T <interface> utility
  */
@@ -341,6 +366,72 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int ksz8463_set_hwtstamp_config(struct ksz_device *dev,
+				       struct ksz_port *prt,
+				       struct kernel_hwtstamp_config *config)
+{
+	const u16 *regs = dev->info->regs;
+	int ret;
+
+	if (config->flags)
+		return -EINVAL;
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		prt->ptpmsg_irq[KSZ8463_SYNC_MSG].ts_en  = false;
+		prt->ptpmsg_irq[KSZ8463_XDREQ_PDRES_MSG].ts_en = false;
+		prt->hwts_tx_en = false;
+		break;
+	case HWTSTAMP_TX_ON:
+		prt->ptpmsg_irq[KSZ8463_SYNC_MSG].ts_en  = true;
+		prt->ptpmsg_irq[KSZ8463_XDREQ_PDRES_MSG].ts_en = true;
+		prt->hwts_tx_en = true;
+
+		ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_1STEP, 0);
+		if (ret)
+			return ret;
+
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		prt->hwts_rx_en = false;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		prt->hwts_rx_en = true;
+		break;
+	default:
+		config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	return ksz_ptp_enable_mode(dev);
+}
+
+int ksz8463_hwtstamp_set(struct dsa_switch *ds, int port,
+			 struct kernel_hwtstamp_config *config,
+			 struct netlink_ext_ack *extack)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *prt;
+	int ret;
+
+	prt = &dev->ports[port];
+
+	ret = ksz8463_set_hwtstamp_config(dev, prt, config);
+	if (ret)
+		return ret;
+
+	prt->tstamp_config = *config;
+
+	return 0;
+}
+
 static int ksz_set_hwtstamp_config(struct ksz_device *dev,
 				   struct ksz_port *prt,
 				   struct kernel_hwtstamp_config *config)
@@ -571,6 +662,31 @@ static void ksz_ptp_txtstamp_skb(struct ksz_device *dev,
 	skb_complete_tx_timestamp(skb, &hwtstamps);
 }
 
+static void ksz8463_set_pdelayresp_flag(struct ksz_port *prt,
+					struct sk_buff *skb)
+{
+	struct ptp_header *hdr;
+	unsigned int type;
+	u8 ptp_msg_type;
+
+	if (!ksz_is_ksz8463(prt->ksz_dev))
+		return;
+
+	if (skb_linearize(skb))
+		return;
+
+	type = ptp_classify_raw(skb);
+	if (type == PTP_CLASS_NONE)
+		return;
+
+	hdr = ptp_parse_header(skb, type);
+	if (!hdr)
+		return;
+
+	ptp_msg_type = ptp_get_msgtype(hdr, type);
+	prt->last_tx_is_pdelayresp = (ptp_msg_type == PTP_MSGTYPE_PDELAY_RESP);
+}
+
 void ksz_port_deferred_xmit(struct kthread_work *work)
 {
 	struct ksz_deferred_xmit_work *xmit_work = work_to_xmit_work(work);
@@ -587,6 +703,8 @@ void ksz_port_deferred_xmit(struct kthread_work *work)
 
 	reinit_completion(&prt->tstamp_msg_comp);
 
+	ksz8463_set_pdelayresp_flag(prt, skb);
+
 	dsa_enqueue_skb(skb, skb->dev);
 
 	ksz_ptp_txtstamp_skb(dev, prt, clone);
@@ -979,7 +1097,22 @@ void ksz_ptp_clock_unregister(struct dsa_switch *ds)
 
 static int ksz_read_ts(struct ksz_port *port, u16 reg, u32 *ts)
 {
-	return ksz_read32(port->ksz_dev, reg, ts);
+	u16 ts_reg = reg;
+
+	/**
+	 * On KSZ8463 DREQ and DRESP timestamps share one interrupt line
+	 * so we have to check the nature of the latest event sent to know
+	 * where the timestamp is located
+	 */
+	if (ksz_is_ksz8463(port->ksz_dev)) {
+		const struct ksz_dev_ops *ops = port->ksz_dev->dev_ops;
+
+		if (port->last_tx_is_pdelayresp &&
+		    ts_reg == ops->get_port_addr(port->num, KSZ8463_REG_PORT_DREQ_TS))
+			ts_reg += KSZ8463_DRESP_TS_OFFSET;
+	}
+
+	return ksz_read32(port->ksz_dev, ts_reg, ts);
 }
 
 static irqreturn_t ksz_ptp_msg_thread_fn(int irq, void *dev_id)
diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
index 11408580031d..7067ec9bd1e6 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.h
+++ b/drivers/net/dsa/microchip/ksz_ptp.h
@@ -39,11 +39,16 @@ void ksz_ptp_clock_unregister(struct dsa_switch *ds);
 
 int ksz_get_ts_info(struct dsa_switch *ds, int port,
 		    struct kernel_ethtool_ts_info *ts);
+int ksz8463_get_ts_info(struct dsa_switch *ds, int port,
+			struct kernel_ethtool_ts_info *ts);
 int ksz_hwtstamp_get(struct dsa_switch *ds, int port,
 		     struct kernel_hwtstamp_config *config);
 int ksz_hwtstamp_set(struct dsa_switch *ds, int port,
 		     struct kernel_hwtstamp_config *config,
 		     struct netlink_ext_ack *extack);
+int ksz8463_hwtstamp_set(struct dsa_switch *ds, int port,
+			 struct kernel_hwtstamp_config *config,
+			 struct netlink_ext_ack *extack);
 void ksz_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
 void ksz_port_deferred_xmit(struct kthread_work *work);
 bool ksz_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
@@ -82,10 +87,12 @@ static inline int ksz8463_ptp_irq_setup(struct dsa_switch *ds)
 static inline void ksz8463_ptp_irq_free(struct dsa_switch *ds) {}
 
 #define ksz_get_ts_info NULL
+#define ksz8463_get_ts_info NULL
 
 #define ksz_hwtstamp_get NULL
 
 #define ksz_hwtstamp_set NULL
+#define ksz8463_hwtstamp_set NULL
 
 #define ksz_port_rxtstamp NULL
 
diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h b/drivers/net/dsa/microchip/ksz_ptp_reg.h
index 1a669d6ee889..65ea8577af75 100644
--- a/drivers/net/dsa/microchip/ksz_ptp_reg.h
+++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
@@ -137,4 +137,8 @@
 #define KSZ_XDREQ_MSG			1
 #define KSZ_PDRES_MSG			0
 
+#define KSZ8463_DRESP_TS_OFFSET		(KSZ8463_REG_PORT_DRESP_TS - KSZ8463_REG_PORT_DREQ_TS)
+#define KSZ8463_SYNC_MSG		0
+#define KSZ8463_XDREQ_PDRES_MSG		1
+
 #endif

-- 
2.55.0

Re: [PATCH net-next v3 10/10] net: dsa: microchip: add two-steps PTP support for KSZ8463

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [10]: [PATCH net-next v3 10/10] net: dsa: microchip: add two-steps PTP support for KSZ8463 ---
commit 17314cc888f74f2fde26bc0da770a948737b2b66
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

net: dsa: microchip: add two-steps PTP support for KSZ8463

This adds L2 two-step PTP timestamping for the KSZ8463 by registering the
generic KSZ PTP clock and adding KSZ8463-specific .get_ts_info and
.port_hwtstamp_set callbacks.  A new per-port last_tx_is_pdelayresp flag is
used to decide which of the two timestamp registers behind the shared
interrupt status bit should be read.

A few questions below.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index ac9e8ef5774a..941ae9f66f70 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -242,8 +242,11 @@ static int ksz8463_girq_setup(struct ksz_device *dev)
>  
>  static int ksz8463_reset_switch(struct ksz_device *dev)
>  {
> -	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
> -	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET,
> +	ksz_cfg(dev, KSZ8463_REG_SW_RESET,
> +		KSZ8463_GLOBAL_SOFTWARE_RESET | KSZ8463_PTP_SOFTWARE_RESET,
> +		true);
> +	ksz_cfg(dev, KSZ8463_REG_SW_RESET,
> +		KSZ8463_GLOBAL_SOFTWARE_RESET | KSZ8463_PTP_SOFTWARE_RESET,
>  		false);
>  	return 0;
>  }

[Severity: Low]
ksz8463_reset_switch() is called unconditionally from ksz8463_setup(),
before and independently of the "if (dev->irq > 0)" block, and
independently of CONFIG_NET_DSA_MICROCHIP_KSZ_PTP.

So the PTP reset domain (BIT(2) of 0x126) is now toggled also on boards
that never register a PTP clock, and in kernels built without the KSZ PTP
support.

The changelog only describes adding two-step PTP support and does not
mention resetting the PTP block.  Could the rationale be added to the
commit message, or the extra reset bit be moved into the PTP setup path?

> @@ -2474,17 +2477,24 @@ static int ksz8463_setup(struct dsa_switch *ds)
>  		ret = ksz8463_ptp_irq_setup(ds);
>  		if (ret)
>  			goto free_girq;
> +
> +		ret = ksz_ptp_clock_register(ds);
> +		if (ret) {
> +			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
> +				ret);
> +			goto free_ptp_irq;
> +		}
>  	}

[Severity: Medium]
Does registering the generic clock here also advertise periodic output and
programmable pins that the KSZ8463 path does not implement?

ksz_ptp_clock_register() fills the capabilities unconditionally for every
chip:

	ptp_data->caps.enable		= ksz_ptp_enable;
	ptp_data->caps.verify		= ksz_ptp_verify_pin;
	ptp_data->caps.n_pins		= KSZ_PTP_N_GPIO;
	ptp_data->caps.n_per_out	= 3;
	...
	ptp_data->caps.pin_config = ptp_data->pin_config;

The trigger-output code behind those callbacks uses absolute KSZ9477 and
LAN937x register literals rather than dev->info->regs[], for example in
ksz_ptp_tou_reset() and ksz_ptp_configure_perout():

	ksz_rmw32(dev, REG_PTP_UNIT_INDEX__4,
		  PTP_GPIO_INDEX | PTP_TOU_INDEX, data32);   /* 0x0520 */
	...
	ksz_write32(dev, REG_TRIG_TARGET_NANOSEC, ...);     /* 0x0530+ */

On KSZ8463 the PTP block lives at 0x600 and above (ksz8463_regs[]:
PTP_CLK_CTRL 0x0600, PTP_MSG_CONF1 0x0620), and ksz8463_get_port_addr()
only relocates offsets >= 0x600, so 0x0520-0x0548 are not KSZ8463 PTP
registers.  The KSZ8463 32-bit regmap declares max_register = BIT(13) - 1
and the KSZ8463 entry in ksz_switch_chips[] has neither wr_table nor
rd_table, so these accesses are not filtered and reach the SPI bus.

ksz_ptp_verify_pin() accepts PTP_PF_PEROUT, and ksz_ptp_tou_gpio() returns
0 for anything that is not LAN937x, so a PTP_PIN_SETFUNC followed by
PTP_PEROUT_REQUEST on the new clock can report success while writing into
unrelated registers.  Should the pin/perout capabilities be left out for
this chip?

> @@ -3129,9 +3143,9 @@ const struct dsa_switch_ops ksz8463_switch_ops = {
>  	.port_max_mtu		= ksz88xx_max_mtu,
>  	.suspend		= ksz_suspend,
>  	.resume			= ksz_resume,
> -	.get_ts_info		= ksz_get_ts_info,
> +	.get_ts_info		= ksz8463_get_ts_info,
>  	.port_hwtstamp_get	= ksz_hwtstamp_get,
> -	.port_hwtstamp_set	= ksz_hwtstamp_set,
> +	.port_hwtstamp_set	= ksz8463_hwtstamp_set,
>  	.port_txtstamp		= ksz_port_txtstamp,
>  	.port_rxtstamp		= ksz_port_rxtstamp,
>  	.port_setup_tc		= ksz8_setup_tc,

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index be6b8240ac03..9d8b78687f53 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -297,6 +297,31 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
>  			 tag_en ? PTP_ENABLE : 0);
>  }
>  
> +int ksz8463_get_ts_info(struct dsa_switch *ds, int port,
> +			struct kernel_ethtool_ts_info *ts)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_ptp_data *ptp_data;
> +
> +	ptp_data = &dev->ptp_data;
> +
> +	if (!ptp_data->clock)
> +		return -ENODEV;
> +
> +	ts->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> +			      SOF_TIMESTAMPING_RX_HARDWARE |
> +			      SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	ts->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
> +
> +	ts->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> +			 BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT);
> +
> +	ts->phc_index = ptp_clock_index(ptp_data->clock);
> +
> +	return 0;
> +}
> +

[Severity: Low]
This isn't a bug, but this is a near-verbatim copy of ksz_get_ts_info(),
with only tx_types and rx_filters differing, and ksz8463_hwtstamp_set()
below is a byte-for-byte copy of ksz_hwtstamp_set() apart from the inner
helper call.

ksz_get_ts_info() already differentiates chips inline:

	if (is_lan937x(dev))
		ts->tx_types |= BIT(HWTSTAMP_TX_ON);

and ksz_set_hwtstamp_config() does the same with:

	if (!is_lan937x(dev))
		return -ERANGE;

Could the KSZ8463 differences be expressed the same way, or through
ksz_chip_data fields, instead of a second pair of callbacks plus a second
set of "#define ... NULL" stubs in ksz_ptp.h?  As it stands the missing
clock check discussed below has to be fixed in two places.

> @@ -341,6 +366,72 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static int ksz8463_set_hwtstamp_config(struct ksz_device *dev,
> +				       struct ksz_port *prt,
> +				       struct kernel_hwtstamp_config *config)
> +{
> +	const u16 *regs = dev->info->regs;
> +	int ret;
> +
> +	if (config->flags)
> +		return -EINVAL;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_OFF:
> +		prt->ptpmsg_irq[KSZ8463_SYNC_MSG].ts_en  = false;
> +		prt->ptpmsg_irq[KSZ8463_XDREQ_PDRES_MSG].ts_en = false;
> +		prt->hwts_tx_en = false;
> +		break;
> +	case HWTSTAMP_TX_ON:
> +		prt->ptpmsg_irq[KSZ8463_SYNC_MSG].ts_en  = true;
> +		prt->ptpmsg_irq[KSZ8463_XDREQ_PDRES_MSG].ts_en = true;
> +		prt->hwts_tx_en = true;
> +
> +		ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_1STEP, 0);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	switch (config->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		prt->hwts_rx_en = false;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> +		prt->hwts_rx_en = true;
> +		break;
> +	default:
> +		config->rx_filter = HWTSTAMP_FILTER_NONE;
> +		return -ERANGE;
> +	}
> +
> +	return ksz_ptp_enable_mode(dev);
> +}

[Severity: Medium]
Only L2 filters are accepted here and the changelog says "IPv4 and IPv6
layers aren't supported", but should PTP_IPV4_UDP_ENABLE (BIT(4)) and
PTP_IPV6_UDP_ENABLE (BIT(3)) of PTP_MSG_CONF1 be cleared somewhere?

The only writes to that register are the set-only rmw in
ksz_ptp_clock_register():

	ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1],
			PTP_TC_P2P | PTP_802_1AS | PTP_ETH_ENABLE,
			PTP_TC_P2P | PTP_802_1AS | PTP_ETH_ENABLE);

and the PTP_1STEP clear above, so the transport-detect bits keep their
reset value.  With the new PTP soft reset in ksz8463_reset_switch() the
register is back at its hardware default, and the existing comment in
ksz8463_config_cpu_port() suggests the default is not zero on this chip:

	/* Turn off PTP function as the switch enables it by default */
	regmap_update_bits(ksz_regmap_16(dev), KSZ8463_PTP_MSG_CONF1,
			   PTP_ENABLE, 0);

If BIT(3)/BIT(4) are set after reset, UDP-encapsulated PTP frames are
detected too, and since PTP_802_1AS forwards detected PTP frames to the
host port only, L4 PTP traffic crossing the switch would be trapped and
could raise timestamp interrupts the driver does not expect.

> +
> +int ksz8463_hwtstamp_set(struct dsa_switch *ds, int port,
> +			 struct kernel_hwtstamp_config *config,
> +			 struct netlink_ext_ack *extack)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *prt;
> +	int ret;
> +
> +	prt = &dev->ports[port];
> +
> +	ret = ksz8463_set_hwtstamp_config(dev, prt, config);
> +	if (ret)
> +		return ret;
> +
> +	prt->tstamp_config = *config;
> +
> +	return 0;
> +}
> +

[Severity: High]
This isn't a bug introduced by this patch, but can this path be reached on
a KSZ8463 without a PTP clock and oops?

ksz8463_get_ts_info() added above returns -ENODEV when ptp_data->clock is
NULL, while ksz8463_hwtstamp_set() has no such check and always ends in
ksz_ptp_enable_mode(), which does:

	if (tag_en)
		ptp_schedule_worker(ptp_data->clock, 0);
	else
		ptp_cancel_worker_sync(ptp_data->clock);

ksz8463_setup() only calls ksz_ptp_clock_register() (the sole place where
ptp_data->lock, ptp_data->clock_lock and ptp_data->clock are initialized)
inside "if (dev->irq > 0)", but ksz8463_switch_ops installs
.port_hwtstamp_set unconditionally, and dsa_user_hwtstamp_set() only
checks that the op pointer exists.

dev->irq comes from spi->irq in ksz_spi_probe() and is 0 when the DT node
describes no interrupt, which the driver supports.  In that case even a
trivial HWTSTAMP_TX_OFF plus HWTSTAMP_FILTER_NONE request ends in
ptp_cancel_worker_sync(NULL), which passes &ptp->aux_work to
kthread_cancel_delayed_work_sync().

The same hole existed before with the generic ksz_hwtstamp_set(), but the
new KSZ8463-specific copy keeps it while making the clock and lock
initialization conditional.  Would a "!ptp_data->clock" check, as done in
the new ksz8463_get_ts_info(), be appropriate here?

> @@ -571,6 +662,31 @@ static void ksz_ptp_txtstamp_skb(struct ksz_device *dev,
>  	skb_complete_tx_timestamp(skb, &hwtstamps);
>  }
>  
> +static void ksz8463_set_pdelayresp_flag(struct ksz_port *prt,
> +					struct sk_buff *skb)
> +{
> +	struct ptp_header *hdr;
> +	unsigned int type;
> +	u8 ptp_msg_type;
> +
> +	if (!ksz_is_ksz8463(prt->ksz_dev))
> +		return;
> +
> +	if (skb_linearize(skb))
> +		return;
> +
> +	type = ptp_classify_raw(skb);
> +	if (type == PTP_CLASS_NONE)
> +		return;
> +
> +	hdr = ptp_parse_header(skb, type);
> +	if (!hdr)
> +		return;
> +
> +	ptp_msg_type = ptp_get_msgtype(hdr, type);
> +	prt->last_tx_is_pdelayresp = (ptp_msg_type == PTP_MSGTYPE_PDELAY_RESP);
> +}
> +
>  void ksz_port_deferred_xmit(struct kthread_work *work)
>  {
>  	struct ksz_deferred_xmit_work *xmit_work = work_to_xmit_work(work);
> @@ -587,6 +703,8 @@ void ksz_port_deferred_xmit(struct kthread_work *work)
>  
>  	reinit_completion(&prt->tstamp_msg_comp);
>  
> +	ksz8463_set_pdelayresp_flag(prt, skb);
> +
>  	dsa_enqueue_skb(skb, skb->dev);
>  
>  	ksz_ptp_txtstamp_skb(dev, prt, clone);

[Severity: Medium]
Can a late timestamp interrupt observe the flag of the following packet?

prt->last_tx_is_pdelayresp is written here from the deferred-xmit kthread
and read from the threaded PTP interrupt in ksz_ptp_msg_thread_fn() ->
ksz_read_ts(), with no lock, no READ_ONCE()/WRITE_ONCE() and no barrier.

Correctness seems to depend on the interrupt for packet N always being
consumed before packet N+1 is queued, but ksz_ptp_txtstamp_skb() gives up
after a timeout:

	ret = wait_for_completion_timeout(&prt->tstamp_msg_comp,
					  msecs_to_jiffies(100));
	if (!ret)
		return;

So for a Pdelay_Resp whose interrupt is late, the worker can proceed to
the next frame (say a Pdelay_Req) and set the flag to false; the delayed
interrupt for the Pdelay_Resp then reads KSZ8463_REG_PORT_DREQ_TS instead
of DREQ_TS + KSZ8463_DRESP_TS_OFFSET and completes tstamp_msg_comp, so
that value is handed to skb_complete_tx_timestamp() for the next packet.

reinit_completion() is a plain "x->done = 0" while complete() takes
x->wait.lock, so it cannot discard the stale completion either.  Is there
something that ties an interrupt to the packet it belongs to?

> @@ -979,7 +1097,22 @@ void ksz_ptp_clock_unregister(struct dsa_switch *ds)
>  
>  static int ksz_read_ts(struct ksz_port *port, u16 reg, u32 *ts)
>  {
> -	return ksz_read32(port->ksz_dev, reg, ts);
> +	u16 ts_reg = reg;
> +
> +	/**
      ^^^^

[Severity: Low]
This isn't a bug, but "/**" is the kernel-doc marker and this is an
ordinary comment inside a function body, so plain "/*" would be better.

> +	 * On KSZ8463 DREQ and DRESP timestamps share one interrupt line
                                                          ^^^^^^^^^^^^^^

[Severity: Low]
The commit message says the two timestamps "share one interrupt bit
status", which matches the hardware: KSZ8463_REG_PORT_DREQ_TS and
KSZ8463_REG_PORT_DRESP_TS share a single status bit in
KSZ8463_PTP_TS_ISR rather than sitting on separate interrupt lines.  The
sentence is also missing a terminating period.

> +	 * so we have to check the nature of the latest event sent to know
> +	 * where the timestamp is located
> +	 */
> +	if (ksz_is_ksz8463(port->ksz_dev)) {
> +		const struct ksz_dev_ops *ops = port->ksz_dev->dev_ops;
> +
> +		if (port->last_tx_is_pdelayresp &&
> +		    ts_reg == ops->get_port_addr(port->num, KSZ8463_REG_PORT_DREQ_TS))
> +			ts_reg += KSZ8463_DRESP_TS_OFFSET;
> +	}
> +
> +	return ksz_read32(port->ksz_dev, ts_reg, ts);
>  }

[ ... ]