On Fri, 22 May 2026 at 14:06, Nicolas Frattaroli
<[email protected]> wrote:
> On Thursday, 21 May 2026 17:00:17 Central European Summer Time Daniel Stone
> wrote:
> > On Wed, 22 Apr 2026 at 19:24, Nicolas Frattaroli
> > <[email protected]> wrote:
> > > + /* Pick up any changes detected by the probe functions. */
> > > + if (dev->mode_config.delayed_event) {
> >
> > Not your doing, as it was just the same before, but doesn't this read
> > need to be protected by the mode_config mutex?
>
> It looks like the fuzzy meaning of the mode_config.mutex came to
> bite here. It is set to true with the lock held in
> drm_helper_probe_single_connector_modes(), but set to false in this
> function without the lock, and always read without the lock
> (drm_kms_helper_poll_enable(), reschedule_output_poll_work()). Some
> of these calls happen in paths where it might be inconvenient to take
> a lock this coarse.
>
> Specifically, drm_helper_probe_single_connector_modes() has this
> comment right before setting delayed_event to true:
>
> /*
> * The hotplug event code might call into the fb
> * helpers, and so expects that we do not hold any
> * locks. Fire up the poll struct instead, it will
> * disable itself again.
> */
>
> So there was a problem with accessing parts under a lock before,
> so if we're reintroducing the coarse lock wherever the boolean is
> accessed we might be going backwards here.
Yeah, that makes sense - those are definitely called from an IRQ
context so it wouldn't be safe to take the mode_config mutex.
I wonder if the safest path which guarantees eventual consistency is
to change the !trylock(mode_config) { goto out } path in
output_poll_execute() to jump directly to the schedule_delayed_work()?
That way we could read and modify delayed_event under the mode_config
mutex; the if (changed) path will not be taken if it wasn't possible
to take the mode_config mutex.
Cheers,
Daniel
