Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Incomplete error handling in `adv7511_power_on()` leaves state out of sync Pre-existing issues: - [High] Missing `cancel_work_sync()` in removal path causes Use-After-Free - [Low] I2C device ID array uses positional initializers -- --- Patch [2]: [PATCH 2/2] drm/bridge: adv7511: handle regcache_sync() failure on power on --- commit 5725e03d2519d4fb324dba03e70e2f0824a5bd67 Author: bui duc phuc <[email protected]> drm/bridge: adv7511: handle regcache_sync() failure on power on The commit adds error handling for regcache_sync() in adv7511_power_on(). If it fails, it logs an error and attempts to power off the chip. > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index db55423781ea9..452180a95c384 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -367,12 +367,20 @@ static void __adv7511_power_on(struct adv7511 *adv7511) > > static void adv7511_power_on(struct adv7511 *adv7511) > { > + int ret; > + > __adv7511_power_on(adv7511); > > /* > * Most of the registers are reset during power down or when HPD is low. > */ > - regcache_sync(adv7511->regmap); > + ret = regcache_sync(adv7511->regmap); > + if (ret) { > + dev_err(&adv7511->i2c_main->dev, > + "Failed to sync register cache: %d\n", ret); > + __adv7511_power_off(adv7511); > + return; > + } [Severity: High] Does this early return leave the hardware state out of sync? If adv7511_power_on() is called when the device was already logically powered, this early return bypasses setting adv7511->powered = false and skips calling adv7533_dsi_power_off(). Could this leave the DSI hardware powered on while the software state remains powered == true, causing subsequent EDID reads to fail because they see powered == true and attempt to read from a chip where the rest of the components are powered off? > > if (adv7511->info->has_dsi) > adv7533_dsi_power_on(adv7511); While reviewing this, I noticed a pre-existing issue in the removal path: > static void adv7511_remove(struct i2c_client *i2c) > { > struct adv7511 *adv7511 = i2c_get_clientdata(i2c); [Severity: High] This isn't a bug introduced by this patch, but does this function need to cancel the hotplug work item? If a hotplug interrupt fires and schedules adv7511->hpd_work just before or during adv7511_remove(), the work could run after the memory is freed by devres, resulting in a use-after-free. Would adding cancel_work_sync(&adv7511->hpd_work) here prevent this? I also noticed another pre-existing issue in the I2C device ID array: > static const struct i2c_device_id adv7511_i2c_ids[] = { > { "adv7511", (kernel_ulong_t)&adv7511_chip_info }, > { "adv7511w", (kernel_ulong_t)&adv7511_chip_info }, [Severity: Low] This is a pre-existing issue, but the I2C subsystem guidelines state that initialized arrays of type struct i2c_device_id must use named initializers. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
