Re: [PATCH net v2] net: ethernet: ti: am65-cpsw: fix freeing IRQ in am65_cpsw_nuss_remove_tx_chns()

[Roger Quadros]

On 18/01/2025 02:40, Jacob Keller wrote:
> 
> 
> On 1/16/2025 5:54 AM, Roger Quadros wrote:
>> When getting the IRQ we use k3_udma_glue_tx_get_irq() which returns
>> 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.
>>
> 
> Using the phrase "NULL check" is a bit odd since the value returned
> isn't a pointer. It is correct that checking for 0 is not sufficient
> since it could be a negative error value. Given that IRQ numbers are
> typically considered like an opaque object, perhaps thinking in terms of
> pointers and NULL is common place. Either way, its not worth re-rolling
> for a minor phrasing change like this.

I agree with your view.

> 
> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>

Thanks!

> 
>> There is no issue at probe time but at runtime user can invoke
>> .set_channels which results in the following call chain.
>> am65_cpsw_set_channels()
>>  am65_cpsw_nuss_update_tx_rx_chns()
>>   am65_cpsw_nuss_remove_tx_chns()
>>   am65_cpsw_nuss_init_tx_chns()
>>
>> At this point if am65_cpsw_nuss_init_tx_chns() fails due to
>> k3_udma_glue_tx_get_irq() then tx_chn->irq will be set to a
>> negative value.
>>
>> Then, at subsequent .set_channels with higher channel count we
>> will attempt to free an invalid IRQ in am65_cpsw_nuss_remove_tx_chns()
>> leading to a kernel warning.
>>
>> The issue is present in the original commit that introduced this driver,
>> although there, am65_cpsw_nuss_update_tx_rx_chns() existed as
>> am65_cpsw_nuss_update_tx_chns().
>>
>> Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
>> Signed-off-by: Roger Quadros <rogerq@xxxxxxxxxx>
>> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
>> Reviewed-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
>> ---
>> Changes in v2:
>> - Fixed typo in commit log k3_udma_glue_rx_get_irq->k3_udma_glue_tx_get_irq
>> - Added more details to commit log
>> - Added Reviewed-by tags
>> - Link to v1: https://lore.kernel.org/r/20250114-am65-cpsw-fix-tx-irq-free-v1-1-b2069e6ed185@xxxxxxxxxx
>> ---
>>  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,
> 

-- 
cheers,
-roger