Re: [PATCH 06/15] drm/exynos/ipp: free partially allocated resources on error

[Andrzej Hajda <a.hajda@samsung.com>]

On 08/26/2014 07:00 AM, Joonyoung Shim wrote:
> Hi Andrzej,
>
> On 08/22/2014 04:52 PM, Andrzej Hajda wrote:
>> In case of allocation errors some already allocated buffers
>> were not freed. The patch fixes it.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 68 ++++++++++++++++-----------------
>>  1 file changed, 33 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> index 060a198..728f3b9 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
>> @@ -599,6 +599,37 @@ static int ipp_set_mem_node(struct exynos_drm_ippdrv *ippdrv,
>>  	return ret;
>>  }
>>  
>> +static int ipp_put_mem_node(struct drm_device *drm_dev,
>> +		struct drm_exynos_ipp_cmd_node *c_node,
>> +		struct drm_exynos_ipp_mem_node *m_node)
>> +{
>> +	int i;
>> +
>> +	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
>> +
>> +	if (!m_node) {
>> +		DRM_ERROR("invalid dequeue node.\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
>> +
>> +	/* put gem buffer */
>> +	for_each_ipp_planar(i) {
>> +		unsigned long handle = m_node->buf_info.handles[i];
>> +		if (handle)
>> +			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>> +							c_node->filp);
>> +	}
>> +
>> +	/* conditionally remove from queue */
>> +	if (m_node->list.next)
> How about do INIT_LIST_HEAD for list in ipp_get_mem_node()?

I am not sure if it is better. For sure it adds unnecessary
initialization sequence.
Maybe adding list_is_initialized inline function to list.h would be the
best solution.

Regards
Andrzej

>
>> +		list_del(&m_node->list);
>> +	kfree(m_node);
>> +
>> +	return 0;
>> +}
>> +
>>  static struct drm_exynos_ipp_mem_node
>>  		*ipp_get_mem_node(struct drm_device *drm_dev,
>>  		struct drm_file *file,
>> @@ -634,7 +665,8 @@ static struct drm_exynos_ipp_mem_node
>>  					qbuf->handle[i], file);
>>  			if (IS_ERR(addr)) {
>>  				DRM_ERROR("failed to get addr.\n");
>> -				goto err_clear;
>> +				ipp_put_mem_node(drm_dev, c_node, m_node);
>> +				return ERR_PTR(-EFAULT);
>>  			}
>>  
>>  			buf_info->handles[i] = qbuf->handle[i];
>> @@ -649,40 +681,6 @@ static struct drm_exynos_ipp_mem_node
>>  	mutex_unlock(&c_node->mem_lock);
>>  
>>  	return m_node;
>> -
>> -err_clear:
>> -	kfree(m_node);
>> -	return ERR_PTR(-EFAULT);
>> -}
>> -
>> -static int ipp_put_mem_node(struct drm_device *drm_dev,
>> -		struct drm_exynos_ipp_cmd_node *c_node,
>> -		struct drm_exynos_ipp_mem_node *m_node)
>> -{
>> -	int i;
>> -
>> -	DRM_DEBUG_KMS("node[0x%x]\n", (int)m_node);
>> -
>> -	if (!m_node) {
>> -		DRM_ERROR("invalid dequeue node.\n");
>> -		return -EFAULT;
>> -	}
>> -
>> -	DRM_DEBUG_KMS("ops_id[%d]\n", m_node->ops_id);
>> -
>> -	/* put gem buffer */
>> -	for_each_ipp_planar(i) {
>> -		unsigned long handle = m_node->buf_info.handles[i];
>> -		if (handle)
>> -			exynos_drm_gem_put_dma_addr(drm_dev, handle,
>> -							c_node->filp);
>> -	}
>> -
>> -	/* delete list in queue */
>> -	list_del(&m_node->list);
>> -	kfree(m_node);
>> -
>> -	return 0;
>>  }
>>  
>>  static void ipp_free_event(struct drm_pending_event *event)
>>
>