| Message ID | 1408693946-15456-7-git-send-email-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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()? > + 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) >> > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 08/27/2014 07:27 PM, Andrzej Hajda wrote: > 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. In other words, this NULL checking is unnecessary if you initialize the list_head by INIT_LIST_HEAD. > Maybe adding list_is_initialized inline function to list.h would be the > best solution. There is just list_empty and we can't know whether list is initialized or not. I recommend to use initialized list_head. Thanks. > > 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) >>> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) + 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)
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(-)