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@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.
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
