Hi Marek, On Wed, Oct 28, 2026 at 10:28:41AM +0100, Marek Szyprowski wrote: > Hi, > > Handling this patch takes definitely too long, but maybe You will find a few > minutes > for the v6 with those fixes:
Yeah the ball is definetly in my court. I have been meaning to fix Sashiko's comments and send a new version, but it has not been at the top of my todo list. Hopefully I can find some time to sit down with the hardware and test it again in the somewhat near future, and send a new version. > 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. Thanks for all the hints! Best regards, Henrik Grimler > >> + 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 >
