driver: stm32-i2c-v2: i2c_target_read_processed_cb_t return value not respected

[ddavidebor]

Describe the bug
The I2C target v2 (slave) driver for STM32 platforms is not properly checking the return values of callback functions as defined in the I2C target API. Specifically, the functions i2c_target_write_requested_cb_t, i2c_target_write_received_cb_t, i2c_target_read_requested_cb_t, and i2c_target_read_processed_cb_t are being called without proper error handling of their return values. This can lead to unexpected behavior.

To Reproduce
This issue is present in the STM32 I2C target driver implementation. The problem can be observed in the source code.

Expected behavior
The driver should check the return values of all callback functions and respond appropriately, such as NACKing or stopping the transfer when an error is returned.

Impact
This issue could lead to silent failures in I2C communication, making it difficult to debug I2C-related problems. It may cause data corruption or communication deadlocks in systems relying on proper I2C target behavior. It may also cause discrepancies when moving from SoC to another

Logs and console output
N/A for this issue, as it's a code-level problem rather than a runtime error.

Environment (please complete the following information):

• OS: Various (issue is in the Zephyr RTOS code)
• Toolchain: Various
• Commit SHA or Version used: Latest main branch as of reporting date

Additional context
Here's the problematic code:

slave_cb = slave_cfg->callbacks;

if (LL_I2C_IsActiveFlag_TXIS(i2c)) {
    uint8_t val;
    slave_cb->read_processed(slave_cfg, &val);  // ISSUE: Return value is ignored
    LL_I2C_TransmitData8(i2c, val);
    return;
}

if (LL_I2C_IsActiveFlag_RXNE(i2c)) {
    uint8_t val = LL_I2C_ReceiveData8(i2c);
    if (slave_cb->write_received(slave_cfg, val)) {  
        LL_I2C_AcknowledgeNextData(i2c, LL_I2C_NACK);
    }
    return;
}

if (LL_I2C_IsActiveFlag_NACK(i2c)) {
    LL_I2C_ClearFlag_NACK(i2c);
}

if (LL_I2C_IsActiveFlag_STOP(i2c)) {
    stm32_i2c_disable_transfer_interrupts(dev);
    /* Flush remaining TX byte before clearing Stop Flag */
    LL_I2C_ClearFlag_TXE(i2c);
    LL_I2C_ClearFlag_STOP(i2c);
    slave_cb->stop(slave_cfg); 
    /* Prepare to ACK next transmissions address byte */
    LL_I2C_AcknowledgeNextData(i2c, LL_I2C_ACK);
}

if (LL_I2C_IsActiveFlag_ADDR(i2c)) {
    uint32_t dir;
    LL_I2C_ClearFlag_ADDR(i2c);
    dir = LL_I2C_GetTransferDirection(i2c);
    if (dir == LL_I2C_DIRECTION_WRITE) {
        slave_cb->write_requested(slave_cfg);  // ISSUE: Return value is ignored
        LL_I2C_EnableIT_RX(i2c);
    } else {
        uint8_t val;
        slave_cb->read_requested(slave_cfg, &val);  // ISSUE: Return value is ignored
        LL_I2C_TransmitData8(i2c, val);
        LL_I2C_EnableIT_TX(i2c);
    }
    stm32_i2c_enable_transfer_interrupts(dev);
}

[erwango]

@ddavidebor Thanks for raising this issue. Don't hesitate to submit a pull request to fix this behavior if you feel able to do so.

[FRASTM]

It add this PR #80994 about what you stated,
@ddavidebor Is it what you had in mind to fix the pb ?

[ddavidebor]

@FRASTM yup thank you ! Much appreciated!