Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] drm_bridge_add() is called prematurely in adv7511_probe(), exposing the bridge to concurrent access before initialization completes and risking a Use-After-Free if later probe steps fail. - [High] Hardware regulators are disabled before the DRM bridge is unregistered in adv7511_remove(), allowing concurrent I2C access to powered-down hardware. - [High] Concurrent execution of HPD work and bridge detection modifies hardware sequences and state without synchronization. -- --- Patch [1]: [PATCH] drm/bridge: adv7511: cancel HPD work during teardown --- commit b44080d3f93c1a423486c2876b7db016031c7403 Author: Hongyan Xu <[email protected]> drm/bridge: adv7511: cancel HPD work during teardown The threaded IRQ handler queues hpd_work. The work accesses the regmap and calls drm_bridge_hpd_notify(), so it must not run after the bridge or the devm-allocated driver data has been released. Free the IRQ before cancelling hpd_work so the handler cannot queue the work again. Do this in remove and when adv7533_attach_dsi() fails after the IRQ has been registered. The remove-side race was reported previously, but the proposed fix did not cover the late probe failure path. This issue was found by a static analysis tool. Link: https://lore.kernel.org/r/[email protected] > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index db99e252aeebc..316b9f80d7429 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1400,11 +1400,15 @@ static int adv7511_probe(struct i2c_client *i2c) > if (adv7511->info->has_dsi) { > ret = adv7533_attach_dsi(adv7511); > if (ret) > - goto err_unregister_audio; > + goto err_free_irq; > } > > return 0; > > +err_free_irq: > + if (i2c->irq) > + devm_free_irq(dev, i2c->irq, adv7511); > + cancel_work_sync(&adv7511->hpd_work); > err_unregister_audio: > drm_bridge_remove(&adv7511->bridge); [Severity: High] This is a pre-existing issue, but is it safe to expose the bridge before probe completes? drm_bridge_add() is called earlier in adv7511_probe(), before critical structures like the waitqueue are initialized: drm_bridge_add(&adv7511->bridge); if (i2c->irq) { init_waitqueue_head(&adv7511->wq); If a concurrent KMS driver discovers the bridge and initiates operations, could it crash on the uninitialized waitqueue? For example, adv7511_wait_for_edid() accesses this waitqueue: ret = wait_event_interruptible_timeout(adv7511->wq, adv7511->edid_read, msecs_to_jiffies(timeout)); Also, if the probe subsequently fails and we reach this cleanup path, could the concurrent KMS driver be left with a dangling pointer to devres-allocated memory? [ ... ] > @@ -1425,6 +1429,10 @@ static void adv7511_remove(struct i2c_client *i2c) > { > struct adv7511 *adv7511 = i2c_get_clientdata(i2c); > > + if (i2c->irq) > + devm_free_irq(&i2c->dev, i2c->irq, adv7511); > + cancel_work_sync(&adv7511->hpd_work); > + [Severity: High] This isn't a bug introduced by this patch, but does the driver have an unsynchronized concurrency issue between HPD work and bridge detection? adv7511_hpd_work() runs asynchronously and modifies state: if (status == connector_status_connected && adv7511->status == connector_status_disconnected && adv7511->powered) { regcache_mark_dirty(adv7511->regmap); adv7511_power_on(adv7511); } if (adv7511->status != status) { adv7511->status = status; At the same time, KMS can concurrently call adv7511_detect(): if (status == connector_status_connected && hpd && adv7511->powered) { regcache_mark_dirty(adv7511->regmap); adv7511_power_on(adv7511); if (adv7511->status == connector_status_connected) status = connector_status_disconnected; } Since neither function holds a lock while reading and modifying adv7511->status and adv7511->powered, could both evaluate the state as requiring a power cycle? This might cause both to concurrently invoke adv7511_power_on(), resulting in non-atomic, interleaved I2C register writes and overlapping regcache_sync operations. > of_node_put(adv7511->host_node); > > adv7511_uninit_regulators(adv7511); > > drm_bridge_remove(&adv7511->bridge); [Severity: High] This is a pre-existing issue, but does removing regulators before the bridge is unregistered allow concurrent I2C access to powered-down hardware? In adv7511_remove(), adv7511_uninit_regulators() cuts power to the chip. If a concurrent modeset, hotplug polling thread, or sysfs EDID read invokes bridge operations (like adv7511_detect() or adv7511_edid_read()) before drm_bridge_remove() executes, could the driver attempt I2C transactions on an unresponsive chip? This could potentially cause hardware crashes, I2C bus timeouts, or driver lockups. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
