Re: [PATCH net-next v3 2/4] net: ethernet: oa_tc6: deliver the PHY interrupt to phylib

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new irq_chip (drivers/net/ethernet/oa_tc6.c:596-602)
  provides only .irq_mask/.irq_unmask (shadow flag flushed from…
- [Medium] In oa_tc6_process_extended_status()
  (drivers/net/ethernet/oa_tc6.c:953-955) the driver only calls…
- [Low] Concern that OA_TC6_PHY_INT and all code it gates is dead in-tree
  because no oa_tc6 consumer sets the flag.
- [Low] Inconsistent quirk gating: oa_tc6_process_extended_status()
  (oa_tc6.c:953) schedules tc6->phy_irq_work based only on…
- [Low] Concern that oa_tc6_phy_irq_bus_sync_unlock() (oa_tc6.c:571-594)
  only logs a failed INT_MASK0 read or write, leaving the hardware mask…
- [Low] oa_tc6_phy_irq_bus_sync_unlock()
  (drivers/net/ethernet/oa_tc6.c:571-594) performs a read-modify-write…

--

net: ethernet: oa_tc6: deliver the PHY interrupt to phylib

This adds a nested virtual IRQ plus a custom irq_chip to oa_tc6 so an
in-band MAC-PHY PHY interrupt can be delivered to phylib, gated by a new
OA_TC6_PHY_INT quirk flag.  Dispatch of the nested IRQ is deferred to a
work item, and the mask bit is pushed to INT_MASK0 from
irq_bus_sync_unlock().

A few questions below about the mask handling and the dispatch gating.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 6fcc5f561d56..b4a95155e924 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -528,6 +535,114 @@ int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
>  }
>  EXPORT_SYMBOL_GPL(oa_tc6_mdiobus_write_c45);
>  
> +static void oa_tc6_phy_irq_work(struct work_struct *work)
> +{
> +	struct oa_tc6 *tc6 = container_of(work, struct oa_tc6, phy_irq_work);
> +
> +	/* Dispatched off the SPI chunk-processing thread so that
> +	 * phy_interrupt() taking phydev->lock and issuing synchronous SPI
> +	 * control transfers from PHY handle_interrupt() cannot stall the single
> +	 * thread pumping TX/RX data chunks.
> +	 */
> +	handle_nested_irq(tc6->phy_virq);
> +}
> +

[ ... ]

> +static void oa_tc6_phy_irq_bus_sync_unlock(struct irq_data *irqd)
> +{
> +	struct oa_tc6 *tc6 = irq_data_get_irq_chip_data(irqd);
> +	u32 regval;
> +	int ret;
> +
> +	ret = oa_tc6_read_register(tc6, OA_TC6_REG_INT_MASK0, &regval);
> +	if (ret) {
> +		dev_err(&tc6->spi->dev, "Failed to read INT_MASK0: %d\n", ret);
> +		goto unlock;
> +	}
> +
> +	if (tc6->phy_irq_masked)
> +		regval |= OA_TC6_INT_MASK0_PHY_INT_MASK;
> +	else
> +		regval &= ~OA_TC6_INT_MASK0_PHY_INT_MASK;
> +
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
> +	if (ret)
> +		dev_err(&tc6->spi->dev, "Failed to write INT_MASK0: %d\n", ret);
> +
> +unlock:
> +	mutex_unlock(&tc6->phy_irq_lock);
> +}

[Severity: Low]
This read-modify-write of INT_MASK0 is serialised by the new
tc6->phy_irq_lock, but is that lock taken by the other writer of the same
register?

oa_tc6_disable_traffic() writes INT_MASK0 from the threaded IRQ context
without phy_irq_lock:

	oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);

tc6->spi_ctrl_lock only makes each individual control transfer atomic, not
the read/modify/write pair here, so can this sequence lose an update?

CPU0 (oa_tc6_macphy_threaded_irq -> oa_tc6_disable_traffic)
	writes INT_MASK0 = OA_TC6_INT_MASK0_ALL_INTERRUPTS

CPU1 (phylib mask/unmask -> oa_tc6_phy_irq_bus_sync_unlock)
	read INT_MASK0   (stale copy, taken before CPU0's write)
	write INT_MASK0  (reverts the error-interrupt masking)

The reverse order would clobber the PHY_INT mask bit phylib just asked
for.  The practical effect looks small since
oa_tc6_macphy_threaded_irq() calls disable_irq_nosync(tc6->spi->irq)
before oa_tc6_disable_traffic(), but should all INT_MASK0 modifiers share
one lock or a lock-protected shadow value?

[Severity: Low]
This isn't a bug, but both error paths here only log and return, so
tc6->phy_irq_masked (and phylib's view of the line) can stay permanently
out of sync with the hardware mask bit, with no retry.  The same
log-and-continue shape is used by regmap_irq_sync_unlock() in
drivers/base/regmap/regmap-irq.c, since the genirq callback returns void
and has nowhere to propagate an error, and a failing oa_tc6 control
transfer already means the SPI link to the MAC-PHY is broken.  Is it worth
noting in a comment that a failed sync leaves the mask bit undefined?

> +
> +static struct irq_chip oa_tc6_phy_irq_chip = {
> +	.name                   = "oa_tc6_phy",
> +	.irq_mask               = oa_tc6_phy_irq_mask,
> +	.irq_unmask             = oa_tc6_phy_irq_unmask,
> +	.irq_bus_lock           = oa_tc6_phy_irq_bus_lock,
> +	.irq_bus_sync_unlock    = oa_tc6_phy_irq_bus_sync_unlock,
> +};
> +
> +static int oa_tc6_phy_irq_map(struct irq_domain *domain, unsigned int irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_data(irq, domain->host_data);
> +	irq_set_chip_and_handler(irq, &oa_tc6_phy_irq_chip, handle_simple_irq);
> +	irq_set_nested_thread(irq, true);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}

[Severity: High]
The commit message says:

    "The interrupt starts masked (hardware reset default) and is only
    unmasked when phylib requests it, so disabling the nested IRQ actually
    masks the hardware source too, preventing interrupt storms."

Is that true for this chip?  There is no .irq_disable callback and
oa_tc6_phy_irq_map() does not set IRQ_DISABLE_UNLAZY, so
disable_irq()/disable_irq_nosync() takes the lazy path in kernel/irq/chip.c:

	irq_state_set_disabled(desc);
	if (desc->irq_data.chip->irq_disable) {
		desc->irq_data.chip->irq_disable(&desc->irq_data);
		irq_state_set_masked(desc);
	} else if (mask) {
		mask_irq(desc);
	}

With no .irq_disable and mask == false, OA_TC6_INT_MASK0_PHY_INT_MASK is
never written, so the in-band source stays armed.

The usual lazy-disable safety net, where the flow handler masks the line on
the next occurrence, does not apply to nested IRQs.  handle_nested_irq()
goes through irq_can_handle_actions():

	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
		desc->istate |= IRQS_PENDING;
		return false;
	}

so phy_interrupt() is never called and nothing masks the line.

Can this then be reached from phylib itself?  phy_interrupt() in
drivers/net/phy/phy.c does:

		phydev->irq_rerun = 1;
		disable_irq_nosync(irq);
		return IRQ_HANDLED;

i.e. the virq is disabled from inside the nested handler without the PHY
status being read or acked, and it is only re-enabled later in
mdio_bus_phy_resume():

		if (phydev->irq_rerun) {
			phydev->irq_rerun = 0;
			enable_irq(phydev->irq);

In that window PHYINT is unmasked in INT_MASK0 and unacknowledged, so the
MAC-PHY keeps IRQn asserted and oa_tc6_macphy_isr() ->
oa_tc6_macphy_threaded_irq() -> oa_tc6_process_extended_status() keeps
looping with an empty-chunk transfer plus a STATUS0 read/write and a no-op
schedule_work() each round, with nothing able to clear the condition.

Would adding an .irq_disable callback, or
irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY) in oa_tc6_phy_irq_map(),
make a disabled virq really mask the in-band source as the commit message
describes?

[ ... ]

> @@ -810,6 +943,17 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
>  		return ret;
>  	}
>  
> +	/* Dispatch the PHY interrupt to phylib via the nested virtual IRQ so
> +	 * the PHY driver reads and acknowledges its status. This is deferred
> +	 * to a workqueue rather than dispatched synchronously here, since
> +	 * phy_interrupt() takes phydev->lock and PHY handle_interrupt() issues
> +	 * synchronous SPI control transfers, which would otherwise block this
> +	 * thread.
> +	 */
> +	if ((tc6->quirk_flags & OA_TC6_PHY_INT) &&
> +	    FIELD_GET(OA_TC6_STATUS0_PHY_INT, value))
> +		schedule_work(&tc6->phy_irq_work);
> +

[Severity: Medium]
Should OA_TC6_INT_MASK0_PHY_INT_MASK be set here for the duration of the
deferral?

As the commit message notes, PHYINT "stays asserted until acked", and the
ack only happens later in oa_tc6_phy_irq_work() -> handle_nested_irq() ->
phy_interrupt() -> drv->handle_interrupt().  Until the worker runs, STATUS0
keeps bit 7 set, so the existing per-chunk short-circuit earlier in this
function:

	if (!value)
		return 0;

never fires, and every RX chunk footer carrying EXT_STS pays an extra
STATUS0 read plus STATUS0 write control transfer on the single SPI
chunk-processing thread.

Also, the MAC-PHY IRQ is requested with IRQF_TRIGGER_LOW | IRQF_ONESHOT in
oa_tc6_init(), so does the still-asserted line re-trigger the hard ISR as
soon as the threaded handler returns, adding another empty-chunk transfer
per round until the work item is scheduled?  The irq thread runs at
realtime priority while the ack worker runs on system_percpu_wq at normal
priority, so that window is under the worker's scheduling latency.  The
mask bit and the shadow/bus_lock machinery added by this patch already give
a way to quiesce the source while the ack is deferred.

[Severity: Low]
This dispatch is gated on OA_TC6_PHY_INT alone, while the only initializer
of tc6->phy_irq_work and tc6->phy_virq, oa_tc6_phy_irq_setup() called from
oa_tc6_mdiobus_register(), is skipped when OA_TC6_BROKEN_PHY is set:

oa_tc6_phy_init()
	if (tc6->quirk_flags & OA_TC6_BROKEN_PHY)
		return 0;

For a consumer passing OA_TC6_BROKEN_PHY | OA_TC6_PHY_INT, would this
schedule_work() then run on a devm_kzalloc()-zeroed work_struct that never
saw INIT_WORK(), and would the worker call handle_nested_irq(0)?  No
in-tree caller combines the two flags today, so this is not reachable as
things stand, but the flags are independent bits in an exported enum in
include/linux/oa_tc6.h.  Would gating the dispatch on the resource actually
created, for example a non-zero tc6->phy_virq, make the lifecycle
self-consistent?

>  	if (FIELD_GET(OA_TC6_STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
>  		oa_tc6_look_for_new_frame(tc6);
>  		net_err_ratelimited("%s: Receive buffer overflow error\n",

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918143302.76909-1-parthiban.veerasooran%40microchip.com