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

[Jakub Kicinski <kuba@kernel.org>]

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

[ ... ]