Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Medium] drm/bridge: lt9611c: negative errno stored in fw_version corrupts firmware_show() - [Medium] drm/bridge: lt9611c: unlocked reset and power control in PM callbacks - [Low] drm/bridge: lt9611c: unused function warnings when CONFIG_PM_SLEEP=n - [Low] drm/bridge: lt9611c: missing MODULE_DEVICE_TABLE() for lt9611c_id - [Low] drm/bridge: lt9611c: unlocked reset of hdmi_connected after bridge attach --
commit 990744b0d700fbd77a4343ac0bdea653a817a6ce Author: Sunyun Yang <[email protected]> drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver This commit adds a DRM bridge driver for the Lontium LT9611C, LT9611EX, and LT9611UXD MIPI DSI to HDMI bridge chips. It supports EDID reading, HPD detection, HDMI audio over I2S, dual-DSI mode, and firmware upgrades via sysfs. > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c > b/drivers/gpu/drm/bridge/lontium-lt9611c.c > new file mode 100644 > index 0000000000000..8fc86c0732122 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c > @@ -0,0 +1,1361 @@ [ ... ] > +static ssize_t firmware_store(struct device *dev, struct device_attribute > *attr, > + const char *buf, size_t len) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + int ret; > + > + if (!lt9611c) > + return -ENODEV; > + > + dev_warn(dev, "starting firmware upgrade — display will be > disrupted\n"); > + > + ret = lt9611c_firmware_upgrade(lt9611c); > + if (ret < 0) { > + dev_err(dev, "upgrade failure\n"); > + return ret; > + } > + > + lt9611c_lock(lt9611c); > + WRITE_ONCE(lt9611c->fw_version, lt9611c_read_version(lt9611c)); > + lt9611c_unlock(lt9611c); > + > + if (READ_ONCE(lt9611c->fw_version) < 0) > + dev_warn(dev, "upgrade succeeded but failed to read new fw > version\n"); > + else > + dev_info(dev, "firmware upgrade succeeded, version: 0x%04x\n", > + READ_ONCE(lt9611c->fw_version)); > + > + return len; > +} > + > +static ssize_t firmware_show(struct device *dev, struct device_attribute > *attr, char *buf) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + > + if (!lt9611c) > + return -ENODEV; > + > + return sysfs_emit(buf, "0x%04x\n", READ_ONCE(lt9611c->fw_version)); > +} [Severity: Medium] In drivers/gpu/drm/bridge/lontium-lt9611c.c, what happens in firmware_show() if reading the version fails after a firmware upgrade in firmware_store()? When regmap_write() or regmap_bulk_read() fails inside lt9611c_read_version(), it returns a negative errno that firmware_store() writes directly into lt9611c->fw_version via WRITE_ONCE(). Subsequent reads of the sysfs firmware attribute will then format that negative errno with "0x%04x" in firmware_show(), emitting a sign-extended 32-bit hex error code (such as 0xfffffffb) to userspace instead of a 16-bit firmware version or an error return. Should firmware_store() only update lt9611c->fw_version when lt9611c_read_version() succeeds, or should firmware_show() check for a negative lt9611c->fw_version before formatting it? [ ... ] > +static int lt9611c_probe(struct i2c_client *client) > +{ [ ... ] > + drm_bridge_add(<9611c->bridge); > + > + /* Attach primary DSI */ > + dsi = lt9611c_attach_dsi(lt9611c, dsi0_node); > + if (IS_ERR(dsi)) { > + ret = PTR_ERR(dsi); > + goto err_remove_bridge; > + } > + > + /* Attach secondary DSI, if specified */ > + if (dsi1_node) { > + dsi = lt9611c_attach_dsi(lt9611c, dsi1_node); > + if (IS_ERR(dsi)) { > + ret = PTR_ERR(dsi); > + goto err_remove_bridge; > + } > + } > + > + lt9611c->hdmi_connected = false; > + enable_irq(client->irq); [Severity: Low] In lt9611c_probe() in drivers/gpu/drm/bridge/lontium-lt9611c.c, can setting lt9611c->hdmi_connected = false here race with lt9611c_bridge_detect()? Once drm_bridge_add() and lt9611c_attach_dsi() have been called, the DSI host can bind the DRM device and run initial connector detection, which invokes lt9611c_bridge_detect() and updates lt9611c->hdmi_connected under lt9611c->mcu_lock: guard(mutex)(<9611c->mcu_lock); ret = lt9611c_read_write_flow(lt9611c, &cmd, &rsp); if (ret) dev_err(dev, "failed to read HPD status (err=%d)\n", ret); else connected = (hpd_status == 0x02); lt9611c->hdmi_connected = connected; Writing lt9611c->hdmi_connected = false in lt9611c_probe() without holding lt9611c->mcu_lock after the bridge is attached can clobber the detected state (and lt9611c is already zero-initialized by devm_drm_bridge_alloc()). Could this assignment be dropped or moved before drm_bridge_add()? [ ... ] > +static int lt9611c_bridge_suspend(struct device *dev) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + int ret; > + > + disable_irq(lt9611c->client->irq); > + cancel_work_sync(<9611c->work); > + > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); > + > + ret = regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), > lt9611c->supplies); > + if (ret) { > + dev_err(lt9611c->dev, "regulator bulk disable failed.\n"); > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0); > + msleep(400); > + enable_irq(lt9611c->client->irq); > + return ret; > + } > + > + return 0; > +} > + > +static int lt9611c_bridge_resume(struct device *dev) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + int ret; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(lt9611c->supplies), > lt9611c->supplies); > + if (ret) { > + dev_err(lt9611c->dev, "regulator bulk enable failed.\n"); > + return ret; > + } > + lt9611c_reset(lt9611c); > + enable_irq(lt9611c->client->irq); > + > + return ret; > +} [Severity: Medium] In drivers/gpu/drm/bridge/lontium-lt9611c.c, should lt9611c_bridge_suspend() and lt9611c_bridge_resume() hold lt9611c->mcu_lock while asserting reset_gpio, toggling the regulators, and running lt9611c_reset()? During system suspend and resume, disable_irq() and cancel_work_sync() only stop the HPD interrupt and lt9611c->work. Because fw_devlink relaxes bidirectional OF-graph links and there is no PM device link ordering this I2C client against the DRM master device or audio codec, bridge callbacks such as lt9611c_bridge_detect(), lt9611c_bridge_edid_read(), lt9611c_bridge_atomic_enable(), or lt9611c_hdmi_audio_prepare() can run concurrently. Those callbacks serialize hardware access with lt9611c->mcu_lock, whereas lt9611c_bridge_suspend() and lt9611c_bridge_resume() assert reset_gpio, disable/enable the vcc and vdd supplies, and run the 400ms reset sequence without acquiring lt9611c->mcu_lock, which could reset or power down the chip mid-transaction in lt9611c_read_write_flow(). > + > +static const struct dev_pm_ops lt9611c_bridge_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(lt9611c_bridge_suspend, > + lt9611c_bridge_resume) > +}; > + > +static const struct i2c_device_id lt9611c_id[] = { > + { .name = "lt9611c", .driver_data = > (kernel_ulong_t)<9611c_chip_data[CHIP_LT9611C] }, > + { .name = "lt9611ex", .driver_data = > (kernel_ulong_t)<9611c_chip_data[CHIP_LT9611EX] }, > + { .name = "lt9611uxd", .driver_data = > (kernel_ulong_t)<9611c_chip_data[CHIP_LT9611UXD] }, > + { /* sentinel */ } > +}; [Severity: Low] In drivers/gpu/drm/bridge/lontium-lt9611c.c, is MODULE_DEVICE_TABLE(i2c, lt9611c_id) missing after lt9611c_id? Without MODULE_DEVICE_TABLE(i2c, lt9611c_id), building the driver as a module (CONFIG_DRM_LONTIUM_LT9611C=m) omits the i2c:lt9611c, i2c:lt9611ex, and i2c:lt9611uxd modalias entries from modules.alias, preventing automatic module loading when devices are instantiated via I2C device IDs. > + > +static const struct of_device_id lt9611c_match_table[] = { > + { .compatible = "lontium,lt9611c", .data = > <9611c_chip_data[CHIP_LT9611C] }, > + { .compatible = "lontium,lt9611ex", .data = > <9611c_chip_data[CHIP_LT9611EX] }, > + { .compatible = "lontium,lt9611uxd", .data = > <9611c_chip_data[CHIP_LT9611UXD] }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, lt9611c_match_table); > + > +static struct i2c_driver lt9611c_driver = { > + .driver = { > + .name = "lt9611c", > + .of_match_table = lt9611c_match_table, > + .pm = <9611c_bridge_pm_ops, > + .dev_groups = lt9611c_attr_groups, > + }, [Severity: Low] In drivers/gpu/drm/bridge/lontium-lt9611c.c, will building with CONFIG_PM_SLEEP=n trigger unused-function warnings for lt9611c_bridge_suspend() and lt9611c_bridge_resume()? SET_SYSTEM_SLEEP_PM_OPS() expands to nothing when CONFIG_PM_SLEEP is disabled, leaving both static functions unreferenced without __maybe_unused annotations. Could this use DEFINE_SIMPLE_DEV_PM_OPS(lt9611c_bridge_pm_ops, lt9611c_bridge_suspend, lt9611c_bridge_resume) (or SYSTEM_SLEEP_PM_OPS()) along with pm_sleep_ptr(<9611c_bridge_pm_ops) on .pm so the compiler can dead-code eliminate the callbacks and dev_pm_ops structure when CONFIG_PM_SLEEP=n? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
