Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1465895 > unrolled thread
| Started by | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| First post | 2016-08-19 04:20 +0200 |
| Last post | 2016-08-19 09:50 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 0/2] GPU-DRM-Savage: Fine-tuning for savage_bci_cmdbuf() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-19 04:20 +0200
[PATCH 2/2] GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-19 04:30 +0200
Re: [PATCH 2/2] GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection Daniel Vetter <daniel@ffwll.ch> - 2016-08-19 10:00 +0200
[PATCH 1/2] GPU-DRM-Savage: Use memdup_user() rather than duplicating SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-19 05:00 +0200
Re: [PATCH 0/2] GPU-DRM-Savage: Fine-tuning for savage_bci_cmdbuf() Daniel Vetter <daniel@ffwll.ch> - 2016-08-19 09:50 +0200
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-08-19 04:20 +0200 |
| Subject | [PATCH 0/2] GPU-DRM-Savage: Fine-tuning for savage_bci_cmdbuf() |
| Message-ID | <s7Hzz-817-1@gated-at.bofh.it> |
From: Markus Elfring <elfring@users.sourceforge.net> Date: Thu, 18 Aug 2016 21:38:37 +0200 A few update suggestions were taken into account from static source code analysis. Markus Elfring (2): Use memdup_user() rather than duplicating its implementation Less function calls after error detection drivers/gpu/drm/savage/savage_state.c | 42 +++++++++++++++-------------------- 1 file changed, 18 insertions(+), 24 deletions(-) -- 2.9.3
[toc] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-08-19 04:30 +0200 |
| Subject | [PATCH 2/2] GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection |
| Message-ID | <s7HJg-84B-13@gated-at.bofh.it> |
| In reply to | #1465895 |
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 21:28:58 +0200
The kfree() function was called in a few cases by the
savage_bci_cmdbuf() function during error handling
even if a passed variable contained a null pointer.
Adjust jump targets according to the Linux coding style convention.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/gpu/drm/savage/savage_state.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/savage/savage_state.c b/drivers/gpu/drm/savage/savage_state.c
index 3dc0d8f..5b484aa 100644
--- a/drivers/gpu/drm/savage/savage_state.c
+++ b/drivers/gpu/drm/savage/savage_state.c
@@ -1004,7 +1004,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
kvb_addr = memdup_user(cmdbuf->vb_addr, cmdbuf->vb_size);
if (IS_ERR(kvb_addr)) {
ret = PTR_ERR(kvb_addr);
- goto done;
+ goto free_cmd;
}
cmdbuf->vb_addr = kvb_addr;
}
@@ -1013,13 +1013,13 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
GFP_KERNEL);
if (kbox_addr == NULL) {
ret = -ENOMEM;
- goto done;
+ goto free_vb;
}
if (copy_from_user(kbox_addr, cmdbuf->box_addr,
cmdbuf->nbox * sizeof(struct drm_clip_rect))) {
ret = -EFAULT;
- goto done;
+ goto free_vb;
}
cmdbuf->box_addr = kbox_addr;
}
@@ -1052,7 +1052,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
"beyond end of command buffer\n");
DMA_FLUSH();
ret = -EINVAL;
- goto done;
+ goto free_box;
}
/* fall through */
case SAVAGE_CMD_DMA_PRIM:
@@ -1071,7 +1071,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
cmdbuf->vb_stride,
cmdbuf->nbox, cmdbuf->box_addr);
if (ret != 0)
- goto done;
+ goto free_box;
first_draw_cmd = NULL;
}
}
@@ -1086,7 +1086,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
"beyond end of command buffer\n");
DMA_FLUSH();
ret = -EINVAL;
- goto done;
+ goto free_box;
}
ret = savage_dispatch_state(dev_priv, &cmd_header,
(const uint32_t *)cmdbuf->cmd_addr);
@@ -1099,7 +1099,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
"beyond end of command buffer\n");
DMA_FLUSH();
ret = -EINVAL;
- goto done;
+ goto free_box;
}
ret = savage_dispatch_clear(dev_priv, &cmd_header,
cmdbuf->cmd_addr,
@@ -1117,12 +1117,12 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
cmd_header.cmd.cmd);
DMA_FLUSH();
ret = -EINVAL;
- goto done;
+ goto free_box;
}
if (ret != 0) {
DMA_FLUSH();
- goto done;
+ goto free_box;
}
}
@@ -1133,7 +1133,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
cmdbuf->nbox, cmdbuf->box_addr);
if (ret != 0) {
DMA_FLUSH();
- goto done;
+ goto free_box;
}
}
@@ -1147,11 +1147,11 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
savage_freelist_put(dev, dmabuf);
}
-done:
- /* If we didn't need to allocate them, these'll be NULL */
- kfree(kcmd_addr);
- kfree(kvb_addr);
+free_box:
kfree(kbox_addr);
-
+free_vb:
+ kfree(kvb_addr);
+free_cmd:
+ kfree(kcmd_addr);
return ret;
}
--
2.9.3
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-19 10:00 +0200 |
| Subject | Re: [PATCH 2/2] GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection |
| Message-ID | <s7MSC-2Nt-11@gated-at.bofh.it> |
| In reply to | #1465909 |
On Thu, Aug 18, 2016 at 09:48:04PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 18 Aug 2016 21:28:58 +0200
>
> The kfree() function was called in a few cases by the
> savage_bci_cmdbuf() function during error handling
> even if a passed variable contained a null pointer.
>
> Adjust jump targets according to the Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Not sure this is worth it, I'll pass. Patch 1 merged. Btw I consider
cocci patches a good way to get started somewhere, but then it's much more
useful to do a bit more involved things. We keep a list of small&big
janitor tasks:
https://www.x.org/wiki/DRMJanitors/
Cleaning up all the cocci errors in drm isn't good since then the next
person won't have something easy to get started, i.e. consider you're
budget used up ;-)
-Daniel
> ---
> drivers/gpu/drm/savage/savage_state.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/savage/savage_state.c b/drivers/gpu/drm/savage/savage_state.c
> index 3dc0d8f..5b484aa 100644
> --- a/drivers/gpu/drm/savage/savage_state.c
> +++ b/drivers/gpu/drm/savage/savage_state.c
> @@ -1004,7 +1004,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> kvb_addr = memdup_user(cmdbuf->vb_addr, cmdbuf->vb_size);
> if (IS_ERR(kvb_addr)) {
> ret = PTR_ERR(kvb_addr);
> - goto done;
> + goto free_cmd;
> }
> cmdbuf->vb_addr = kvb_addr;
> }
> @@ -1013,13 +1013,13 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> GFP_KERNEL);
> if (kbox_addr == NULL) {
> ret = -ENOMEM;
> - goto done;
> + goto free_vb;
> }
>
> if (copy_from_user(kbox_addr, cmdbuf->box_addr,
> cmdbuf->nbox * sizeof(struct drm_clip_rect))) {
> ret = -EFAULT;
> - goto done;
> + goto free_vb;
> }
> cmdbuf->box_addr = kbox_addr;
> }
> @@ -1052,7 +1052,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> "beyond end of command buffer\n");
> DMA_FLUSH();
> ret = -EINVAL;
> - goto done;
> + goto free_box;
> }
> /* fall through */
> case SAVAGE_CMD_DMA_PRIM:
> @@ -1071,7 +1071,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> cmdbuf->vb_stride,
> cmdbuf->nbox, cmdbuf->box_addr);
> if (ret != 0)
> - goto done;
> + goto free_box;
> first_draw_cmd = NULL;
> }
> }
> @@ -1086,7 +1086,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> "beyond end of command buffer\n");
> DMA_FLUSH();
> ret = -EINVAL;
> - goto done;
> + goto free_box;
> }
> ret = savage_dispatch_state(dev_priv, &cmd_header,
> (const uint32_t *)cmdbuf->cmd_addr);
> @@ -1099,7 +1099,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> "beyond end of command buffer\n");
> DMA_FLUSH();
> ret = -EINVAL;
> - goto done;
> + goto free_box;
> }
> ret = savage_dispatch_clear(dev_priv, &cmd_header,
> cmdbuf->cmd_addr,
> @@ -1117,12 +1117,12 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> cmd_header.cmd.cmd);
> DMA_FLUSH();
> ret = -EINVAL;
> - goto done;
> + goto free_box;
> }
>
> if (ret != 0) {
> DMA_FLUSH();
> - goto done;
> + goto free_box;
> }
> }
>
> @@ -1133,7 +1133,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> cmdbuf->nbox, cmdbuf->box_addr);
> if (ret != 0) {
> DMA_FLUSH();
> - goto done;
> + goto free_box;
> }
> }
>
> @@ -1147,11 +1147,11 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
> savage_freelist_put(dev, dmabuf);
> }
>
> -done:
> - /* If we didn't need to allocate them, these'll be NULL */
> - kfree(kcmd_addr);
> - kfree(kvb_addr);
> +free_box:
> kfree(kbox_addr);
> -
> +free_vb:
> + kfree(kvb_addr);
> +free_cmd:
> + kfree(kcmd_addr);
> return ret;
> }
> --
> 2.9.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-08-19 05:00 +0200 |
| Subject | [PATCH 1/2] GPU-DRM-Savage: Use memdup_user() rather than duplicating |
| Message-ID | <s7Ich-8fA-7@gated-at.bofh.it> |
| In reply to | #1465895 |
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 18:12:03 +0200
Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/gpu/drm/savage/savage_state.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/savage/savage_state.c b/drivers/gpu/drm/savage/savage_state.c
index c01ad0a..3dc0d8f 100644
--- a/drivers/gpu/drm/savage/savage_state.c
+++ b/drivers/gpu/drm/savage/savage_state.c
@@ -1001,15 +1001,9 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
cmdbuf->cmd_addr = kcmd_addr;
}
if (cmdbuf->vb_size) {
- kvb_addr = kmalloc(cmdbuf->vb_size, GFP_KERNEL);
- if (kvb_addr == NULL) {
- ret = -ENOMEM;
- goto done;
- }
-
- if (copy_from_user(kvb_addr, cmdbuf->vb_addr,
- cmdbuf->vb_size)) {
- ret = -EFAULT;
+ kvb_addr = memdup_user(cmdbuf->vb_addr, cmdbuf->vb_size);
+ if (IS_ERR(kvb_addr)) {
+ ret = PTR_ERR(kvb_addr);
goto done;
}
cmdbuf->vb_addr = kvb_addr;
--
2.9.3
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-19 09:50 +0200 |
| Message-ID | <s7MIV-2K9-5@gated-at.bofh.it> |
| In reply to | #1465895 |
On Thu, Aug 18, 2016 at 09:42:33PM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Thu, 18 Aug 2016 21:38:37 +0200 > > A few update suggestions were taken into account > from static source code analysis. savage is one of the dri1 legacy drivers, imo not really worth it to spend time on them. otoh no one will notice any breakage either ;-) I guess I'll apply. -Daniel > > Markus Elfring (2): > Use memdup_user() rather than duplicating its implementation > Less function calls after error detection > > drivers/gpu/drm/savage/savage_state.c | 42 +++++++++++++++-------------------- > 1 file changed, 18 insertions(+), 24 deletions(-) > > -- > 2.9.3 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web