Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine.

View Change

8 comments:

• File erasure_layout.c:

  • Patch Set #5, Line 240:

    May I ask you to also add doc comment here , most importantly to say that region_end is inclusive

  • Patch Set #5, Line 320: * @param region_end end address of the region

    I know it wasn't here before, but let's document it here that *end is inclusive*. those 1-offsets are tricky, but also it will be useful for future readers

    Also, maybe you can check with your own eyes that region end is inclusive? I checked (by the callsites of erase_write) but just in case!

  • Patch Set #5, Line 335: region_end - region_start

    If region end is inclusive, this needs to be +1 byte... am I right? (because the 3rd arg is length)
    It seems we can add one more test case for this, a test case with protected region of 1 last byte

  • Patch Set #5, Line 369: len = min(region_end, region.end - 1) - addr + 1;

    Maybe add a comment above this line that
    region_end is inclusive, but region.end is exclusive

• File tests/erase_func_algo.c:

  • Patch Set #5, Line 1042: _tagged

    Maybe without `_tagged` ? the rest of functions have suffix, so there will be no clash

  • Patch Set #5, Line 1082: #define ERASE_PROTECTED(i) static int block_erase_chip_with_protected_region_ ## i \

    That's what I was thinking for the previous patch, a macro loop! :)

  • Patch Set #5, Line 1092: void erase_unwritable_regions_skipflag_on_test_success(void **state)

    I think this qualifies for a comment:

    This test that runs through the test cases with protected regions on the chip, and with a flag set to skip (i.e. ignore) protected regions. Ignoring means they are left as is, and no opcodes should be sent to any memory blocks inside protected regions.

    feel free to improve if you want!

  • Patch Set #5, Line 1172:

    void erase_unwritable_regions_skipflag_off_test_success(void **state)
    {
    //	flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, false);
    }
    

    I meant to write this test too, but didn't finish it.
    Probably better to remove from this patch, and I will add it later

To view, visit change 82393. To unsubscribe, or for help writing mail filters, visit settings.