i2c filter driver

[lxuxx]

No description provided.

[lxuxx]

The test is done on ast2600 DC-SCM2.0 board. It requires to release ast2600 which depends on spi monitor functions in #36

[rusty1968]

Files reviewed:

• src/i2cmonitor.rs (424 lines, new file)
• src/tests/functional/i2cmonitor_test.rs (190 lines, new file)
• src/pinctrl.rs (17 lines added)

---

Summary Table

Severity	#	Issue
Soundness	1	No singleton enforcement — double new() aliases hardware
Soundness	2	Stack address given to hardware as DMA pointer; AstI2cFMTbl missing #[repr(C)]
Bug	3	clr_white_list_tbl write-before-read — clearing loop is dead code
Safety	4	i2cf_debug! / i2cf_error! panic on format buffer overflow
Safety	5	ISR ring-buffer subtraction uses non-wrapping arithmetic
Idiom	6	Result<i32, _> always returning Ok(0) — should be Result<(), _>
Idiom	7	Four bool parameters on ast_i2c_filter_en — use a config struct
Idiom	8	get_pclk() takes &mut self and returns a hardcoded constant
Idiom	9	dump_regs uses magic 0..4 instead of 0..AST_I2C_F_COUNT
Idiom	10	Manual zero-fill loop — use .fill(0)
Idiom	11	Opaque bit-shifting for start_index
Idiom	12	Clock selection validated by u32 equality — use an enum
Naming	13	File and struct named "monitor" but the driver implements a "filter"
Test	14	Test bypasses driver abstraction with direct PAC register access
Test	15	Two UartControllers on the same UART peripheral

---

Soundness

1. No singleton enforcement — aliased hardware access

I2cMonitor::new() constructs &'static references to memory-mapped registers with nothing preventing a second call. Two I2cMonitor instances would alias the same hardware without synchronisation, which is undefined behaviour.

The canonical embedded-Rust pattern is to consume the PAC peripheral tokens so the type system enforces exclusive ownership at compile time.

// Current — a second call creates aliased &'static references
pub fn new(logger: L) -> Self {
    let i2cfilter_glb = unsafe { &*I2cfilter::PTR };
    ...
}

// Idiomatic — moves the PAC tokens in, preventing a second instantiation
pub fn new(
    glb: I2cfilter,
    thr0: I2cFilterThr,
    thr1: I2cFilterThr1,
    thr2: I2cFilterThr2,
    thr3: I2cFilterThr3,
    logger: L,
) -> Self { ... }

2. Stack address written to hardware as a DMA/table pointer

// src/i2cmonitor.rs — set_dev_white_list_tbl
let table_ptr = core::ptr::from_ref::<AstI2cFMTbl>(&self.i2cfilter_tbl[index]) as u32;
self.i2cfilter_thrs[index]
    .i2cfilterthr08()
    .write(|w| unsafe { w.addr().bits(table_ptr) });

Two problems:

1. Dangling pointer. I2cMonitor is typically stack-allocated. Once the enclosing scope returns, the hardware holds a pointer to freed stack memory. The table must live in a static or heap allocation that outlives all hardware access.

2. Missing #[repr(C)]. AstI2cFMTbl and AstI2cFBitmap have no #[repr(C)] attribute, so the compiler is free to reorder or add padding to their fields. The hardware expects a specific memory layout; without #[repr(C)] the table contents are unspecified.

// Required on both types
#[repr(C)]
pub struct AstI2cFBitmap { ... }

#[repr(C)]
pub struct AstI2cFMTbl { ... }

---

Bugs

3. clr_white_list_tbl — write-before-read makes the clearing loop dead code

fn clr_white_list_tbl(&mut self, index: usize) {
    // Writes 0 to the register first …
    self.i2cfilter_thrs[index]
        .i2cfilterthr08()
        .write(|w| unsafe { w.addr().bits(0) });

    // … then reads the same register back (always 0 now).
    let tbl_addr: u32 = self.i2cfilter_thrs[index].i2cfilterthr08().read().bits();
    let tbl_ptr = tbl_addr as *mut AstI2cFMTbl;
    unsafe {
        if !tbl_ptr.is_null() { // never true — tbl_addr was just set to 0
            ...
        }
    }
}

The intent is clearly to (1) read the current table address, (2) zero-fill the table it points to, then (3) clear the register. The zero-fill loop never executes because the register was already cleared before the read. The two operations must be swapped.

Additionally, deriving a raw pointer from a value read back from a hardware register and casting it to a Rust reference is unsound in the general case — the hardware address space may not correspond to a valid Rust allocation.

---

Safety

4. i2cf_debug! / i2cf_error! panic on format buffer overflow

macro_rules! i2cf_debug {
    ($logger:expr, $($arg:tt)*) => {
        let mut buf: heapless::String<64> = heapless::String::new();
        write!(buf, $($arg)*).unwrap(); // panics if output > 64 bytes
        $logger.debug(buf.as_str());
    };
}

In a no_std embedded context a panic is typically fatal (hard fault or infinite loop). Several dump_regs log lines include hex register values and surrounding text that can exceed 64 bytes. Either increase the buffer size, or handle the error without panicking (e.g. silently truncate):

let _ = write!(buf, $($arg)*); // truncates silently on overflow

5. Non-wrapping arithmetic in ISR ring-buffer calculation

// src/i2cmonitor.rs — ast_i2c_filter_isr
if info_wp > info_rp {
    count = info_wp - info_rp;
} else {
    count = (info_wp + 0x10) - info_rp; // can overflow if info_wp is near u8::MAX
}

info_wp is a u8. Adding 0x10 to a value near u8::MAX overflows: panics in debug builds, wraps silently in release. Use explicit wrapping arithmetic:

count = info_wp.wrapping_add(0x10).wrapping_sub(info_rp);

---

Idiomaticity

6. Result<i32, &'static str> always returning Ok(0)

ast_i2c_filter_default, ast_i2c_filter_update, and ast_i2c_filter_en all return Ok(0). The i32 carries no information.

// Current
pub fn ast_i2c_filter_default(...) -> Result<i32, &'static str> {
    ...
    Ok(0)
}

// Idiomatic
pub fn ast_i2c_filter_default(...) -> Result<(), &'static str> {
    ...
    Ok(())
}

7. Four bool parameters on ast_i2c_filter_en

#[allow(clippy::fn_params_excessive_bools)]
pub fn ast_i2c_filter_en(
    &mut self,
    filter_en: bool,
    wlist_en: bool,
    clr_idx: bool,
    clr_tbl: bool,
    index: usize,
) -> Result<i32, &'static str>

The #[allow] suppression acknowledges the problem. Four positional booleans make call sites unreadable (ast_i2c_filter_en(true, false, true, false, 2)). Replace with a config struct:

pub struct FilterConfig {
    pub filter_en: bool,
    pub wlist_en: bool,
    pub clr_idx: bool,
    pub clr_tbl: bool,
}

pub fn ast_i2c_filter_en(&mut self, config: FilterConfig, index: usize) -> Result<(), &'static str>

8. get_pclk() takes &mut self and returns a hardcoded constant

pub fn get_pclk(&mut self) -> u32 {
    50_000_000
}

Does not read or modify any state. Should be either:

• A named constant: const PCLK_HZ: u32 = 50_000_000;
• Or at minimum a &self method (no mutation needed)

9. Magic number 0..4 in dump_regs

for i in 0..4 {  // should be 0..AST_I2C_F_COUNT

If AST_I2C_F_COUNT is ever changed, this loop silently diverges. Use the constant.

10. Manual zero-fill loop — use .fill(0)

// Current
for i in 0..AST_I2C_F_REMAP_SIZE {
    self.i2cfilter_data[index].filter_idx[i] = 0;
}

// Idiomatic
self.i2cfilter_data[index].filter_idx.fill(0);

11. Opaque bit-shifting for start_index

let start_index = (idx >> 2) << 2; // rounds idx down to nearest multiple of 4

The intent is clear to an author but reads as arbitrary bit manipulation. More readable alternatives:

let start_index = idx & !0b11;       // bitmask approach
let start_index = (idx / 4) * 4;    // arithmetic approach

12. Clock selection validated by u32 equality — use an enum

pub fn set_initial_timing(&mut self, index: usize, cfg_clock: u32) {
    if cfg_clock == AST_CFG_CLOCK0 || cfg_clock == AST_CFG_CLOCK1 {
        ...
    } else {
        i2cf_error!(self.logger, "i2c filter invalid clock");
        // silently returns — caller has no indication of failure
    }
}

Any u32 can be passed; invalid values are silently ignored (with only a log message, no error return). Use an enum to make invalid states unrepresentable:

pub enum FilterClock {
    Hz100K,
    Hz400K,
}

pub fn set_initial_timing(&mut self, index: usize, clock: FilterClock) { ... }

---

Naming

13. File and struct named "monitor" but the driver implements a "filter"

The file is i2cmonitor.rs, the struct is I2cMonitor, and the init function is ast_i2c_filter_init. The PAC types and all internal names use "filter". The "monitor" name is misleading — a monitor typically implies passive observation, while this driver actively filters (blocks/allows) I2C transactions.

Rename to i2cfilter.rs / I2cFilter for consistency with the rest of the codebase and the hardware documentation.

---

Test Issues

14. Test bypasses driver abstraction with direct PAC register access

test_i2cmonitor_register_write constructs raw PAC peripheral references and writes directly to registers managed by I2cMonitor:

let p0 = unsafe { &*I2cfilter::ptr() };
p0.i2cfilter008().write(|w| unsafe { w.topirqen().bits(0xf) });

let p1 = unsafe { &*I2cFilterThr::PTR };
p1.i2cfilterthr08().write(|w| unsafe { w.bits(0x1234_abcd) });

This creates aliased mutable access to hardware that I2cMonitor considers itself to have exclusive control over. It also defeats the purpose of the driver abstraction. Register-level testing should either be in a dedicated HAL test or the driver should expose a verification method.

15. Two UartControllers on the same UART peripheral

pub fn test_i2cmonitor(uart: &mut UartController<'_>) {
    ...
    let mut dbg_uart = UartController::new(peripherals.uart, &mut delay2);
    let mut i2c_monitor = I2cMonitor::new(UartLogger::new(&mut dbg_uart));

uart (passed in from the test harness) and dbg_uart (created locally from peripherals.uart) are both UartControllers wrapping the same physical UART. Their concurrent writes will interleave unpredictably. The I2cMonitor logger should reuse the test's existing uart handle, or a separate debug UART peripheral should be used.

[linux-foundation-easycla]

• ❌ - login: @lxuxx / name: Linlin Xu . The commit (012816e, 32d797f) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.