[PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register

[Hans Verkuil]

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 at 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 at google.com>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco at 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.
>