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

[Dariusz Marcinkiewicz]

Hi Russel.

On Wed, Oct 16, 2019 at 6:22 PM Russell King - ARM Linux admin
<linux at 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.

Thank you!