[PATCH] net: gemini: Free the shared queue after removing the ports

[PATCH] net: gemini: Free the shared queue after removing the ports

[Myeonghun Pak]

The software free queue is shared by both Ethernet ports, but each port
remove callback frees it. The parent also frees the queue before its
managed child devices are removed. This can free the queue while a port
or its threaded free-queue IRQ handler still uses it, and free the same
allocation again when the children are removed.

Keep the queue owned by the parent. Explicitly depopulate its child
devices before freeing it, so both ports are unregistered and their
managed IRQ handlers have been released. Disable the free-queue
interrupt after those handlers have drained, since the threaded handler
can re-enable it. Only clean up the queue if it was allocated; it may
never have been set up if neither interface was opened.

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/net/ethernet/cortina/gemini.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 4c762229ce42..e493d495f2ef 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2364,7 +2364,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
 		unregister_netdev(port->netdev);
 	}
 	clk_disable_unprepare(port->pclk);
-	geth_cleanup_freeq(port->geth);
 }
 
 static void gemini_ethernet_init(struct gemini_ethernet *geth)
@@ -2656,7 +2655,10 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
 {
 	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
 
-	geth_cleanup_freeq(geth);
+	devm_of_platform_depopulate(&pdev->dev);
+	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
+	if (geth->freeq_ring)
+		geth_cleanup_freeq(geth);
 	geth->initialized = false;
 }
 
-- 
2.47.1

Re: [PATCH] net: gemini: Free the shared queue after removing the ports

[Andrew Lunn]

> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  drivers/net/ethernet/cortina/gemini.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 4c762229ce42..e493d495f2ef 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2364,7 +2364,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  		unregister_netdev(port->netdev);
>  	}
>  	clk_disable_unprepare(port->pclk);
> -	geth_cleanup_freeq(port->geth);
>  }
>  
>  static void gemini_ethernet_init(struct gemini_ethernet *geth)
> @@ -2656,7 +2655,10 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
>  {
>  	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
>  
> -	geth_cleanup_freeq(geth);
> +	devm_of_platform_depopulate(&pdev->dev);

This seems wrong. If you have to call devm_of_platform_depopulate()
why are you using devm?

> +	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> +	if (geth->freeq_ring)
> +		geth_cleanup_freeq(geth);

gemini_ethernet_remove() is supposed to do the opposite of
gemini_ethernet_probe(). What is the opposite of geth_cleanup_freeq()?
Where is it called in gemini_ethernet_probe()?

	Andrew