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 Lontium LT7911EXC eDP to MIPI DSI bridge Date: Mon, 25 May 2026 19:24:24 +1000 Message-ID: In-Reply-To: <20260522015735.2833-3-syyang@lontium.com> References: <20260522015735.2833-1-syyang@lontium.com> <20260522015735.2833-3-syyang@lontium.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 #### Bug: Firmware leak and goto label ordering error The `lt7911exc_firmware_upgrade_work` function has a critical goto ordering= problem. When the firmware upgrade fails after `mutex_lock(<7911exc->upg= rade_lock)`, the error path jumps to `out_unlock`, which falls through to `= out_release_fw` and then `out_clear_status`. But when `fw->size > total_siz= e` or `kvmalloc` fails, the code jumps to `out_release_fw` =E2=80=94 and th= en falls through to `out_clear_status`. However, the `out_unlock` label als= o falls through to `out_release_fw`. The problem is this sequence: ```c out_unlock: mutex_lock(<7911exc->ocm_lock); lt7911exc_hw_mcu_run(lt7911exc); lt7911exc->fw_version =3D lt7911exc_read_version(lt7911exc); mutex_unlock(<7911exc->ocm_lock); mutex_unlock(<7911exc->upgrade_lock); /* Notify DRM framework that hardware state changed/needs a modeset */ if (lt7911exc->bridge.dev) drm_kms_helper_hotplug_event(lt7911exc->bridge.dev); out_release_fw: release_firmware(fw); out_clear_status: ``` When `lt7911exc_block_erase` or `lt7911exc_write_data` fails and we goto `o= ut_unlock`, this is correct. But the `out_release_fw` label is positioned *= *after** `out_unlock`, so when we goto `out_release_fw` directly (from the = size check or kvmalloc failure), `release_firmware` is called but the `upgr= ade_lock` is never unlocked =E2=80=94 the lock was never taken in that path= , so that's fine. Wait =E2=80=94 actually, looking more carefully, the `upg= rade_lock` is taken **after** the `buffer` work: ```c mutex_lock(<7911exc->upgrade_lock); ... ret =3D lt7911exc_block_erase(lt7911exc); if (ret) { ... goto out_unlock; } ``` The early errors (`fw->size > total_size`, kvmalloc failure) goto `out_rele= ase_fw` which does NOT unlock `upgrade_lock` because it was never taken =E2= =80=94 this is correct. But the real bug is that the `out_unlock` path runs= `lt7911exc_hw_mcu_run` and `lt7911exc_read_version` even on error, then do= es `drm_kms_helper_hotplug_event` after a failed upgrade, which generates a= spurious hotplug event. The hotplug event is misleading but not a crash. **However**, there is a subtle but real issue: the `out_clear_status` block= accesses `lt7911exc` fields after `release_firmware(fw)` =E2=80=94 the fir= mware pointer `fw` is unrelated to `lt7911exc`, so this is fine for memory = safety. But the **actual bug** is: if `request_firmware` fails, we jump to = `out_clear_status` directly, and the `upgrade` flag is correctly cleared. G= ood. #### Bug: Firmware data vs CRC mismatch The firmware upgrade writes a CRC computed from a zero-padded copy: ```c buffer =3D kvmalloc(total_size, GFP_KERNEL); ... memset(buffer, 0xff, total_size); memcpy(buffer, fw->data, fw->size); crc32 =3D cal_crc32_custom(buffer, total_size); kvfree(buffer); ``` But the actual write to hardware uses the raw firmware data: ```c ret =3D lt7911exc_write_data(lt7911exc, fw, 0); ``` Inside `lt7911exc_write_data`, only `fw->size` bytes are written. The CRC i= s computed over the full `total_size` (64K - 4) with 0xFF padding, but the = hardware only receives `fw->size` bytes. Either the full padded buffer shou= ld be written, or the CRC should be computed over just `fw->data` with `fw-= >size`. As-is, the CRC verification at the end (`lt7911exc_upgrade_result`)= will always fail if `fw->size < total_size`, because the hardware CRC won'= t match the padded CRC. #### Issue: Unchecked regmap writes in flash programming Throughout the flash programming functions, many `regmap_write` calls have = their return values silently discarded: ```c static int lt7911exc_write_crc(struct lt7911exc *lt7911exc, u32 crc32, u64 = addr) { ... regmap_write(lt7911exc->regmap, 0xe05f, 0x01); regmap_write(lt7911exc->regmap, 0xe05a, (addr >> 16) & 0xff); regmap_write(lt7911exc->regmap, 0xe05b, (addr >> 8) & 0xff); regmap_write(lt7911exc->regmap, 0xe05c, addr & 0xff); ``` Similarly in `lt7911exc_write_data`: ```c regmap_write(lt7911exc->regmap, 0xe05f, 0x05); regmap_write(lt7911exc->regmap, 0xe05f, 0x01); ``` And in `lt7911exc_upgrade_result`: ```c regmap_write(lt7911exc->regmap, 0xe0ee, 0x01); regmap_write(lt7911exc->regmap, 0xe07b, 0x60); regmap_write(lt7911exc->regmap, 0xe07b, 0x40); ``` If any of these I2C writes fail during a flash erase/program operation, the= firmware upgrade will silently produce corrupted results. These should at = minimum be checked and cause an early return on error. #### Issue: Locking design =E2=80=94 upgrade_lock released too early in sys= fs path In `lt7911exc_firmware_store`, the `upgrade_lock` is taken with `mutex_tryl= ock`, then the `upgrade` flag is set, and **both locks are released**: ```c if (!mutex_trylock(<7911exc->upgrade_lock)) return -EBUSY; mutex_lock(<7911exc->ocm_lock); ... lt7911exc->upgrade =3D true; mutex_unlock(<7911exc->ocm_lock); mutex_unlock(<7911exc->upgrade_lock); schedule_work(<7911exc->work); ``` The `upgrade_lock` is released before `schedule_work`. Then in the worker, = `upgrade_lock` is re-acquired. But between the sysfs `store` releasing `upg= rade_lock` and the worker acquiring it, a second `store` could `mutex_trylo= ck(&upgrade_lock)` successfully and also see `upgrade =3D=3D true` =E2=86= =92 return `-EBUSY`. This is actually safe because of the `upgrade` flag ch= eck, but it means the `upgrade_lock` in `firmware_store` serves no real pur= pose =E2=80=94 the `ocm_lock` + `upgrade` flag alone would be sufficient. T= he two-lock scheme is confusing and should be simplified. #### Issue: Sysfs attribute naming ```c static DEVICE_ATTR_RW(lt7911exc_firmware); ``` This creates an attribute named `lt7911exc_firmware`. Kernel sysfs conventi= ons say the attribute should not include the driver name as a prefix =E2=80= =94 it should just be `firmware` (or `firmware_version` / `firmware_update`= ). The device is already scoped to this driver instance. Also, having a sin= gle attribute for both read (version) and write (trigger upgrade) is confus= ing =E2=80=94 consider splitting into separate `firmware_version` (read-onl= y) and `firmware_update` (write-only) attributes. #### Issue: Kconfig help text formatting ```c help DRM driver for the Lontium LT7911EXC bridge chip.The LT7911EXC converts eDP input to MIPI ``` Missing space after period: `chip.The` should be `chip. The`. #### Issue: Comment style Several comments use C++ style which, while accepted, is inconsistent with = typical kernel bridge driver style: ```c //hardware requires delay usleep_range(1000, 2000); ``` ```c //enable mipi stream ``` ```c //disable mipi stream ``` Should use `/* ... */` style per kernel coding conventions. #### Issue: `lt7911exc_dsi_host_transfer` is a no-op The transfer callback accepts write commands but does nothing with them: ```c guard(mutex)(<7911exc->ocm_lock); if (lt7911exc->upgrade) return -EBUSY; return msg->tx_len; ``` The comment explains this is intentional (firmware handles DSI init), which= is fine. But the function acquires the `ocm_lock` and checks the `upgrade`= flag for writes that are immediately discarded =E2=80=94 this is unnecessa= ry overhead. If the firmware truly handles everything, consider whether thi= s callback is needed at all, or document more clearly why the lock/flag che= ck is still needed (e.g., to prevent panel init during upgrade). #### Issue: `lt7911exc_prog_init` has an unnecessary pattern ```c ret =3D regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(se= q_write)); if (ret) return ret; return 0; ``` Should just be `return regmap_multi_reg_write(...)`. #### Nit: `u64` type for addresses that fit in `u32` ```c static int lt7911exc_prog_init(struct lt7911exc *lt7911exc, u64 addr) ``` ```c static int lt7911exc_write_data(struct lt7911exc *lt7911exc, const struct f= irmware *fw, u64 addr) ``` ```c static int lt7911exc_write_crc(struct lt7911exc *lt7911exc, u32 crc32, u64 = addr) ``` The flash addresses are clearly within a 64KB space (`FW_SIZE =3D 64 * 1024= `), and the register writes only use the lower 24 bits. Using `u64` for the= se addresses is misleading =E2=80=94 `u32` would be appropriate. #### Nit: `cal_crc32_custom` naming The function name `cal_crc32_custom` is vague. Something like `lt7911exc_cr= c32_be_byteswap` would better describe that it computes a CRC32-BE over byt= e-reversed 32-bit words. Also, the `length` parameter should be `size_t`, n= ot `u64`. #### Nit: `dev_set_drvdata` and `i2c_set_clientdata` redundancy In probe: ```c dev_set_drvdata(dev, lt7911exc); ... i2c_set_clientdata(client, lt7911exc); ``` `i2c_set_clientdata(client, lt7911exc)` calls `dev_set_drvdata(&client->dev= , lt7911exc)` internally. Since `dev =3D &client->dev`, these are redundant= . Remove the `dev_set_drvdata` call. #### Nit: `of_node` check is unnecessary ```c if (!np) return -ENODEV; ``` The driver has `depends on OF` in Kconfig and uses OF match table, so `of_n= ode` will always be set when the driver probes. This check is superfluous. #### Missing: `drm_panel_bridge_remove` leak on detach In `lt7911exc_dsi_host_detach`, if a panel bridge was created via `drm_pane= l_bridge_add_typed` in the attach path, the detach only clears `output.dev`= but does not call `drm_panel_bridge_remove`: ```c static int lt7911exc_dsi_host_detach(struct mipi_dsi_host *host, struct mip= i_dsi_device *dev) { struct lt7911exc *lt7911exc =3D dsi_host_to_lt7911exc(host); if (!lt7911exc->output.dev) return 0; lt7911exc->output.dev =3D NULL; return 0; } ``` The panel bridge is only cleaned up in `lt7911exc_remove`. If a DSI device = attaches, detaches, and attaches again, `drm_panel_bridge_add_typed` would = be called a second time, leaking the first bridge. The detach callback shou= ld clean up `output.bridge` and `output.panel` as well. --- Generated by Claude Code Patch Reviewer