Re: [PATCH v9 2/2] drm: Send per-connector hotplug events

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

+To: Simona Vetter, who wrote two of the commits relevant to this question.

On Thursday, 21 May 2026 17:00:17 Central European Summer Time Daniel Stone wrote:
> Hi,
> 
> On Wed, 22 Apr 2026 at 19:24, Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> > @@ -760,16 +760,12 @@ static void output_poll_execute(struct work_struct *work)
> >         struct drm_connector *connector;
> >         struct drm_connector_list_iter conn_iter;
> >         enum drm_connector_status old_status;
> > -       bool repoll = false, changed;
> > +       bool repoll = false, changed = false;
> >         u64 old_epoch_counter;
> >
> >         if (!dev->mode_config.poll_enabled)
> >                 return;
> >
> > -       /* Pick up any changes detected by the probe functions. */
> > -       changed = dev->mode_config.delayed_event;
> > -       dev->mode_config.delayed_event = false;
> > -
> >         if (!drm_kms_helper_poll) {
> >                 if (dev->mode_config.poll_running) {
> >                         drm_kms_helper_disable_hpd(dev);
> > @@ -844,8 +841,15 @@ static void output_poll_execute(struct work_struct *work)
> >         mutex_unlock(&dev->mode_config.mutex);
> >
> >  out:
> > +       /* 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.

The relevant commits are:

Commit 162b6a57ac50 (drm/probe-helper: don't lose hotplug event, 2015-01-21)
Commit ed293f775493 (drm/sysfs: Send out uevent when connector->force changes, 2015-11-19)

One obvious move would be to change it to an atomic so at least we
punch through any caches, but that doesn't help if the intended
design was that it's in a consistent state with the rest of
mode_config. Though I don't think that's the case?

Kind regards,
Nicolas Frattaroli

> 
> The series is very satisfying otherwise; both patches are:
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> 
> Cheers,
> Daniel
>