Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1310325 > unrolled thread
| Started by | Peter Senna Tschudin <peter.senna@collabora.com> |
|---|---|
| First post | 2016-01-15 18:50 +0100 |
| Last post | 2016-01-15 19:30 +0100 |
| Articles | 7 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH V2 0/7] usb-misc: sisusbvga: cleanup and bug fix Peter Senna Tschudin <peter.senna@collabora.com> - 2016-01-15 18:50 +0100
[PATCH V2 5/7] usb-misc: sisusbvga: Remove null test before calls to kfree() Peter Senna Tschudin <peter.senna@collabora.com> - 2016-01-15 18:50 +0100
Re: [PATCH V2 0/7] usb-misc: sisusbvga: cleanup and bug fix Joe Perches <joe@perches.com> - 2016-01-15 19:00 +0100
Re: [PATCH V2 0/7] usb-misc: sisusbvga: cleanup and bug fix Jason Cooper <jason@lakedaemon.net> - 2016-01-15 19:10 +0100
Re: [PATCH V2 1/7] usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes Joe Perches <joe@perches.com> - 2016-01-15 19:00 +0100
Re: [PATCH V2 1/7] usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes "Peter Senna" <peter.senna@collabora.co.uk> - 2016-01-15 19:10 +0100
Re: Re: [PATCH V2 1/7] usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes Alan Stern <stern@rowland.harvard.edu> - 2016-01-15 19:30 +0100
| From | Peter Senna Tschudin <peter.senna@collabora.com> |
|---|---|
| Date | 2016-01-15 18:50 +0100 |
| Subject | [PATCH V2 0/7] usb-misc: sisusbvga: cleanup and bug fix |
| Message-ID | <qRgFz-W7-11@gated-at.bofh.it> |
The file drivers/usb/misc/sisusbvga/sisusb.c had many (192) coding style issues
reported by checkpatch. This file also had a problematic error path in the
probe function that could result in dereferencing a null pointer.
This patch series fix coding style issues and a problematic error path which
could result in a null pointer dereference.
Patch 1 and 2 change whitespace only, patch 3 to 6 fix various coding style
issues, and patch 7 fix a null pointer dereference bug.
Joe Perches suggested me to include objtdiff output for patches that are not
supposed to make semantic changes, but it is not working well for me with gcc
(GCC) 5.3.1 20151207 (Red Hat 5.3.1-2). Even compiling the same source code
produces different output from objdump. The objdump command I'm using is from
./scripts/objdiff. See an example:
# A patch that should not make any semantic change
$ cat /tmp/patch
diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index 8efbaba..a48b086d 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -1353,7 +1353,7 @@ sisusb_testreadwrite(struct sisusb_usb_data *sisusb)
static char srcbuffer[] = { 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77 };
char destbuffer[10];
size_t dummy;
- int i,j;
+ int i, j;
sisusb_copy_memory(sisusb, srcbuffer, sisusb->vrambase, 7, &dummy);
# Lets compile and collect a dump based on linux-next/master
$ git checkout linux-next/master
$ md5sum drivers/usb/misc/sisusbvga/sisusb.c
d6ffbd44f3f1cf81fd55ce84441ab889 drivers/usb/misc/sisusbvga/sisusb.c
$ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
$ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump
# Now let's apply the patch and collect other dump
$ git apply /tmp/patch
$ md5sum drivers/usb/misc/sisusbvga/sisusb.c
0b7d579c8ae2159f677c6a5c6efc4956 drivers/usb/misc/sisusbvga/sisusb.c
$ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
$ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/dump
# I was expecting the diff to be empty
$ diff /tmp/base_dump /tmp/dump
9135,9136c9135
< : 8e 4d 31 mov 0x31(%rbp),%cs
< : 46 00 00 rex.RX add %r8b,(%rax)
---
> : 25 c4 31 46 00 and $0x4631c4,%eax
9139c9138
< : 4b 00 00 rex.WXB add %al,(%r8)
---
> : 00 4b 00 add %cl,0x0(%rbx)
# But here is the interesting part. Even compiling the exact same source code
# produces different results
$ git checkout -- .
$ md5sum drivers/usb/misc/sisusbvga/sisusb.c
d6ffbd44f3f1cf81fd55ce84441ab889 drivers/usb/misc/sisusbvga/sisusb.c
$ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
$ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump_again
$ diff /tmp/base_dump /tmp/base_dump_again
9135,9136c9135,9136
< : 8e 4d 31 mov 0x31(%rbp),%cs
< : 46 00 00 rex.RX add %r8b,(%rax)
---
> : de 10 ficom (%rax)
> : 33 46 00 xor 0x0(%rsi),%eax
9139c9139
< : 4b 00 00 rex.WXB add %al,(%r8)
---
> : 00 4b 00 add %cl,0x0(%rbx)
Peter Senna Tschudin (7):
usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes
usb-misc: sisusbvga: Fix coding style: vertical whitespace changes
usb-misc: sisusbvga: Fix coding style: braces, parenthesis, comment
usb-misc: sisusbvga: Fix coding style: remove assignment from if tests
usb-misc: sisusbvga: Remove null test before calls to kfree()
usb-misc: sisusbvga: Remove memory allocation logs
usb-misc: sisusbvga: fix error path
drivers/usb/misc/sisusbvga/sisusb.c | 1543 +++++++++++++++++------------------
1 file changed, 752 insertions(+), 791 deletions(-)
--
2.5.0
[toc] | [next] | [standalone]
| From | Peter Senna Tschudin <peter.senna@collabora.com> |
|---|---|
| Date | 2016-01-15 18:50 +0100 |
| Subject | [PATCH V2 5/7] usb-misc: sisusbvga: Remove null test before calls to kfree() |
| Message-ID | <qRgFC-W7-49@gated-at.bofh.it> |
| In reply to | #1310325 |
From: Peter Senna Tschudin <peter.senna@gmail.com>
This patch removes null test before calls to kfree() as kfree() can
handle null pointers safely.
Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
---
No changes from V1.
Tested by compilation only.
drivers/usb/misc/sisusbvga/sisusb.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index 40f360a..bea6d88 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -76,15 +76,11 @@ static void sisusb_free_buffers(struct sisusb_usb_data *sisusb)
int i;
for (i = 0; i < NUMOBUFS; i++) {
- if (sisusb->obuf[i]) {
- kfree(sisusb->obuf[i]);
- sisusb->obuf[i] = NULL;
- }
- }
- if (sisusb->ibuf) {
- kfree(sisusb->ibuf);
- sisusb->ibuf = NULL;
+ kfree(sisusb->obuf[i]);
+ sisusb->obuf[i] = NULL;
}
+ kfree(sisusb->ibuf);
+ sisusb->ibuf = NULL;
}
static void sisusb_free_urbs(struct sisusb_usb_data *sisusb)
--
2.5.0
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-01-15 19:00 +0100 |
| Message-ID | <qRgPg-ZC-1@gated-at.bofh.it> |
| In reply to | #1310325 |
(cc'ing Masahiro Yamada and Jason Cooper for objdiff)
On Fri, 2016-01-15 at 18:41 +0100, Peter Senna Tschudin wrote:
> The file drivers/usb/misc/sisusbvga/sisusb.c had many (192) coding style issues
> reported by checkpatch. This file also had a problematic error path in the
> probe function that could result in dereferencing a null pointer.
>
> This patch series fix coding style issues and a problematic error path which
> could result in a null pointer dereference.
>
> Patch 1 and 2 change whitespace only, patch 3 to 6 fix various coding style
> issues, and patch 7 fix a null pointer dereference bug.
>
> Joe Perches suggested me to include objtdiff output for patches that are not
> supposed to make semantic changes, but it is not working well for me with gcc
> (GCC) 5.3.1 20151207 (Red Hat 5.3.1-2). Even compiling the same source code
> produces different output from objdump. The objdump command I'm using is from
> ./scripts/objdiff. See an example:
It seems gcc 5.3 isn't producing consistent output
for whitespace changes.
As far as I know, gcc has never made guarantees about
object output for the same input content.
> # A patch that should not make any semantic change
> $ cat /tmp/patch
> diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
> index 8efbaba..a48b086d 100644
> --- a/drivers/usb/misc/sisusbvga/sisusb.c
> +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> @@ -1353,7 +1353,7 @@ sisusb_testreadwrite(struct sisusb_usb_data *sisusb)
> static char srcbuffer[] = { 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77 };
> char destbuffer[10];
> size_t dummy;
> - int i,j;
> + int i, j;
>
> sisusb_copy_memory(sisusb, srcbuffer, sisusb->vrambase, 7, &dummy);
>
> # Lets compile and collect a dump based on linux-next/master
> $ git checkout linux-next/master
> $ md5sum drivers/usb/misc/sisusbvga/sisusb.c
> d6ffbd44f3f1cf81fd55ce84441ab889 drivers/usb/misc/sisusbvga/sisusb.c
>
> $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
> $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
> sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump
>
> # Now let's apply the patch and collect other dump
> $ git apply /tmp/patch
> $ md5sum drivers/usb/misc/sisusbvga/sisusb.c
> 0b7d579c8ae2159f677c6a5c6efc4956 drivers/usb/misc/sisusbvga/sisusb.c
>
> $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
> $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
> sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/dump
>
> # I was expecting the diff to be empty
> $ diff /tmp/base_dump /tmp/dump
> 9135,9136c9135
> < : 8e 4d 31 mov 0x31(%rbp),%cs
> < : 46 00 00 rex.RX add %r8b,(%rax)
> ---
> > : 25 c4 31 46 00 and $0x4631c4,%eax
> 9139c9138
> < : 4b 00 00 rex.WXB add %al,(%r8)
> ---
> > : 00 4b 00 add %cl,0x0(%rbx)
>
> # But here is the interesting part. Even compiling the exact same source code
> # produces different results
> $ git checkout -- .
> $ md5sum drivers/usb/misc/sisusbvga/sisusb.c
> d6ffbd44f3f1cf81fd55ce84441ab889 drivers/usb/misc/sisusbvga/sisusb.c
>
> $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
> $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
> sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump_again
>
> $ diff /tmp/base_dump /tmp/base_dump_again
> 9135,9136c9135,9136
> < : 8e 4d 31 mov 0x31(%rbp),%cs
> < : 46 00 00 rex.RX add %r8b,(%rax)
> ---
> > : de 10 ficom (%rax)
> > : 33 46 00 xor 0x0(%rsi),%eax
> 9139c9139
> < : 4b 00 00 rex.WXB add %al,(%r8)
> ---
> > : 00 4b 00 add %cl,0x0(%rbx)
>
> Peter Senna Tschudin (7):
> usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes
> usb-misc: sisusbvga: Fix coding style: vertical whitespace changes
> usb-misc: sisusbvga: Fix coding style: braces, parenthesis, comment
> usb-misc: sisusbvga: Fix coding style: remove assignment from if tests
> usb-misc: sisusbvga: Remove null test before calls to kfree()
> usb-misc: sisusbvga: Remove memory allocation logs
> usb-misc: sisusbvga: fix error path
>
> drivers/usb/misc/sisusbvga/sisusb.c | 1543 +++++++++++++++++------------------
> 1 file changed, 752 insertions(+), 791 deletions(-)
>
[toc] | [prev] | [next] | [standalone]
| From | Jason Cooper <jason@lakedaemon.net> |
|---|---|
| Date | 2016-01-15 19:10 +0100 |
| Message-ID | <qRgYX-1jy-31@gated-at.bofh.it> |
| In reply to | #1310327 |
Hey Joe, Peter, On Fri, Jan 15, 2016 at 09:51:49AM -0800, Joe Perches wrote: > (cc'ing Masahiro Yamada and Jason Cooper for objdiff) > > On Fri, 2016-01-15 at 18:41 +0100, Peter Senna Tschudin wrote: ... > As far as I know, gcc has never made guarantees about > object output for the same input content. True, we may want to reach out to the folks doing reproducible builds. They may have seen/diagnosed/fixed this already. > > # But here is the interesting part. Even compiling the exact same source code > > # produces different results > > $ git checkout -- . > > $ md5sum drivers/usb/misc/sisusbvga/sisusb.c > > d6ffbd44f3f1cf81fd55ce84441ab889 drivers/usb/misc/sisusbvga/sisusb.c > > > > $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o > > $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \ > > sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump_again > > > > $ diff /tmp/base_dump /tmp/base_dump_again > > 9135,9136c9135,9136 > > < : 8e 4d 31 mov 0x31(%rbp),%cs > > < : 46 00 00 rex.RX add %r8b,(%rax) > > --- > > > : de 10 ficom (%rax) > > > : 33 46 00 xor 0x0(%rsi),%eax > > 9139c9139 > > < : 4b 00 00 rex.WXB add %al,(%r8) > > --- > > > : 00 4b 00 add %cl,0x0(%rbx) Well, this clearly shows that the whitespace change is a red herring. I'm just guessing here, but could you try with -j1 to see if you can still reproduce this inconsistency? Cleaning the tree between builds may also narrow down the suspects. thx, Jason.
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-01-15 19:00 +0100 |
| Subject | Re: [PATCH V2 1/7] usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes |
| Message-ID | <qRgPh-ZC-7@gated-at.bofh.it> |
| In reply to | #1310325 |
On Fri, 2016-01-15 at 18:41 +0100, Peter Senna Tschudin wrote: > This patch fixes whitespace coding style issues that can be fixed > within a single line. trivia: > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c [] > @@ -549,7 +549,7 @@ static int sisusb_recv_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len, > } > > static int sisusb_send_packet(struct sisusb_usb_data *sisusb, int len, > - struct sisusb_packet *packet) > + struct sisusb_packet *packet) I think all of these should use indentation alignment to open parenthesis.
[toc] | [prev] | [next] | [standalone]
| From | "Peter Senna" <peter.senna@collabora.co.uk> |
|---|---|
| Date | 2016-01-15 19:10 +0100 |
| Subject | Re: [PATCH V2 1/7] usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes |
| Message-ID | <qRgYX-1jy-19@gated-at.bofh.it> |
| In reply to | #1310330 |
CC: Alan Stern <stern@rowland.harvard.edu> On Friday, January 15, 2016 18:56 CET, Joe Perches <joe@perches.com> wrote: > On Fri, 2016-01-15 at 18:41 +0100, Peter Senna Tschudin wrote: > > This patch fixes whitespace coding style issues that can be fixed > > within a single line. > > trivia: > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c > [] > > @@ -549,7 +549,7 @@ static int sisusb_recv_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len, > > } > > > > static int sisusb_send_packet(struct sisusb_usb_data *sisusb, int len, > > - struct sisusb_packet *packet) > > + struct sisusb_packet *packet) > > I think all of these should use indentation alignment > to open parenthesis. That was my original idea too, but I have the impression things are different for usb, at least for usb-host. Alan, can you comment on this continuation line style issue here?
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-01-15 19:30 +0100 |
| Subject | Re: Re: [PATCH V2 1/7] usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes |
| Message-ID | <qRhih-1sI-3@gated-at.bofh.it> |
| In reply to | #1310344 |
On Fri, 15 Jan 2016, Peter Senna wrote: > CC: Alan Stern <stern@rowland.harvard.edu> > > On Friday, January 15, 2016 18:56 CET, Joe Perches <joe@perches.com> wrote: > > > On Fri, 2016-01-15 at 18:41 +0100, Peter Senna Tschudin wrote: > > > This patch fixes whitespace coding style issues that can be fixed > > > within a single line. > > > > trivia: > > > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c > > [] > > > @@ -549,7 +549,7 @@ static int sisusb_recv_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len, > > > } > > > > > > static int sisusb_send_packet(struct sisusb_usb_data *sisusb, int len, > > > - struct sisusb_packet *packet) > > > + struct sisusb_packet *packet) > > > > I think all of these should use indentation alignment > > to open parenthesis. > That was my original idea too, but I have the impression things are different for usb, at least for usb-host. Alan, can you comment on this continuation line style issue here? Different files use different alignment for continuation lines. I use the style I like (two extra tabs) for the code I write, but other people make different choices. Unless I happen to be the maintainer or an active contributor for the source file in question, my preferences don't matter much. Alan Stern
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web