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.