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

[Hans Verkuil]

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 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.

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.
>>
>