[Nouveau] [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function

[Hans Verkuil]

On 08/20/2018 08:51 PM, Lyude Paul wrote:
> On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil at cisco.com>
>>
>> If aux->transfer == NULL, then just return without doing
>> anything. In that case the function is likely called for
>> a non-(e)DP connector.
>>
>> This never happened for the i915 driver, but the nouveau and amdgpu
>> drivers need this check.
> Could you give a backtrace from where you're hitting this issue with nouveau and
> amdgpu? It doesn't make a whole ton of sense to have connectors registering DP
> aux busses if they aren't actually DP, that should probably just be fixed...

The difference between the i915 driver and the nouveau (and amdgpu) driver is
that in the i915 driver the drm_dp_cec_set_edid/unset_edid/irq functions are
called from code that is exclusively for DisplayPort connectors.

For nouveau/amdgpu they are called from code that is shared between DisplayPort
and HDMI, so aux->transfer may be NULL.

Rather than either testing for the connector type or for a non-NULL aux->transfer
every time I call a drm_dp_cec_* function, it is better to just test for
aux->transfer in the drm_dp_cec_* functions themselves. It's more robust.

So there isn't a bug or anything like that, it's just so that these drm_dp_cec
functions can handle a slightly different driver design safely.

The registration and unregistration of the cec devices is always DP specific,
and an attempt to register a cec device for a non-DP connector will now fail
with a WARN_ON.

Regards,

	Hans

> 
>>
>> The alternative would be to add this check in those drivers before
>> every drm_dp_cec call, but it makes sense to check it in the
>> drm_dp_cec functions to prevent a kernel oops.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil at cisco.com>
>> ---
>>  drivers/gpu/drm/drm_dp_cec.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
>> index 988513346e9c..1407b13a8d5d 100644
>> --- a/drivers/gpu/drm/drm_dp_cec.c
>> +++ b/drivers/gpu/drm/drm_dp_cec.c
>> @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
>>  	u8 cec_irq;
>>  	int ret;
>>  
>> +	/* No transfer function was set, so not a DP connector */
>> +	if (!aux->transfer)
>> +		return;
>> +
>>  	mutex_lock(&aux->cec.lock);
>>  	if (!aux->cec.adap)
>>  		goto unlock;
>> @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
>> struct edid *edid)
>>  	unsigned int num_las = 1;
>>  	u8 cap;
>>  
>> +	/* No transfer function was set, so not a DP connector */
>> +	if (!aux->transfer)
>> +		return;
>> +
>>  #ifndef CONFIG_MEDIA_CEC_RC
>>  	/*
>>  	 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
>> @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
>>   */
>>  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
>>  {
>> +	/* No transfer function was set, so not a DP connector */
>> +	if (!aux->transfer)
>> +		return;
>> +
>>  	cancel_delayed_work_sync(&aux->cec.unregister_work);
>>  
>>  	mutex_lock(&aux->cec.lock);
>> @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
>> const char *name,
>>  				   struct device *parent)
>>  {
>>  	WARN_ON(aux->cec.adap);
>> +	if (WARN_ON(!aux->transfer))
>> +		return;
>>  	aux->cec.name = name;
>>  	aux->cec.parent = parent;
>>  	INIT_DELAYED_WORK(&aux->cec.unregister_work,
>