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@xxxxxxxxxx>
>>
>> Use the new cec_notifier_conn_(un)register() functions to
>> (un)register the notifier for the HDMI connector.
>>
>> Signed-off-by: Dariusz Marcinkiewicz <darekm@xxxxxxxxxx>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
