From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: add support for lontium lt8713sx bridge driver Date: Wed, 04 Mar 2026 06:57:55 +1000 Message-ID: In-Reply-To: <20260303-lt8713sx-bridge-driver-v5-2-6cc2a855aafa@oss.qualcomm.com> References: <20260303-lt8713sx-bridge-driver-v5-0-6cc2a855aafa@oss.qualcomm.com> <20260303-lt8713sx-bridge-driver-v5-2-6cc2a855aafa@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Issues (in descending severity):** #### 1. Bug: Redundant assignment of `bridge.funcs` (line 555) ```c lt8713sx =3D devm_drm_bridge_alloc(dev, struct lt8713sx, bridge, <8713sx_= bridge_funcs); ... lt8713sx->bridge.funcs =3D <8713sx_bridge_funcs; ``` `devm_drm_bridge_alloc()` already sets `bridge->funcs =3D funcs` (confirmed= in `drm_bridge.c:349`). The manual assignment at line 555 is redundant. Wh= ile not harmful, it suggests a misunderstanding of the API and should be re= moved to match convention =E2=80=94 no other in-tree user of `devm_drm_brid= ge_alloc()` does this. #### 2. Firmware data corruption when `fw->size < SZ_64K` (line 128) ```c memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1); ``` This unconditionally copies `SZ_64K - 1` bytes from firmware data, but the = firmware size is only validated as `<=3D SZ_256K - 1`. If the firmware is s= maller than 64K (which is explicitly handled in `lt8713sx_firmware_upgrade`= with `if (lt8713sx->fw->size < SZ_64K)`), this `memcpy` reads past the end= of `lt8713sx->fw->data`, causing an out-of-bounds read. The copy size shou= ld be `min(lt8713sx->fw->size, (size_t)(SZ_64K - 1))`. Similarly, when `fw->size < SZ_64K`, the bank firmware section on line 137-= 139: ```c memcpy(lt8713sx->fw_buffer + SZ_64K, lt8713sx->fw->data + SZ_64K, lt8713sx->fw->size - SZ_64K); ``` would produce an underflow in the size argument (`fw->size - SZ_64K` wraps = to a huge number since `size_t` is unsigned). This would be catastrophic. T= he bank firmware copy and bank_num calculation should be guarded by a check= that `fw->size > SZ_64K`. #### 3. `bank_crc_value` array bounds (lines 50, 143-149) ```c u32 bank_crc_value[17]; ... lt8713sx->bank_num =3D (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k; ``` The maximum firmware size is `SZ_256K - 1` (262143 bytes). The bank region = starts at `SZ_64K` (65536), so the max bank data is `262143 - 65536 =3D 196= 607` bytes. With 12K banks: `ceil(196607 / 12288) =3D 16`. The array has 17= entries (indices 0-16), so this is just barely sufficient, but the magic n= umber 17 is fragile. A `#define` or computed constant would be safer and cl= earer. More importantly, there is no bounds check on `bank_num` before usin= g it to index `bank_crc_value[]`. #### 4. `lt8713sx_block_erase` silently ignores timeout (lines 238-249) ```c while (1) { flash_status =3D lt8713sx_read_flash_status(lt8713sx); if ((flash_status & 0x01) =3D=3D 0) break; if (i > 50) break; i++; msleep(50); } ``` When `i > 50`, the loop breaks silently =E2=80=94 the erase timeout is not = reported. The function returns `void` so the caller (`lt8713sx_firmware_upg= rade`) has no idea the erase failed and proceeds to write firmware to a pot= entially not-erased flash. This could result in a corrupted firmware on the= bridge chip. This function should return an error code on timeout. #### 5. `lt8713sx_write_data` always returns 0 (line 313) ```c static int lt8713sx_write_data(struct lt8713sx *lt8713sx, const u8 *data, u= 64 filesize) { ... return 0; } ``` Despite returning `int`, `lt8713sx_write_data` ignores all `regmap_write()`= return values and always returns 0. The callers check `ret < 0` but this c= an never happen. Either the function should check regmap errors, or the ret= urn type should be `void` (and callers adjusted). #### 6. `i2c_device_id` should not contain commas (line 573) ```c static struct i2c_device_id lt8713sx_id[] =3D { { "lontium,lt8713sx", 0 }, ``` The I2C device ID string should not contain a comma =E2=80=94 that's the OF= compatible format, not the I2C device name. Compare with other lontium dri= vers which use e.g. `"lontium,lt9611"`. Wait =E2=80=94 I see the other lont= ium drivers also use the comma format. This appears to be a pre-existing co= nvention in the lontium drivers, though technically it's not correct for I2= C device names (they should match the `driver.name` or be plain device name= s). Additionally, the `0` second field is unnecessary for modern kernels an= d can be omitted (as `lt9611.c` does). Also this should be `const`: ```c static struct i2c_device_id lt8713sx_id[] =3D { ``` should be: ```c static const struct i2c_device_id lt8713sx_id[] =3D { ``` #### 7. `enable_gpio` acquired but not in DT binding (line 475) ```c lt8713sx->enable_gpio =3D devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_= HIGH); ``` The DT binding in patch 1 has no `enable-gpios` property. If this GPIO is n= eeded, add it to the binding. If it's not needed, remove the code. Having d= river code that references undocumented DT properties will cause `dt_bindin= g_check` warnings. #### 8. Unused includes (lines 10, 13, 18, 19) Several headers appear unused: - `` =E2=80=94 no interrupt handling in this driver - `` =E2=80=94 this is an I2C driver - `` =E2=80=94 no wait queues used - `` =E2=80=94 no workqueues used #### 9. `usleep_range(1000, 10000)` has a very wide range (line 446) ```c usleep_range(1000, 10000); ``` A 10x spread between min and max is unusually large. Typically the range sh= ould be tighter, e.g., `usleep_range(1000, 1500)` or `usleep_range(1000, 20= 00)`, unless the hardware truly doesn't care. This warrants a comment or ti= ghtening. #### 10. Global CRC table (line 30, 560) ```c DECLARE_CRC8_TABLE(lt8713sx_crc_table); ... crc8_populate_msb(lt8713sx_crc_table, 0x31); ``` The CRC table is module-global (file-scoped `static`), not per-device. If t= wo instances of this bridge exist, `crc8_populate_msb` would be called twic= e from two different `probe` calls, which is harmless but wasteful. More im= portantly, the table population happens in `probe()` but could theoreticall= y be used via the sysfs attribute before a second `probe()` completes, lead= ing to a race. Consider using `once` semantics or making it `__initdata` wi= th a module init function. #### 11. sysfs attribute naming (line 495) ```c static DEVICE_ATTR_WO(lt8713sx_firmware); ``` This creates a sysfs attribute named `lt8713sx_firmware`. The convention fo= r sysfs attributes is to not prefix with the driver name =E2=80=94 since th= e attribute is already scoped to the device, just `firmware_update` or `upd= ate_firmware` would be clearer. Also, a write-only sysfs attribute that tri= ggers firmware update regardless of what is written is somewhat unusual =E2= =80=94 the content of `buf` is completely ignored. Consider documenting thi= s in the ABI or at minimum adding a comment. #### 12. `lt8713sx_firmware_update` cleanup path (lines 410-421) The `guard(mutex)` at line 387 means the mutex is held until function retur= n. The error label at line 410 always calls `lt8713sx_i2c_disable()`, which= writes to regmap =E2=80=94 this is fine. However, `kvfree(lt8713sx->fw_buf= fer)` is called unconditionally. If `lt8713sx_prepare_firmware_data` fails = at `request_firmware`, `fw_buffer` is NULL (never allocated), and `kvfree(N= ULL)` is safe. If it fails at `kvmalloc`, `fw_buffer` is also NULL. So the = cleanup is correct, but the `if (lt8713sx->fw)` check on line 418 suggests = `fw` might not always be set =E2=80=94 the asymmetry between `fw` (conditio= nal free) and `fw_buffer` (unconditional free) is slightly confusing but fu= nctionally correct. #### 13. `u64` types where `u32` would suffice Several variables use `u64` where `u32` is more than adequate: - `sz_12k` (line 103): `12 * SZ_1K =3D 12288`, easily fits in `u32` - `addr` in `lt8713sx_bank_result_check` (line 348): max value is `0x010000= + 16*0x3000 =3D 0x40000`, fits in `u32` - `filesize` parameter in `lt8713sx_write_data` (line 284): max is `SZ_256K= `, fits in `u32` Using `u64` for these is misleading and generates slightly worse code on 32= -bit architectures. --- Generated by Claude Code Patch Reviewer