Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301711 > unrolled thread

[PATCH 0/5] usb-misc: cleanup sisusbvga

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] usb-misc: cleanup sisusbvga Peter Senna Tschudin <peter.senna@collabora.com> - 2016-01-05 18:00 +0100
    [PATCH 5/5] usb-misc: sisusbvga: Remove null test before kfree() Peter Senna Tschudin <peter.senna@collabora.com> - 2016-01-05 18:00 +0100
    [PATCH 3/5] usb-misc: sisusbvga: Remove assignments from if tests Peter Senna Tschudin <peter.senna@collabora.com> - 2016-01-05 18:00 +0100
    Re: [PATCH 0/5] usb-misc: cleanup sisusbvga Joe Perches <joe@perches.com> - 2016-01-05 20:00 +0100
      Re: [PATCH 0/5] usb-misc: cleanup sisusbvga Peter Senna Tschudin <peter.senna@collabora.com> - 2016-01-06 16:40 +0100
        Re: [PATCH 0/5] usb-misc: cleanup sisusbvga Joe Perches <joe@perches.com> - 2016-01-06 17:10 +0100

#1301711 — [PATCH 0/5] usb-misc: cleanup sisusbvga

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 fix the problematic error path to
avoid the null pointer dereference.

Patch 1 is the biggest and fix only whitespace, tab and newline issues. I used

$ git diff -w --word-diff=porcelain drivers/usb/misc/sisusbvga/sisusb.c

to verify that this patch do not change any visible character. If the size of
this patch is a problem, please let me know in how many patches to split it.

Patch 2 follows fixing trivial coding style, mostly related to braces,
and parenthesis.

Patch 3 remove assignments from if tests.

Patch 4 remove calls to dev_err() on memory allocation failures and fix one
error path to avoid dereferencing a null pointer. The patch change the
problematic error path to cleanup previously allocated resources and abort the
probe with -ENOMEM instead of only calling dev_err() and continue with the
probe.

Patch 5 remove null tests before calls to kfree().

Peter Senna Tschudin (5):
  usb-misc: sisusbvga: Fix coding style: white space
  usb-misc: sisusbvga: Fix coding style: braces, parenthesis, comment
  usb-misc: sisusbvga: Fix coding style: remove assignment from if tests
  usb-misc: sisusbvga: Remove memory allocation logs and fix error path
  usb-misc: sisusbvga: Remove null test before calls to kfree()

 drivers/usb/misc/sisusbvga/sisusb.c | 1543 +++++++++++++++++------------------
 1 file changed, 752 insertions(+), 791 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301712 — [PATCH 5/5] usb-misc: sisusbvga: Remove null test before kfree()

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>
---
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 875e365..8d0b29a 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301715 — [PATCH 3/5] usb-misc: sisusbvga: Remove assignments from if tests

From: Peter Senna Tschudin <peter.senna@gmail.com>

The file drivers/usb/misc/sisusbvga/sisusb.c had 6 assignments inside if
tests. This patch move the assignement outside the test.

Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
---
Tested by compilation only.

 drivers/usb/misc/sisusbvga/sisusb.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index e6bb1ad..6cce4fb 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -1378,7 +1378,8 @@ static int sisusb_clear_vram(struct sisusb_usb_data *sisusb,
 		return 0;
 
 	/* allocate free buffer/urb and clear the buffer */
-	if ((i = sisusb_alloc_outbuf(sisusb)) < 0)
+	i = sisusb_alloc_outbuf(sisusb);
+	if (i < 0)
 		return -EBUSY;
 
 	memset(sisusb->obuf[i], 0, sisusb->obufsize);
@@ -3067,7 +3068,8 @@ static int sisusb_probe(struct usb_interface *intf,
 
 	/* Allocate buffers */
 	sisusb->ibufsize = SISUSB_IBUF_SIZE;
-	if (!(sisusb->ibuf = kmalloc(SISUSB_IBUF_SIZE, GFP_KERNEL))) {
+	sisusb->ibuf = kmalloc(SISUSB_IBUF_SIZE, GFP_KERNEL);
+	if (!sisusb->ibuf) {
 		dev_err(&sisusb->sisusb_dev->dev, "Failed to allocate memory for input buffer");
 		retval = -ENOMEM;
 		goto error_2;
@@ -3076,7 +3078,8 @@ static int sisusb_probe(struct usb_interface *intf,
 	sisusb->numobufs = 0;
 	sisusb->obufsize = SISUSB_OBUF_SIZE;
 	for (i = 0; i < NUMOBUFS; i++) {
-		if (!(sisusb->obuf[i] = kmalloc(SISUSB_OBUF_SIZE, GFP_KERNEL))) {
+		sisusb->obuf[i] = kmalloc(SISUSB_OBUF_SIZE, GFP_KERNEL);
+		if (!sisusb->obuf[i]) {
 			if (i == 0) {
 				dev_err(&sisusb->sisusb_dev->dev, "Failed to allocate memory for output buffer\n");
 				retval = -ENOMEM;
@@ -3088,7 +3091,8 @@ static int sisusb_probe(struct usb_interface *intf,
 	}
 
 	/* Allocate URBs */
-	if (!(sisusb->sisurbin = usb_alloc_urb(0, GFP_KERNEL))) {
+	sisusb->sisurbin = usb_alloc_urb(0, GFP_KERNEL);
+	if (!sisusb->sisurbin) {
 		dev_err(&sisusb->sisusb_dev->dev, "Failed to allocate URBs\n");
 		retval = -ENOMEM;
 		goto error_3;
@@ -3096,7 +3100,8 @@ static int sisusb_probe(struct usb_interface *intf,
 	sisusb->completein = 1;
 
 	for (i = 0; i < sisusb->numobufs; i++) {
-		if (!(sisusb->sisurbout[i] = usb_alloc_urb(0, GFP_KERNEL))) {
+		sisusb->sisurbout[i] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!sisusb->sisurbout[i]) {
 			dev_err(&sisusb->sisusb_dev->dev,
 					"Failed to allocate URBs\n");
 			retval = -ENOMEM;
@@ -3112,7 +3117,8 @@ static int sisusb_probe(struct usb_interface *intf,
 
 #ifdef INCL_SISUSB_CON
 	/* Allocate our SiS_Pr */
-	if (!(sisusb->SiS_Pr = kmalloc(sizeof(struct SiS_Private), GFP_KERNEL))) {
+	sisusb->SiS_Pr = kmalloc(sizeof(struct SiS_Private), GFP_KERNEL);
+	if (!sisusb->SiS_Pr) {
 		dev_err(&sisusb->sisusb_dev->dev, "Failed to allocate SiS_Pr\n");
 	}
 #endif
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301871

On Tue, 2016-01-05 at 17:54 +0100, Peter Senna Tschudin wrote:
[]
> Patch 1 is the biggest and fix only whitespace, tab and newline issues. I used
> 
> $ git diff -w --word-diff=porcelain drivers/usb/misc/sisusbvga/sisusb.c
> 
> to verify that this patch do not change any visible character. If the size of
> this patch is a problem, please let me know in how many patches to split it.

Thanks.

Maybe for future series, but not one in particular unless
you feel like respinning it:

As porcelain is meant for script consumption,  I think a good
mechanism
for human review is to separate patch 1 into 2 patches.

1: Only horizontal line whitespace changes
   git diff -w shouldn't show any changes and
   git blame -w would not show any change either.
2: Only vertical line changes additions/reformatting

This makes it easy to use git diff -w on patch 1 and
patch 2 can be more easily scanned visually.

It's also nice to use objdiff and show no object changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302855

On Tue, Jan 05, 2016 at 10:53:24AM -0800, Joe Perches wrote:
> On Tue, 2016-01-05 at 17:54 +0100, Peter Senna Tschudin wrote:
> []
> > Patch 1 is the biggest and fix only whitespace, tab and newline issues. I used
> > 
> > $ git diff -w --word-diff=porcelain drivers/usb/misc/sisusbvga/sisusb.c
> > 
> > to verify that this patch do not change any visible character. If the size of
> > this patch is a problem, please let me know in how many patches to split it.
> 
> Thanks.
> 
> Maybe for future series, but not one in particular unless
> you feel like respinning it:
> 
> As porcelain is meant for script consumption,  I think a good
> mechanism
> for human review is to separate patch 1 into 2 patches.
> 
> 1: Only horizontal line whitespace changes
>    git diff -w shouldn't show any changes and
>    git blame -w would not show any change either.
> 2: Only vertical line changes additions/reformatting
> 
> This makes it easy to use git diff -w on patch 1 and
> patch 2 can be more easily scanned visually.
This makes sense, the challenge is doing horizontal first, as making
vertical changes will probably require additional horizontal changes. Or
is it ok to fix horizontal issues caused by vertical changes in the
vertical changes patch? Is it reasonable to respin if other issues are
reported for this series?  Anyway I'll make the next series as you
suggest.

> 
> It's also nice to use objdiff and show no object changes.
Nice tool. I didn't know about it. Should I create a README file for the
scripts directory?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302867

On Wed, 2016-01-06 at 16:30 +0100, Peter Senna Tschudin wrote:
> is it ok to fix horizontal issues caused by vertical changes in the
> vertical changes patch?

Yes.

> Is it reasonable to respin if other issues are
> reported for this series?

Your choice.
I think it'd be better to respin in any case.

> > It's also nice to use objdiff and show no object changes.
> Nice tool. I didn't know about it. Should I create a README file for the
> scripts directory?

Maybe.

I think most script and patch submission documentation
is in Documentation/

Maybe this should be added to SubmittingPatches and/or
SubmitChecklist.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web