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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

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

No, the global KSZ8463_REG_ISR register works the same way than the 
other switches, it's a read-only aggregation of other interrupt status 
registers. There is no need to acknowledge the global bit.


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

So far only bit 4 is used but others could be used later.

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

ptp_clock_register() not being called is intended at that stage. It 
prevents from exposing PTP clocks to userspace until the proper support 
is completely there. Because the PTP clocks aren't exposed, the PTP 
interrupt won't get enabled so the issue described above won't happen.

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

This is true, I didn't pay attention to that because I don't have any 
MDIO node in my device-tree. KSZ8463 doesn't support PHY irqs so it 
feels like a good thing to return -EINVAL here.

There is a real issue on platforms that define a irq_default_domain 
though. I can fix this by creating ksz8463_mdio_register(), any thoughts 
about this ?

>>   
>>   	/* 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?
> 

.ts_en flags need to be handled differently on the KSZ8463 anyway, this 
is done on PATCH 10.
>> +	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?
> 

The PTP clock isn't registered so far so this won't happen. Proper PTP 
support is added on PATCH 10.

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

It won't be an issue until PTP clock is registered, it's properly 
handled in PATCH 10 with the last_tx_is_pdelayresp flag>> +
>> +		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?
> That would mean putting in ksz_device an attribute that only the KSZ8463 
uses. I would prefer not to.


Best regards,
Bastien