Re: [PULL v2 3/3] i82596: Implement enhanced TX/RX with packet queuing and filtering

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 4 Nov 2025 at 15:22, <deller@kernel.org> wrote:
>
> From: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
>
> In this patch I have added the following:
> - Rewrote transmit path with CSMA/CD collision handling and retry logic
> - Implemented flexible TX buffer descriptor (TBD) chain processing
> - Rewrote receive path with packet filtering and monitor mode support
> - Added RX packet queue for handling resource exhaustion
> - Implemented queue flush timer and management
> - Added RX state machine with proper state transitions
> - Implemented packet filtering (unicast, broadcast, multicast, promiscuous)
> - Added SCB RU_START enhancement to find usable RFDs
> - Implemented dump command support
> - Added bus throttle timer loading (LOAD_THROTTLE/LOAD_START commands)
> - Enhanced signal_ca with proper initialization sequence
> - Finally, adding self-test functionality
>
> Note:
> With this patch, and the previous ones in the patch series, we are able
> to achive proper 82596 NIC emulation.

Hi; Coverity notices a logic error in this function
(CID 1642873):

> +static ssize_t i82596_receive_packet(I82596State *s, const uint8_t *buf,
> +                                      size_t size, bool from_queue)
> +{

In this loop, I cut out the insides of some of the if() blocks
to make the structure clearer:

> +    do {
> +        if (simplified_mode && I596_LOOPBACK) {
              [...]
>          } else {
> -            s->scb_status &= ~SCB_STATUS_CX;
               [...]
> +            if (bytes_copied < payload_size) {
                   [...]
> +                }
> +            }
>          }
> -        update_scb_status(s);
> +        break;

This "break" is at the top level inside the do {} loop,
so it unconditionally exits it.

>
> -        /* Interrupt after doing cmd? */
> -        if (cmd & CMD_INTR) {
> -            s->send_irq = 1;
> +    } while (bytes_copied < payload_size);

So having a while() condition here is dead code, because
execution always stops at the "break" on the first
iteration, and doesn't reach the condition check.

Should the 'break' have been inside an if() somewhere?

thanks
-- PMM