On Tue, 4 Nov 2025 at 15:22, <[email protected]> wrote:
>
> From: Soumyajyotii Ssarkar <[email protected]>
>
> 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 spotted an issue in this code (CID 1642868):
> +static ssize_t i82596_receive_packet(I82596State *s, const uint8_t *buf,
> + size_t size, bool from_queue)
> +{
> + struct i82596_rx_descriptor rfd;
> + uint32_t rfd_addr, rbd_addr;
> + uint16_t rx_status = 0;
> + uint16_t is_broadcast = 0;
> + bool packet_completed = true;
> + bool simplified_mode = false;
> + size_t frame_size = size;
> + size_t payload_size = 0;
> + size_t bytes_copied = 0;
> + const uint8_t *packet_data = buf;
> + bool crc_valid = true;
Here we set crc_valid to true...
> + bool out_of_resources = false;
> + size_t crc_size = i82596_get_crc_size(s);
> +
> + trace_i82596_receive_packet(buf, size);
[snipped]
> +rx_complete:
> + if (I596_CRCINM && !I596_LOOPBACK && packet_completed) {
> + uint8_t crc_data[4];
> + size_t crc_len = crc_size;
> +
> + if (I596_CRC16_32) {
> + uint32_t crc = crc32(~0, packet_data, frame_size);
> + crc = cpu_to_be32(crc);
> + memcpy(crc_data, &crc, 4);
> + } else {
> + uint16_t crc = i82596_calculate_crc16(packet_data, frame_size);
> + crc = cpu_to_be16(crc);
> + memcpy(crc_data, &crc, 2);
> }
>
> - if (s->cu_status != CU_ACTIVE) {
> - break;
> + if (simplified_mode) {
> + address_space_write(&address_space_memory,
> + rfd_addr + 0x1E + bytes_copied,
> + MEMTXATTRS_UNSPECIFIED, crc_data, crc_len);
> + }
> + }
...but nowhere in any of this code do we ever set crc_valid to
anything else...
> +
> + if (packet_completed && crc_valid) {
...so this condition is the same as "if (packet_completed)"...
> + rx_status |= STAT_C | STAT_OK;
> + if (is_broadcast) {
> + rx_status |= 0x0001;
> + }
> + } else if (packet_completed) {
...and the code in this block is unreachable.
> + rx_status |= STAT_C;
> + if (!crc_valid) {
> + rx_status |= RX_CRC_ERRORS;
> + }
> + } else {
> + rx_status |= STAT_B;
> + }
> +
What was the intention here?
thanks
-- PMM
