[<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
Powered by blists - more mailing lists