Re: [PATCH v5] drm/bridge: sii9234: use extcon cable detection logic to detect MHL

[Henrik Grimler <henrik@grimler.se>]

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, sashiko-bot@kernel.org 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 <henrik@grimler.se>
> >
> > 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
>