Hi, Handling this patch takes definitely too long, but maybe You will find a few minutes for the v6 with those fixes:
On 23.07.2026 11:51, [email protected] wrote: > 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. This is the lack of return dev_err_probe(...) similar to the ones already reported for the earlier versions. > [ ... ] >> @@ -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. This is indeed an important remark. Just call schedule_work(&ctx->extcon_wq); in the else branch for (!ctx->extcon) check at the end of probe function. >> @@ -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. IMHO this is a generic issue with devm_extcon_* API and for now I would suggest getting back to non-devm extcon API. >> + if (ctx->cable_state > 0) >> + sii9234_cable_out(ctx); >> + } else { >> + sii9234_cable_out(ctx); >> + } >> drm_bridge_remove(&ctx->bridge); >> } Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
