linux-kernel - Re: [PATCH net] net: ethernet: ti: am65-cpsw: fix freeing IRQ in am65_cpsw_nuss_remove_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <gygqjjyso3p4qgam4fpjdkqidj2lhxldkmaopqg32bw3g4ktpj@43tmtsdexkqv>
Date: Wed, 15 Jan 2025 10:48:28 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Roger Quadros <rogerq@...nel.org>
CC: Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Grygorii Strashko
	<grygorii.strashko@...com>,
        Siddharth Vadapalli <s-vadapalli@...com>, <srk@...com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: ethernet: ti: am65-cpsw: fix freeing IRQ in
 am65_cpsw_nuss_remove_tx_chns()

On Tue, Jan 14, 2025 at 06:44:02PM +0200, Roger Quadros wrote:

Hello Roger,

> When getting the IRQ we use k3_udma_glue_rx_get_irq() which returns

You probably meant "k3_udma_glue_tx_get_irq()" instead? It is used to
assign tx_chn->irq within am65_cpsw_nuss_init_tx_chns() as follows:

		tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);

Additionally, following the above section we have:

		if (tx_chn->irq < 0) {
			dev_err(dev, "Failed to get tx dma irq %d\n",
				tx_chn->irq);
			ret = tx_chn->irq;
			goto err;
		}

Could you please provide details on the code-path which will lead to a
negative "tx_chn->irq" within "am65_cpsw_nuss_remove_tx_chns()"?

There seem to be two callers of am65_cpsw_nuss_remove_tx_chns(), namely:
1. am65_cpsw_nuss_update_tx_rx_chns()
2. am65_cpsw_nuss_suspend()
Since both of them seem to invoke am65_cpsw_nuss_remove_tx_chns() only
in the case where am65_cpsw_nuss_init_tx_chns() *did not* error out, it
appears to me that "tx_chn->irq" will never be negative within
am65_cpsw_nuss_remove_tx_chns()

Please let me know if I have overlooked something.

Regards,
Siddharth.

> negative error value on error. So not NULL check is not sufficient
> to deteremine if IRQ is valid. Check that IRQ is greater then zero
> to ensure it is valid.
> 
> Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
> Signed-off-by: Roger Quadros <rogerq@...nel.org>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 5465bf872734..e1de45fb18ae 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2248,7 +2248,7 @@ static void am65_cpsw_nuss_remove_tx_chns(struct am65_cpsw_common *common)
>  	for (i = 0; i < common->tx_ch_num; i++) {
>  		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
>  
> -		if (tx_chn->irq)
> +		if (tx_chn->irq > 0)
>  			devm_free_irq(dev, tx_chn->irq, tx_chn);
>  
>  		netif_napi_del(&tx_chn->napi_tx);
> 
> ---
> base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
> change-id: 20250114-am65-cpsw-fix-tx-irq-free-846ac55ee6e1
> 
> Best regards,
> -- 
> Roger Quadros <rogerq@...nel.org>
>