netdev - Re: [PATCH net-next v3 1/6] net: ethernet: ti: am65-cpsw: Introduce multi queue Rx

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: <3ae005d8-b2e3-4a71-af0b-d57f00727b30@kernel.org>
Date: Mon, 8 Jul 2024 22:42:08 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Siddharth Vadapalli <s-vadapalli@...com>, Julien Panis
 <jpanis@...libre.com>, Simon Horman <horms@...nel.org>,
 Andrew Lunn <andrew@...n.ch>, srk@...com, vigneshr@...com,
 danishanwar@...com, pekka Varis <p-varis@...com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/6] net: ethernet: ti: am65-cpsw: Introduce
 multi queue Rx

On 06/07/2024 04:15, Jakub Kicinski wrote:
> On Wed, 03 Jul 2024 16:51:32 +0300 Roger Quadros wrote:
>>  
>> -	if (queue >= AM65_CPSW_MAX_TX_QUEUES)
>> +	if (queue >= AM65_CPSW_MAX_TX_QUEUES &&
>> +	    queue >= AM65_CPSW_MAX_RX_QUEUES)
>>  		return -EINVAL;
> 
> both MAXes are 8, the else conditions below are dead code
> Same for set

yes. Maybe I should just use one define for both? e.g. AM65_CPSW_MAX_QUEUES.

> 
>> -	tx_chn = &common->tx_chns[queue];
>> +	if (queue < AM65_CPSW_MAX_TX_QUEUES) {
>> +		tx_chn = &common->tx_chns[queue];
>> +		coal->tx_coalesce_usecs = tx_chn->tx_pace_timeout / 1000;
>> +	} else {
>> +		coal->tx_coalesce_usecs = ~0;
>> +	}
>>  
>> -	coal->tx_coalesce_usecs = tx_chn->tx_pace_timeout / 1000;
>> +	if (queue < AM65_CPSW_MAX_RX_QUEUES) {
>> +		rx_flow = &common->rx_chns.flows[queue];
>> +		coal->rx_coalesce_usecs = rx_flow->rx_pace_timeout / 1000;
>> +	} else {
>> +		coal->rx_coalesce_usecs = ~0;
>> +	}
> 
> +	for (flow_idx = 0; flow_idx < common->rx_ch_num_flows; flow_idx++) {
> +		flow = &rx_chn->flows[flow_idx];
> +		for (i = 0; i < AM65_CPSW_MAX_RX_DESC; i++) {
> +			page = page_pool_dev_alloc_pages(flow->page_pool);
> +			if (!page) {
> +				dev_err(common->dev, "cannot allocate page in flow %d\n",
> +					flow_idx);
> +				ret = -ENOMEM;
> +				if (i)
> +					goto fail_rx;
>  
> -			return ret;
> -		}
> -		rx_chn->pages[i] = page;
> +				return ret;
> 
> the direct returns now that it's a double-nested loop seem questionable,
> don't you have to goto fail_rx?

Good catch. I should just drop the "if (i)" and goto fail_rx regardless.

-- 
cheers,
-roger