Hi Russel.
On Wed, Oct 16, 2019 at 6:22 PM Russell King - ARM Linux admin
<linux@xxxxxxxxxxxxxxx> 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!
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
