Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Use-After-Free due to race between `flush_work()` and devres teardown. - [High] Missing initial state check breaks display on coldplug. - [Medium] Missing `return` statement after `dev_err_probe()` ignores initialization errors. --
--- Patch [1]: [PATCH v5] drm/bridge: sii9234: use extcon cable detection logic to detect MHL --- commit 15547772b748ad5e9e79681b157e1cae684d850d Author: Henrik Grimler <[email protected]> drm/bridge: sii9234: use extcon cable detection logic to detect MHL To use MHL we currently need the MHL chip to be permanently on, which consumes unnecessary power. Let's use extcon attached to MUIC to enable the MHL chip only if it detects an MHL cable. > diff --git a/drivers/gpu/drm/bridge/sii9234.c > b/drivers/gpu/drm/bridge/sii9234.c > index be67642ab7d31..bf211906fefc6 100644 > --- a/drivers/gpu/drm/bridge/sii9234.c > +++ b/drivers/gpu/drm/bridge/sii9234.c [ ... ] > @@ -864,6 +870,63 @@ static int sii9234_init_resources(struct sii9234 *ctx, > return 0; > } > > +static void sii9234_extcon_work(struct work_struct *work) > +{ > + struct sii9234 *ctx = container_of(work, struct sii9234, extcon_wq); > + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); > + > + if (state == ctx->cable_state) > + return; > + > + ctx->cable_state = state; > + > + if (state > 0) > + sii9234_cable_in(ctx); > + else > + sii9234_cable_out(ctx); > +} [ ... ] > +static int sii9234_extcon_init(struct sii9234 *ctx) > +{ [ ... ] > + ctx->extcon = edev; > + ctx->extcon_nb.notifier_call = sii9234_extcon_notifier; > + INIT_WORK(&ctx->extcon_wq, sii9234_extcon_work); > + ret = devm_extcon_register_notifier(ctx->dev, edev, EXTCON_DISP_MHL, > &ctx->extcon_nb); > + if (ret) > + dev_err_probe(ctx->dev, ret, "failed to register notifier for > MHL\n"); > + > + return 0; > +} [Severity: Medium] Will this ignore initialization errors? If devm_extcon_register_notifier() fails (for example, returning -EPROBE_DEFER or -ENOMEM), the error is logged but ignored since there is no return statement after dev_err_probe(). The probe function completes successfully and assumes ctx->extcon is active. [ ... ] > @@ -919,12 +982,17 @@ static int sii9234_probe(struct i2c_client *client) [ ... ] > i2c_set_clientdata(client, ctx); > > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > - sii9234_cable_in(ctx); > + if (!ctx->extcon) > + sii9234_cable_in(ctx); > > return 0; > } [Severity: High] Does this code skip setting the initial state if the cable is already plugged in during boot? Extcon notifiers trigger on state changes but do not automatically emit an event for the current state upon registration. In sii9234_probe(), if an extcon device is found, sii9234_cable_in() is skipped, but the driver never queries the initial state. If the cable is already connected when the driver probes, the hardware will never be powered on, and the display will remain non-functional until the user physically unplugs and replugs the cable. > @@ -933,7 +1001,13 @@ static void sii9234_remove(struct i2c_client *client) > { > struct sii9234 *ctx = i2c_get_clientdata(client); > > - sii9234_cable_out(ctx); > + if (ctx->extcon) { > + flush_work(&ctx->extcon_wq); [Severity: High] Does this code introduce a race condition with the devres teardown? Since the notifier is registered via devm_extcon_register_notifier(), it will only be unregistered after sii9234_remove() completes. If an extcon state change occurs right after flush_work(&ctx->extcon_wq), sii9234_extcon_notifier() will queue the work again. Once sii9234_remove() completes, devres will unregister the notifier and free ctx (allocated via devm_drm_bridge_alloc()). When the queued work executes in sii9234_extcon_work(): struct sii9234 *ctx = container_of(work, struct sii9234, extcon_wq); int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); it will dereference the freed ctx, resulting in a use-after-free. > + if (ctx->cable_state > 0) > + sii9234_cable_out(ctx); > + } else { > + sii9234_cable_out(ctx); > + } > drm_bridge_remove(&ctx->bridge); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
