zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.58k stars 6.48k forks source link

driver: i2c_dw: i2c target callbacks do not check returned values #78487

Open robcont opened 1 week ago

robcont commented 1 week ago

Describe the bug

The I2C DW target driver does not properly check 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 invoked without proper error handling of their return values, which may result in unexpected behavior.

To Reproduce

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.

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):

Additional context

Here's the problematic code:

#ifdef CONFIG_I2C_TARGET
        const struct i2c_target_callbacks *slave_cb = dw->slave_cfg->callbacks;
        uint32_t slave_activity = test_bit_status_activity(reg_base);
        uint8_t data;

        i2c_dw_slave_read_clear_intr_bits(port);

        if (intr_stat.bits.rx_full) {
            if (dw->state != I2C_DW_CMD_SEND) {
                dw->state = I2C_DW_CMD_SEND;
                if (slave_cb->write_requested) {
                    slave_cb->write_requested(dw->slave_cfg); // ISSUE: Return value is ignored
                }
            }
            data = i2c_dw_read_byte_non_blocking(port);
            if (slave_cb->write_received) {
                slave_cb->write_received(dw->slave_cfg, data); // ISSUE: Return value is ignored
            }
        }

        if (intr_stat.bits.rd_req) {
            if (slave_activity) {
                read_clr_rd_req(reg_base);
                dw->state = I2C_DW_CMD_RECV;
                if (slave_cb->read_requested) {
                    slave_cb->read_requested(dw->slave_cfg, &data); // ISSUE: Return value is ignored
                    i2c_dw_write_byte_non_blocking(port, data);
                }
                if (slave_cb->read_processed) {
                    slave_cb->read_processed(dw->slave_cfg, &data); // ISSUE: Return value is ignored
                }
            }
        }
#endif
github-actions[bot] commented 1 week ago

Hi @robcont! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

teburd commented 1 week ago

Thank you for filing, I'll take a look.

Pull requests are also very welcome if you have a fix in mind