Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201201084647.751612010@linuxfoundation.org> <20201201084648.982712007@linuxfoundation.org>
 <20201201154332.GB23661@amd>
In-Reply-To: <20201201154332.GB23661@amd>
From:   Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue, 1 Dec 2020 17:47:12 +0100
Message-ID: <CAKMK7uHw8gJEGQcCwR-MSEpndRKDZwO2BsveDThBSACxZfxLrw@mail.gmail.com>
Subject: Re: [PATCH 4.19 11/57] drm/atomic_helper: Stop modesets on
 unregistered connectors harder
To:     Pavel Machek <pavel@ucw.cz>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        stable <stable@vger.kernel.org>,
        =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        David Airlie <airlied@linux.ie>, Lyude Paul <lyude@redhat.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Christoph Niedermaier <cniedermaier@dh-electronics.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Dec 1, 2020 at 4:43 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > From: Lyude Paul <lyude@redhat.com>
> >
> > commit de9f8eea5a44b0b756d3d6345af7f8e630a3c8c0 upstream.
>
> So this says protected by mutex:
>
> >       /**
> > -      * @registered: Is this connector exposed (registered) with userspace?
> > +      * @registration_state: Is this connector initializing, exposed
> > +      * (registered) with userspace, or unregistered?
> > +      *
> >        * Protected by @mutex.
> >        */
> > -     bool registered;
> > +     enum drm_connector_registration_state registration_state;
> >
> >       /**
> >        * @modes:
> > @@ -1165,6 +1214,24 @@ static inline void drm_connector_unrefer
> >       drm_connector_put(connector);
> >  }
> >
> > +/**
> > + * drm_connector_is_unregistered - has the connector been unregistered from
> > + * userspace?
> > + * @connector: DRM connector
> > + *
> > + * Checks whether or not @connector has been unregistered from userspace.
> > + *
> > + * Returns:
> > + * True if the connector was unregistered, false if the connector is
> > + * registered or has not yet been registered with userspace.
> > + */
> > +static inline bool
> > +drm_connector_is_unregistered(struct drm_connector *connector)
> > +{
> > +     return READ_ONCE(connector->registration_state) ==
> > +             DRM_CONNECTOR_UNREGISTERED;
> > +}
>
>
> But this uses READ_ONCE() for protection, and corresponding
> WRITE_ONCE() is nowhere to be seen. Should this take the mutex, too?

The read once here doesn't protect against any races, but just against
creative compilers doing funny stuff that might really break code
logic. I guess for symmetry we could throw the WRITE_ONCE on the write
side, but it really shouldn't matter, the entire thing is racy by
design. We0d also only ever need the write once on the unregister
call.
-Daniel

>
> Best regards,
>                                                                 Pavel
> --
> http://www.livejournal.com/~pavelmachek


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch