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
