* [PATCHv8 0/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-16 13:39 ` Hans Verkuil 0 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-16 13:39 UTC (permalink / raw) To: linux-media Cc: Dariusz Marcinkiewicz, Laurent Pinchart, dri-devel, Russell King This splits the previous v7.2 patch (1) into two parts: one that replaces cec_notifier_get/put by cec_notifier_conn_(un)register, and one that sets the connector info. That second patch moves the CEC notifier code to tda998x_bridge_detach, but Laurent is making changes in that area and prefers that this isn't implemented yet. But the replacement of the cec_notifier_get/put functions is something that needs to be finished for v5.5. By splitting this patch up I can merge the first half, but delay the second half. This tda driver just won't be able to report the connector information in the meantime. Regards, Hans (1) https://patchwork.linuxtv.org/patch/58464/ Dariusz Marcinkiewicz (2): drm: tda998x: use cec_notifier_conn_(un)register drm: tda998x: set the connector info drivers/gpu/drm/i2c/tda998x_drv.c | 38 ++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 11 deletions(-) -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv8 0/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-16 13:39 ` Hans Verkuil 0 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-16 13:39 UTC (permalink / raw) To: linux-media Cc: Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Russell King, Laurent Pinchart This splits the previous v7.2 patch (1) into two parts: one that replaces cec_notifier_get/put by cec_notifier_conn_(un)register, and one that sets the connector info. That second patch moves the CEC notifier code to tda998x_bridge_detach, but Laurent is making changes in that area and prefers that this isn't implemented yet. But the replacement of the cec_notifier_get/put functions is something that needs to be finished for v5.5. By splitting this patch up I can merge the first half, but delay the second half. This tda driver just won't be able to report the connector information in the meantime. Regards, Hans (1) https://patchwork.linuxtv.org/patch/58464/ Dariusz Marcinkiewicz (2): drm: tda998x: use cec_notifier_conn_(un)register drm: tda998x: set the connector info drivers/gpu/drm/i2c/tda998x_drv.c | 38 ++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 11 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register 2019-10-16 13:39 ` Hans Verkuil @ 2019-10-16 13:39 ` Hans Verkuil -1 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-16 13:39 UTC (permalink / raw) To: linux-media Cc: Russell King, dri-devel, Laurent Pinchart, Hans Verkuil, Dariusz Marcinkiewicz From: Dariusz Marcinkiewicz <darekm@google.com> Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 84c6d4c91c65..8262b44b776e 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -82,6 +82,9 @@ struct tda998x_priv { u8 audio_port_enable[AUDIO_ROUTE_NUM]; struct tda9950_glue cec_glue; struct gpio_desc *calib; + + /* protect cec_notify */ + struct mutex cec_notify_mutex; struct cec_notifier *cec_notify; }; @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) tda998x_edid_delay_start(priv); } else { schedule_work(&priv->detect_work); - cec_notifier_set_phys_addr(priv->cec_notify, - CEC_PHYS_ADDR_INVALID); + + mutex_lock(&priv->cec_notify_mutex); + cec_notifier_phys_addr_invalidate( + priv->cec_notify); + mutex_unlock(&priv->cec_notify_mutex); } handled = true; @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) i2c_unregister_device(priv->cec); - if (priv->cec_notify) - cec_notifier_put(priv->cec_notify); + 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) @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex); + mutex_init(&priv->cec_notify_mutex); INIT_LIST_HEAD(&priv->bridge.list); init_waitqueue_head(&priv->edid_delay_waitq); timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); } - priv->cec_notify = cec_notifier_get(dev); + 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; -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-16 13:39 ` Hans Verkuil 0 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-16 13:39 UTC (permalink / raw) To: linux-media Cc: Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Russell King, Laurent Pinchart, Hans Verkuil From: Dariusz Marcinkiewicz <darekm@google.com> Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 84c6d4c91c65..8262b44b776e 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -82,6 +82,9 @@ struct tda998x_priv { u8 audio_port_enable[AUDIO_ROUTE_NUM]; struct tda9950_glue cec_glue; struct gpio_desc *calib; + + /* protect cec_notify */ + struct mutex cec_notify_mutex; struct cec_notifier *cec_notify; }; @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) tda998x_edid_delay_start(priv); } else { schedule_work(&priv->detect_work); - cec_notifier_set_phys_addr(priv->cec_notify, - CEC_PHYS_ADDR_INVALID); + + mutex_lock(&priv->cec_notify_mutex); + cec_notifier_phys_addr_invalidate( + priv->cec_notify); + mutex_unlock(&priv->cec_notify_mutex); } handled = true; @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) i2c_unregister_device(priv->cec); - if (priv->cec_notify) - cec_notifier_put(priv->cec_notify); + 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) @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex); + mutex_init(&priv->cec_notify_mutex); INIT_LIST_HEAD(&priv->bridge.list); init_waitqueue_head(&priv->edid_delay_waitq); timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); } - priv->cec_notify = cec_notifier_get(dev); + 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; -- 2.23.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register 2019-10-16 13:39 ` Hans Verkuil @ 2019-10-16 16:14 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 20+ messages in thread From: Russell King - ARM Linux admin @ 2019-10-16 16:14 UTC (permalink / raw) To: Hans Verkuil Cc: Dariusz Marcinkiewicz, Laurent Pinchart, dri-devel, linux-media On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: > From: Dariusz Marcinkiewicz <darekm@google.com> > > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Please explain in detail what this mutex actually achieves. > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 84c6d4c91c65..8262b44b776e 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -82,6 +82,9 @@ struct tda998x_priv { > u8 audio_port_enable[AUDIO_ROUTE_NUM]; > struct tda9950_glue cec_glue; > struct gpio_desc *calib; > + > + /* protect cec_notify */ > + struct mutex cec_notify_mutex; > struct cec_notifier *cec_notify; > }; > > @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) > tda998x_edid_delay_start(priv); > } else { > schedule_work(&priv->detect_work); > - cec_notifier_set_phys_addr(priv->cec_notify, > - CEC_PHYS_ADDR_INVALID); > + > + mutex_lock(&priv->cec_notify_mutex); > + cec_notifier_phys_addr_invalidate( > + priv->cec_notify); > + mutex_unlock(&priv->cec_notify_mutex); > } > > handled = true; > @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) > > i2c_unregister_device(priv->cec); > > - if (priv->cec_notify) > - cec_notifier_put(priv->cec_notify); > + mutex_lock(&priv->cec_notify_mutex); > + cec_notifier_conn_unregister(priv->cec_notify); > + priv->cec_notify = NULL; > + mutex_unlock(&priv->cec_notify_mutex); By the time we get here: 1) The interrupt has been freed (which is a synchronous operation) tda998x_irq_thread() can't be called and can't be running, and therefore cec_notifier_phys_addr_invalidate() also can't be called or be running. 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site; if there's any case that _might_ possibly conflict, it is that one. 3) tda998x_destroy() and tda998x_create() can't be called concurrently in any case; the driver model guarantees that ->probe and ->remove for the same device are serialised. > } > > static int tda998x_create(struct device *dev) > @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) > mutex_init(&priv->mutex); /* protect the page access */ > mutex_init(&priv->audio_mutex); /* protect access from audio thread */ > mutex_init(&priv->edid_mutex); > + mutex_init(&priv->cec_notify_mutex); > INIT_LIST_HEAD(&priv->bridge.list); > init_waitqueue_head(&priv->edid_delay_waitq); > timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); > @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) > cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); > } > > - priv->cec_notify = cec_notifier_get(dev); > + mutex_lock(&priv->cec_notify_mutex); > + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); > + mutex_unlock(&priv->cec_notify_mutex); and: 4) priv->cec_notify will be NULL here until such time that cec_notifier_conn_register() has returned. If the mutex is trying to protect something, it's very unclear what it is. Trying to guess what it's protecting against: - Is it protecting against NULL priv->cec_notify? No, because it can be NULL just before we take the lock. - Is it protecting the internals of cec_notifier_conn_register() against the other calls - no, because priv->cec_notify will be NULL until the function has finished. - Is it protecting the write to priv->cec_notify? Maybe, but that doesn't need any protection - architectures are single-copy atomic, which means that a pointer is either written or it is not written. Therefore, it will either be NULL (the state before the call is made) or it will be set correctly (after the call has completed.) So, all in all, I don't see what this lock is doing, and I think it should be removed. If it's necessary for a future change (which may or may not be merged) then the lock should be part of that future change, because the change proposed by this patch certainly does not need it. 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-16 16:14 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 20+ messages in thread From: Russell King - ARM Linux admin @ 2019-10-16 16:14 UTC (permalink / raw) To: Hans Verkuil Cc: linux-media, Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Laurent Pinchart On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: > From: Dariusz Marcinkiewicz <darekm@google.com> > > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Please explain in detail what this mutex actually achieves. > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 84c6d4c91c65..8262b44b776e 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -82,6 +82,9 @@ struct tda998x_priv { > u8 audio_port_enable[AUDIO_ROUTE_NUM]; > struct tda9950_glue cec_glue; > struct gpio_desc *calib; > + > + /* protect cec_notify */ > + struct mutex cec_notify_mutex; > struct cec_notifier *cec_notify; > }; > > @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) > tda998x_edid_delay_start(priv); > } else { > schedule_work(&priv->detect_work); > - cec_notifier_set_phys_addr(priv->cec_notify, > - CEC_PHYS_ADDR_INVALID); > + > + mutex_lock(&priv->cec_notify_mutex); > + cec_notifier_phys_addr_invalidate( > + priv->cec_notify); > + mutex_unlock(&priv->cec_notify_mutex); > } > > handled = true; > @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) > > i2c_unregister_device(priv->cec); > > - if (priv->cec_notify) > - cec_notifier_put(priv->cec_notify); > + mutex_lock(&priv->cec_notify_mutex); > + cec_notifier_conn_unregister(priv->cec_notify); > + priv->cec_notify = NULL; > + mutex_unlock(&priv->cec_notify_mutex); By the time we get here: 1) The interrupt has been freed (which is a synchronous operation) tda998x_irq_thread() can't be called and can't be running, and therefore cec_notifier_phys_addr_invalidate() also can't be called or be running. 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site; if there's any case that _might_ possibly conflict, it is that one. 3) tda998x_destroy() and tda998x_create() can't be called concurrently in any case; the driver model guarantees that ->probe and ->remove for the same device are serialised. > } > > static int tda998x_create(struct device *dev) > @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) > mutex_init(&priv->mutex); /* protect the page access */ > mutex_init(&priv->audio_mutex); /* protect access from audio thread */ > mutex_init(&priv->edid_mutex); > + mutex_init(&priv->cec_notify_mutex); > INIT_LIST_HEAD(&priv->bridge.list); > init_waitqueue_head(&priv->edid_delay_waitq); > timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); > @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) > cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); > } > > - priv->cec_notify = cec_notifier_get(dev); > + mutex_lock(&priv->cec_notify_mutex); > + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); > + mutex_unlock(&priv->cec_notify_mutex); and: 4) priv->cec_notify will be NULL here until such time that cec_notifier_conn_register() has returned. If the mutex is trying to protect something, it's very unclear what it is. Trying to guess what it's protecting against: - Is it protecting against NULL priv->cec_notify? No, because it can be NULL just before we take the lock. - Is it protecting the internals of cec_notifier_conn_register() against the other calls - no, because priv->cec_notify will be NULL until the function has finished. - Is it protecting the write to priv->cec_notify? Maybe, but that doesn't need any protection - architectures are single-copy atomic, which means that a pointer is either written or it is not written. Therefore, it will either be NULL (the state before the call is made) or it will be set correctly (after the call has completed.) So, all in all, I don't see what this lock is doing, and I think it should be removed. If it's necessary for a future change (which may or may not be merged) then the lock should be part of that future change, because the change proposed by this patch certainly does not need it. 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register 2019-10-16 16:14 ` Russell King - ARM Linux admin @ 2019-10-17 7:03 ` Hans Verkuil -1 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-17 7:03 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Dariusz Marcinkiewicz, Laurent Pinchart, dri-devel, linux-media On 10/16/19 6:14 PM, Russell King - ARM Linux admin wrote: > On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: >> From: Dariusz Marcinkiewicz <darekm@google.com> >> >> Use the new cec_notifier_conn_(un)register() functions to >> (un)register the notifier for the HDMI connector. >> >> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Please explain in detail what this mutex actually achieves. Dariusz, since you're the author, can you reply to Russell? If this is going to be a delaying factor, then I'll post a new version without the mutex that just replaces the cec_notifier API. Regards, Hans > >> --- >> drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c >> index 84c6d4c91c65..8262b44b776e 100644 >> --- a/drivers/gpu/drm/i2c/tda998x_drv.c >> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c >> @@ -82,6 +82,9 @@ struct tda998x_priv { >> u8 audio_port_enable[AUDIO_ROUTE_NUM]; >> struct tda9950_glue cec_glue; >> struct gpio_desc *calib; >> + >> + /* protect cec_notify */ >> + struct mutex cec_notify_mutex; >> struct cec_notifier *cec_notify; >> }; >> >> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) >> tda998x_edid_delay_start(priv); >> } else { >> schedule_work(&priv->detect_work); >> - cec_notifier_set_phys_addr(priv->cec_notify, >> - CEC_PHYS_ADDR_INVALID); >> + >> + mutex_lock(&priv->cec_notify_mutex); >> + cec_notifier_phys_addr_invalidate( >> + priv->cec_notify); >> + mutex_unlock(&priv->cec_notify_mutex); >> } >> >> handled = true; >> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) >> >> i2c_unregister_device(priv->cec); >> >> - if (priv->cec_notify) >> - cec_notifier_put(priv->cec_notify); >> + mutex_lock(&priv->cec_notify_mutex); >> + cec_notifier_conn_unregister(priv->cec_notify); >> + priv->cec_notify = NULL; >> + mutex_unlock(&priv->cec_notify_mutex); > > By the time we get here: > > 1) The interrupt has been freed (which is a synchronous operation) > tda998x_irq_thread() can't be called and can't be running, and > therefore cec_notifier_phys_addr_invalidate() also can't be called > or be running. > 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site; > if there's any case that _might_ possibly conflict, it is that one. > 3) tda998x_destroy() and tda998x_create() can't be called concurrently > in any case; the driver model guarantees that ->probe and ->remove > for the same device are serialised. > >> } >> >> static int tda998x_create(struct device *dev) >> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) >> mutex_init(&priv->mutex); /* protect the page access */ >> mutex_init(&priv->audio_mutex); /* protect access from audio thread */ >> mutex_init(&priv->edid_mutex); >> + mutex_init(&priv->cec_notify_mutex); >> INIT_LIST_HEAD(&priv->bridge.list); >> init_waitqueue_head(&priv->edid_delay_waitq); >> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); >> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) >> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); >> } >> >> - priv->cec_notify = cec_notifier_get(dev); >> + mutex_lock(&priv->cec_notify_mutex); >> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); >> + mutex_unlock(&priv->cec_notify_mutex); > > and: > > 4) priv->cec_notify will be NULL here until such time that > cec_notifier_conn_register() has returned. If the mutex is trying > to protect something, it's very unclear what it is. > > Trying to guess what it's protecting against: > > - Is it protecting against NULL priv->cec_notify? No, because it can > be NULL just before we take the lock. > - Is it protecting the internals of cec_notifier_conn_register() > against the other calls - no, because priv->cec_notify will be NULL > until the function has finished. > - Is it protecting the write to priv->cec_notify? Maybe, but that > doesn't need any protection - architectures are single-copy atomic, > which means that a pointer is either written or it is not written. > Therefore, it will either be NULL (the state before the call is made) > or it will be set correctly (after the call has completed.) > > So, all in all, I don't see what this lock is doing, and I think it > should be removed. > > If it's necessary for a future change (which may or may not be merged) > then the lock should be part of that future change, because the change > proposed by this patch certainly does not need it. > > Thanks. > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-17 7:03 ` Hans Verkuil 0 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-17 7:03 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: linux-media, Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Laurent Pinchart On 10/16/19 6:14 PM, Russell King - ARM Linux admin wrote: > On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: >> From: Dariusz Marcinkiewicz <darekm@google.com> >> >> Use the new cec_notifier_conn_(un)register() functions to >> (un)register the notifier for the HDMI connector. >> >> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Please explain in detail what this mutex actually achieves. Dariusz, since you're the author, can you reply to Russell? If this is going to be a delaying factor, then I'll post a new version without the mutex that just replaces the cec_notifier API. Regards, Hans > >> --- >> drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c >> index 84c6d4c91c65..8262b44b776e 100644 >> --- a/drivers/gpu/drm/i2c/tda998x_drv.c >> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c >> @@ -82,6 +82,9 @@ struct tda998x_priv { >> u8 audio_port_enable[AUDIO_ROUTE_NUM]; >> struct tda9950_glue cec_glue; >> struct gpio_desc *calib; >> + >> + /* protect cec_notify */ >> + struct mutex cec_notify_mutex; >> struct cec_notifier *cec_notify; >> }; >> >> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) >> tda998x_edid_delay_start(priv); >> } else { >> schedule_work(&priv->detect_work); >> - cec_notifier_set_phys_addr(priv->cec_notify, >> - CEC_PHYS_ADDR_INVALID); >> + >> + mutex_lock(&priv->cec_notify_mutex); >> + cec_notifier_phys_addr_invalidate( >> + priv->cec_notify); >> + mutex_unlock(&priv->cec_notify_mutex); >> } >> >> handled = true; >> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) >> >> i2c_unregister_device(priv->cec); >> >> - if (priv->cec_notify) >> - cec_notifier_put(priv->cec_notify); >> + mutex_lock(&priv->cec_notify_mutex); >> + cec_notifier_conn_unregister(priv->cec_notify); >> + priv->cec_notify = NULL; >> + mutex_unlock(&priv->cec_notify_mutex); > > By the time we get here: > > 1) The interrupt has been freed (which is a synchronous operation) > tda998x_irq_thread() can't be called and can't be running, and > therefore cec_notifier_phys_addr_invalidate() also can't be called > or be running. > 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site; > if there's any case that _might_ possibly conflict, it is that one. > 3) tda998x_destroy() and tda998x_create() can't be called concurrently > in any case; the driver model guarantees that ->probe and ->remove > for the same device are serialised. > >> } >> >> static int tda998x_create(struct device *dev) >> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) >> mutex_init(&priv->mutex); /* protect the page access */ >> mutex_init(&priv->audio_mutex); /* protect access from audio thread */ >> mutex_init(&priv->edid_mutex); >> + mutex_init(&priv->cec_notify_mutex); >> INIT_LIST_HEAD(&priv->bridge.list); >> init_waitqueue_head(&priv->edid_delay_waitq); >> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); >> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) >> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); >> } >> >> - priv->cec_notify = cec_notifier_get(dev); >> + mutex_lock(&priv->cec_notify_mutex); >> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); >> + mutex_unlock(&priv->cec_notify_mutex); > > and: > > 4) priv->cec_notify will be NULL here until such time that > cec_notifier_conn_register() has returned. If the mutex is trying > to protect something, it's very unclear what it is. > > Trying to guess what it's protecting against: > > - Is it protecting against NULL priv->cec_notify? No, because it can > be NULL just before we take the lock. > - Is it protecting the internals of cec_notifier_conn_register() > against the other calls - no, because priv->cec_notify will be NULL > until the function has finished. > - Is it protecting the write to priv->cec_notify? Maybe, but that > doesn't need any protection - architectures are single-copy atomic, > which means that a pointer is either written or it is not written. > Therefore, it will either be NULL (the state before the call is made) > or it will be set correctly (after the call has completed.) > > So, all in all, I don't see what this lock is doing, and I think it > should be removed. > > If it's necessary for a future change (which may or may not be merged) > then the lock should be part of that future change, because the change > proposed by this patch certainly does not need it. > > Thanks. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register 2019-10-17 7:03 ` Hans Verkuil @ 2019-10-17 7:29 ` Hans Verkuil -1 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-17 7:29 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Dariusz Marcinkiewicz, Laurent Pinchart, dri-devel, linux-media On 10/17/19 9:03 AM, Hans Verkuil wrote: > On 10/16/19 6:14 PM, Russell King - ARM Linux admin wrote: >> On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: >>> From: Dariusz Marcinkiewicz <darekm@google.com> >>> >>> Use the new cec_notifier_conn_(un)register() functions to >>> (un)register the notifier for the HDMI connector. >>> >>> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> Please explain in detail what this mutex actually achieves. > > Dariusz, since you're the author, can you reply to Russell? > > If this is going to be a delaying factor, then I'll post a new version > without the mutex that just replaces the cec_notifier API. I decided to post a v9, moving the mutex to the second patch, which should make the first patch acceptable to everyone for v5.5. Regards, Hans > > Regards, > > Hans > >> >>> --- >>> drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- >>> 1 file changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c >>> index 84c6d4c91c65..8262b44b776e 100644 >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c >>> @@ -82,6 +82,9 @@ struct tda998x_priv { >>> u8 audio_port_enable[AUDIO_ROUTE_NUM]; >>> struct tda9950_glue cec_glue; >>> struct gpio_desc *calib; >>> + >>> + /* protect cec_notify */ >>> + struct mutex cec_notify_mutex; >>> struct cec_notifier *cec_notify; >>> }; >>> >>> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) >>> tda998x_edid_delay_start(priv); >>> } else { >>> schedule_work(&priv->detect_work); >>> - cec_notifier_set_phys_addr(priv->cec_notify, >>> - CEC_PHYS_ADDR_INVALID); >>> + >>> + mutex_lock(&priv->cec_notify_mutex); >>> + cec_notifier_phys_addr_invalidate( >>> + priv->cec_notify); >>> + mutex_unlock(&priv->cec_notify_mutex); >>> } >>> >>> handled = true; >>> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) >>> >>> i2c_unregister_device(priv->cec); >>> >>> - if (priv->cec_notify) >>> - cec_notifier_put(priv->cec_notify); >>> + mutex_lock(&priv->cec_notify_mutex); >>> + cec_notifier_conn_unregister(priv->cec_notify); >>> + priv->cec_notify = NULL; >>> + mutex_unlock(&priv->cec_notify_mutex); >> >> By the time we get here: >> >> 1) The interrupt has been freed (which is a synchronous operation) >> tda998x_irq_thread() can't be called and can't be running, and >> therefore cec_notifier_phys_addr_invalidate() also can't be called >> or be running. >> 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site; >> if there's any case that _might_ possibly conflict, it is that one. >> 3) tda998x_destroy() and tda998x_create() can't be called concurrently >> in any case; the driver model guarantees that ->probe and ->remove >> for the same device are serialised. >> >>> } >>> >>> static int tda998x_create(struct device *dev) >>> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) >>> mutex_init(&priv->mutex); /* protect the page access */ >>> mutex_init(&priv->audio_mutex); /* protect access from audio thread */ >>> mutex_init(&priv->edid_mutex); >>> + mutex_init(&priv->cec_notify_mutex); >>> INIT_LIST_HEAD(&priv->bridge.list); >>> init_waitqueue_head(&priv->edid_delay_waitq); >>> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); >>> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) >>> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); >>> } >>> >>> - priv->cec_notify = cec_notifier_get(dev); >>> + mutex_lock(&priv->cec_notify_mutex); >>> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); >>> + mutex_unlock(&priv->cec_notify_mutex); >> >> and: >> >> 4) priv->cec_notify will be NULL here until such time that >> cec_notifier_conn_register() has returned. If the mutex is trying >> to protect something, it's very unclear what it is. >> >> Trying to guess what it's protecting against: >> >> - Is it protecting against NULL priv->cec_notify? No, because it can >> be NULL just before we take the lock. >> - Is it protecting the internals of cec_notifier_conn_register() >> against the other calls - no, because priv->cec_notify will be NULL >> until the function has finished. >> - Is it protecting the write to priv->cec_notify? Maybe, but that >> doesn't need any protection - architectures are single-copy atomic, >> which means that a pointer is either written or it is not written. >> Therefore, it will either be NULL (the state before the call is made) >> or it will be set correctly (after the call has completed.) >> >> So, all in all, I don't see what this lock is doing, and I think it >> should be removed. >> >> If it's necessary for a future change (which may or may not be merged) >> then the lock should be part of that future change, because the change >> proposed by this patch certainly does not need it. >> >> Thanks. >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-17 7:29 ` Hans Verkuil 0 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-17 7:29 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: linux-media, Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Laurent Pinchart On 10/17/19 9:03 AM, Hans Verkuil wrote: > On 10/16/19 6:14 PM, Russell King - ARM Linux admin wrote: >> On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: >>> From: Dariusz Marcinkiewicz <darekm@google.com> >>> >>> Use the new cec_notifier_conn_(un)register() functions to >>> (un)register the notifier for the HDMI connector. >>> >>> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> Please explain in detail what this mutex actually achieves. > > Dariusz, since you're the author, can you reply to Russell? > > If this is going to be a delaying factor, then I'll post a new version > without the mutex that just replaces the cec_notifier API. I decided to post a v9, moving the mutex to the second patch, which should make the first patch acceptable to everyone for v5.5. Regards, Hans > > Regards, > > Hans > >> >>> --- >>> drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++++++++----- >>> 1 file changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c >>> index 84c6d4c91c65..8262b44b776e 100644 >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c >>> @@ -82,6 +82,9 @@ struct tda998x_priv { >>> u8 audio_port_enable[AUDIO_ROUTE_NUM]; >>> struct tda9950_glue cec_glue; >>> struct gpio_desc *calib; >>> + >>> + /* protect cec_notify */ >>> + struct mutex cec_notify_mutex; >>> struct cec_notifier *cec_notify; >>> }; >>> >>> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) >>> tda998x_edid_delay_start(priv); >>> } else { >>> schedule_work(&priv->detect_work); >>> - cec_notifier_set_phys_addr(priv->cec_notify, >>> - CEC_PHYS_ADDR_INVALID); >>> + >>> + mutex_lock(&priv->cec_notify_mutex); >>> + cec_notifier_phys_addr_invalidate( >>> + priv->cec_notify); >>> + mutex_unlock(&priv->cec_notify_mutex); >>> } >>> >>> handled = true; >>> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev) >>> >>> i2c_unregister_device(priv->cec); >>> >>> - if (priv->cec_notify) >>> - cec_notifier_put(priv->cec_notify); >>> + mutex_lock(&priv->cec_notify_mutex); >>> + cec_notifier_conn_unregister(priv->cec_notify); >>> + priv->cec_notify = NULL; >>> + mutex_unlock(&priv->cec_notify_mutex); >> >> By the time we get here: >> >> 1) The interrupt has been freed (which is a synchronous operation) >> tda998x_irq_thread() can't be called and can't be running, and >> therefore cec_notifier_phys_addr_invalidate() also can't be called >> or be running. >> 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site; >> if there's any case that _might_ possibly conflict, it is that one. >> 3) tda998x_destroy() and tda998x_create() can't be called concurrently >> in any case; the driver model guarantees that ->probe and ->remove >> for the same device are serialised. >> >>> } >>> >>> static int tda998x_create(struct device *dev) >>> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev) >>> mutex_init(&priv->mutex); /* protect the page access */ >>> mutex_init(&priv->audio_mutex); /* protect access from audio thread */ >>> mutex_init(&priv->edid_mutex); >>> + mutex_init(&priv->cec_notify_mutex); >>> INIT_LIST_HEAD(&priv->bridge.list); >>> init_waitqueue_head(&priv->edid_delay_waitq); >>> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); >>> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev) >>> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); >>> } >>> >>> - priv->cec_notify = cec_notifier_get(dev); >>> + mutex_lock(&priv->cec_notify_mutex); >>> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL); >>> + mutex_unlock(&priv->cec_notify_mutex); >> >> and: >> >> 4) priv->cec_notify will be NULL here until such time that >> cec_notifier_conn_register() has returned. If the mutex is trying >> to protect something, it's very unclear what it is. >> >> Trying to guess what it's protecting against: >> >> - Is it protecting against NULL priv->cec_notify? No, because it can >> be NULL just before we take the lock. >> - Is it protecting the internals of cec_notifier_conn_register() >> against the other calls - no, because priv->cec_notify will be NULL >> until the function has finished. >> - Is it protecting the write to priv->cec_notify? Maybe, but that >> doesn't need any protection - architectures are single-copy atomic, >> which means that a pointer is either written or it is not written. >> Therefore, it will either be NULL (the state before the call is made) >> or it will be set correctly (after the call has completed.) >> >> So, all in all, I don't see what this lock is doing, and I think it >> should be removed. >> >> If it's necessary for a future change (which may or may not be merged) >> then the lock should be part of that future change, because the change >> proposed by this patch certainly does not need it. >> >> Thanks. >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register 2019-10-16 16:14 ` Russell King - ARM Linux admin @ 2019-10-17 9:27 ` Dariusz Marcinkiewicz -1 siblings, 0 replies; 20+ messages in thread From: Dariusz Marcinkiewicz @ 2019-10-17 9:27 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Hans Verkuil, Laurent Pinchart, dri-devel, linux-media On Wed, Oct 16, 2019 at 6:14 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: > > From: Dariusz Marcinkiewicz <darekm@google.com> > > > > Use the new cec_notifier_conn_(un)register() functions to > > (un)register the notifier for the HDMI connector. > > > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Please explain in detail what this mutex actually achieves. > .... > So, all in all, I don't see what this lock is doing, and I think it > should be removed. > > If it's necessary for a future change (which may or may not be merged) > then the lock should be part of that future change, because the change > proposed by this patch certainly does not need it. > Yes, with the change being split into 2 patches like that, the mutex is not needed here. Thank you. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register @ 2019-10-17 9:27 ` Dariusz Marcinkiewicz 0 siblings, 0 replies; 20+ messages in thread From: Dariusz Marcinkiewicz @ 2019-10-17 9:27 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Hans Verkuil, linux-media, dri-devel, Daniel Vetter, Laurent Pinchart On Wed, Oct 16, 2019 at 6:14 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: > > From: Dariusz Marcinkiewicz <darekm@google.com> > > > > Use the new cec_notifier_conn_(un)register() functions to > > (un)register the notifier for the HDMI connector. > > > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Please explain in detail what this mutex actually achieves. > .... > So, all in all, I don't see what this lock is doing, and I think it > should be removed. > > If it's necessary for a future change (which may or may not be merged) > then the lock should be part of that future change, because the change > proposed by this patch certainly does not need it. > Yes, with the change being split into 2 patches like that, the mutex is not needed here. Thank you. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv8 2/2] drm: tda998x: set the connector info 2019-10-16 13:39 ` Hans Verkuil @ 2019-10-16 13:39 ` Hans Verkuil -1 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-16 13:39 UTC (permalink / raw) To: linux-media Cc: Russell King, dri-devel, Laurent Pinchart, Hans Verkuil, Dariusz Marcinkiewicz 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); + 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; -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv8 2/2] drm: tda998x: set the connector info @ 2019-10-16 13:39 ` Hans Verkuil 0 siblings, 0 replies; 20+ messages in thread From: Hans Verkuil @ 2019-10-16 13:39 UTC (permalink / raw) To: linux-media Cc: Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Russell King, Laurent Pinchart, Hans Verkuil 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); + 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; -- 2.23.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCHv8 2/2] drm: tda998x: set the connector info 2019-10-16 13:39 ` Hans Verkuil @ 2019-10-16 16:22 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 20+ messages in thread From: Russell King - ARM Linux admin @ 2019-10-16 16:22 UTC (permalink / raw) To: Hans Verkuil Cc: Dariusz Marcinkiewicz, Laurent Pinchart, dri-devel, linux-media 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. -- 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 2/2] drm: tda998x: set the connector info @ 2019-10-16 16:22 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 20+ messages in thread From: Russell King - ARM Linux admin @ 2019-10-16 16:22 UTC (permalink / raw) To: Hans Verkuil Cc: linux-media, Dariusz Marcinkiewicz, dri-devel, Daniel Vetter, Laurent Pinchart 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. -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 2/2] drm: tda998x: set the connector info 2019-10-16 16:22 ` Russell King - ARM Linux admin @ 2019-10-17 9:26 ` Dariusz Marcinkiewicz -1 siblings, 0 replies; 20+ messages in thread From: Dariusz Marcinkiewicz @ 2019-10-17 9:26 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Hans Verkuil, Laurent Pinchart, dri-devel, linux-media 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! _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 2/2] drm: tda998x: set the connector info @ 2019-10-17 9:26 ` Dariusz Marcinkiewicz 0 siblings, 0 replies; 20+ messages in thread From: Dariusz Marcinkiewicz @ 2019-10-17 9:26 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Hans Verkuil, linux-media, dri-devel, Daniel Vetter, Laurent Pinchart 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! ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 2/2] drm: tda998x: set the connector info 2019-10-17 9:26 ` Dariusz Marcinkiewicz @ 2019-10-17 9:44 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 20+ messages in thread From: Russell King - ARM Linux admin @ 2019-10-17 9:44 UTC (permalink / raw) To: Dariusz Marcinkiewicz Cc: Hans Verkuil, Laurent Pinchart, dri-devel, linux-media 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. -- 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv8 2/2] drm: tda998x: set the connector info @ 2019-10-17 9:44 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 20+ messages in thread From: Russell King - ARM Linux admin @ 2019-10-17 9:44 UTC (permalink / raw) To: Dariusz Marcinkiewicz Cc: Hans Verkuil, linux-media, dri-devel, Daniel Vetter, Laurent Pinchart 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. -- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-10-17 9:44 UTC | newest] Thread overview: 20+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-16 13:39 [PATCHv8 0/2] drm: tda998x: use cec_notifier_conn_(un)register Hans Verkuil 2019-10-16 13:39 ` Hans Verkuil 2019-10-16 13:39 ` [PATCHv8 1/2] " Hans Verkuil 2019-10-16 13:39 ` Hans Verkuil 2019-10-16 16:14 ` Russell King - ARM Linux admin 2019-10-16 16:14 ` Russell King - ARM Linux admin 2019-10-17 7:03 ` Hans Verkuil 2019-10-17 7:03 ` Hans Verkuil 2019-10-17 7:29 ` Hans Verkuil 2019-10-17 7:29 ` Hans Verkuil 2019-10-17 9:27 ` Dariusz Marcinkiewicz 2019-10-17 9:27 ` Dariusz Marcinkiewicz 2019-10-16 13:39 ` [PATCHv8 2/2] drm: tda998x: set the connector info Hans Verkuil 2019-10-16 13:39 ` Hans Verkuil 2019-10-16 16:22 ` Russell King - ARM Linux admin 2019-10-16 16:22 ` Russell King - ARM Linux admin 2019-10-17 9:26 ` Dariusz Marcinkiewicz 2019-10-17 9:26 ` Dariusz Marcinkiewicz 2019-10-17 9:44 ` Russell King - ARM Linux admin 2019-10-17 9:44 ` Russell King - ARM Linux admin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.