Re: [PATCHv8 2/2] drm: tda998x: set the connector info

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Thu, Oct 17, 2019 at 11:26:38AM +0200, Dariusz Marcinkiewicz wrote:
> Hi Russel.
> 
> On Wed, Oct 16, 2019 at 6:22 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> ...
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > @@ -1337,6 +1337,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
> > >                                 struct drm_device *drm)
> > >  {
> > >       struct drm_connector *connector = &priv->connector;
> > > +     struct cec_connector_info conn_info;
> > > +     struct cec_notifier *notifier;
> > >       int ret;
> > >
> > >       connector->interlace_allowed = 1;
> > > @@ -1353,6 +1355,17 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
> > >       if (ret)
> > >               return ret;
> > >
> > > +     cec_fill_conn_info_from_drm(&conn_info, connector);
> > > +
> > > +     notifier = cec_notifier_conn_register(priv->cec_glue.parent,
> > > +                                           NULL, &conn_info);
> > > +     if (!notifier)
> > > +             return -ENOMEM;
> > > +
> > > +     mutex_lock(&priv->cec_notify_mutex);
> > > +     priv->cec_notify = notifier;
> > > +     mutex_unlock(&priv->cec_notify_mutex);
> >
> > As per my previous comments, this is a single-copy atomic operation.
> > Either priv->cec_notify is set or it isn't; there is no intermediate
> > value.  It can't be set to a value until cec_notifier_conn_register()
> > has completed.  So the lock doesn't help.
> >
> ....
> > > +
> > >       drm_connector_attach_encoder(&priv->connector,
> > >                                    priv->bridge.encoder);
> > >
> > > @@ -1372,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge)
> ...
> > > +     mutex_lock(&priv->cec_notify_mutex);
> > > +     cec_notifier_conn_unregister(priv->cec_notify);
> > > +     priv->cec_notify = NULL;
> > > +     mutex_unlock(&priv->cec_notify_mutex);
> >
> > This is the only case where the lock makes sense - to ensure that any
> > of the cec_notifier_set_phys_addr*() functions aren't called
> > concurrently with it.  However, there's no locking around the instance
> > in tda998x_connector_get_modes(), so have you ensured that that
> > function can't be called concurrently?
> >
> I assumed that tda998x_connector_get_modes does not need to be
> synchronized as it belongs to the connector that gets cleaned up here.
> But, on a closer look, I don't think that this assumption necessarily
> holds.
> 
> If this patch is to be merged, I can send an update that:
> - strips locking from tda998x_connector_init,
> - in tda998x_connector_get_modes calls cec_notifier* with the lock held.

Okay, I'd suggest a comment in the code describing precisely what the
lock is doing would be a good idea, as it may not be obvious in the
future.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up