| Message ID | 20191016133916.21475-3-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: tda998x: use cec_notifier_conn_(un)register | expand |
On Wed, Oct 16, 2019 at 03:39:16PM +0200, Hans Verkuil wrote: > From: Dariusz Marcinkiewicz <darekm@google.com> > > Fill in the cec_connector_info when calling cec_notifier_conn_register(). > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 8262b44b776e..b0620842fa3a 100644 > --- 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) > { > struct tda998x_priv *priv = bridge_to_tda998x_priv(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? Thanks.
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. Thank you!
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.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 8262b44b776e..b0620842fa3a 100644 --- 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); + drm_connector_attach_encoder(&priv->connector, priv->bridge.encoder); @@ -1372,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge) { struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + mutex_lock(&priv->cec_notify_mutex); + cec_notifier_conn_unregister(priv->cec_notify); + priv->cec_notify = NULL; + mutex_unlock(&priv->cec_notify_mutex); + drm_connector_cleanup(&priv->connector); } @@ -1795,11 +1813,6 @@ static void tda998x_destroy(struct device *dev) cancel_work_sync(&priv->detect_work); i2c_unregister_device(priv->cec); - - mutex_lock(&priv->cec_notify_mutex); - cec_notifier_conn_unregister(priv->cec_notify); - priv->cec_notify = NULL; - mutex_unlock(&priv->cec_notify_mutex); } static int tda998x_create(struct device *dev) @@ -1925,14 +1938,6 @@ static int tda998x_create(struct device *dev) cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); } - mutex_lock(&priv->cec_notify_mutex); - priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); - mutex_unlock(&priv->cec_notify_mutex); - if (!priv->cec_notify) { - ret = -ENOMEM; - goto fail; - } - priv->cec_glue.parent = dev; priv->cec_glue.data = priv; priv->cec_glue.init = tda998x_cec_hook_init;