Attention is currently required from: Maciej Pijanowski, Michał Kopeć, Michał Żygowski.

View Change

14 comments:

• File src/ec/dasharo/ec/acpi/dasharo.asl:

  • Patch Set #10, Line 3:

    // Notifications:
    //   0x80 - hardware backlight toggle
    //   0x81 - backlight toggle
    //   0x82 - backlight down
    //   0x83 - backlight up
    //   0x84 - backlight color change
    //   0x85 - OLED screen toggle
    

    These have been removed from the code, should be removed from here as well.

  • Patch Set #10, Line 11: Name (_HID, "XXXX0000") // TODO: Update after we get a valid ACPI HID prefix

    Don't we have one already? https://uefi.org/ACPI_ID_List?acpi_search=DSHR

  • Patch Set #10, Line 17: Debug = "DASH: RSET"

    Here `Debug` is directly written to, in other cases `Printf` is used, even for simple strings. Please be consistent.

• File src/ec/dasharo/ec/acpi/ec.asl:

  • Patch Set #10, Line 95: ^^^^DASH.RSET()

    Won't it unconditionally turn off airplane mode LED on wake?

• File src/ec/dasharo/ec/dasharo_ec.c:

  • Patch Set #10, Line 17: #define DASHARO_EC_BASE 0x0E00

    Is this reserved from resource allocator?

  • Patch Set #10, Line 43: 2

    REG_DATA

  • Patch Set #10, Line 52: REG_CMD + 2

    REG_DATA

  • Patch Set #10, Line 88: index should be valid due to length test above

    I don't see such test. In fact, I believe that this code would overwrite caller's stack, if the data from EC is long enough.

    Also, the caller expects to get 0-terminated string, but this code doesn't enforce it, which may cause another overflow.

  • Patch Set #10, Line 122: // Read data bytes, index should be valid due to length test above

    Same as `dasharo_ec_read_version`.

  • Patch Set #10, Line 200: #define SPI_ACCESS_SIZE 252

    IIUC this is `(DASHARO_EC_SIZE - REG_DATA - ARRAY_SIZE(read_cmd))` (the last operand defined below, it would be `ARRAY_SIZE(write_cmd)` for writes), perhaps it would be better to write it this way.

  • Patch Set #10, Line 292: if ((rv = ec_spi_reset()))

    I don't know how this EC works, but I assume that this reset doesn't clear SPI status, right?

  • Patch Set #10, Line 323: return 0;

    This and next function return 0 also when `status < 0`, is this the expected behavior?

  • Patch Set #10, Line 553:

    dasharo_ec_read_board((uint8_t *)cur_board_str);
    	dasharo_ec_read_version((uint8_t *)cur_version_str);
    

    Return values are not checked, which makes `cur_*_str` potentially uninitialized.

  • Patch Set #10, Line 570: if ((ec_read(0x10) & 0x01) != 0x01) {

    Magic numbers, please at least add a comment about their meaning.

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