Re: [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned

[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.
---
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2683,6 +2686,8 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
>  {
>  	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
>  
> +	devm_of_platform_depopulate(&pdev->dev);
> +	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
>  	geth_cleanup_freeq(geth);
>  	geth->initialized = false;
>  }
>

[Severity: High]
devm_of_platform_depopulate() unbinds both port devices synchronously,
and gemini_port_remove() ends with clk_disable_unprepare(port->pclk).
By the time we get here both GMAC gate clocks are therefore off. That
includes this writel() to GLOBAL_INTERRUPT_ENABLE_4_REG and the
readw()/writew()/writel() of GLOBAL_SWFQ_RWPTR_REG and
GLOBAL_SW_FREEQ_BASE_SIZE_REG in geth_cleanup_freeq(). All of them
access geth->base while the block is unclocked.

The parent cortina,gemini-ethernet device does not hold a clock of its
own, and the global registers need at least one port clocked. On ARM
that means one of two things. The writes may be silently lost, which
leaves SWFQ_EMPTY_INT_BIT enabled and the old free queue base
programmed. Or the access takes an external abort on unbind or module
unload.

The child path has a similar window. The clock is gated in .remove,
but devres frees the devm threaded IRQ (gemini_port_irq() and
gemini_port_irq_thread()) only after .remove returns.
SWFQ_EMPTY_INT_BIT is still enabled at that point. The thread can
therefore still run geth_fill_freeq() and rewrite
GLOBAL_INTERRUPT_ENABLE_4_REG on an unclocked block.

Could the free queue interrupt be masked, and the queue torn down,
while at least one port clock is still running? Another option is for
the parent to hold its own enable on a port clock across remove. Which
ordering did you have in mind, given that port 1 owns the refill IRQ?
-- 
pw-bot: cr