Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
Dear Tilman,
Thanks for the feedback. I made a lot of mistakes... Sorry for wasting your
time with this patch. I'll work on improving it and dividing it up
into smaller chunks
as recommended by other kernel developers and will try to fix the issues.
With kind regards,
Bas
2015-01-03 16:01 GMT+01:00 Tilman Schmidt :
> [I only just noticed that my first reply got terribly mangled by my
> mailer, so here it is again, hopefully more readable this time.]
>
> Hello Bas,
>
> I have several objections to your patch.
>
> Am 31.12.2014 um 18:34 schrieb Bas Peters:
>> I have not been able to test the code as I do not have access to the
>> hardware but since no new features were really added I don't think that
>> should pose a problem.
>
> It's always problematic to change code you cannot test.
> At the very least, if you do coding style cleanups you should test
> whether the result still compiles and generates the same code as before.
>
>> --- a/drivers/isdn/gigaset/bas-gigaset.c
>> +++ b/drivers/isdn/gigaset/bas-gigaset.c
>> @@ -261,11 +261,12 @@ static inline void dump_urb(enum debuglevel level,
>> const char *tag,
>> {
>> #ifdef CONFIG_GIGASET_DEBUG
>> int i;
>> +
>> gig_dbg(level, "%s urb(0x%08lx)->{", tag, (unsigned long) urb);
>> if (urb) {
>> gig_dbg(level,
>> - " dev=0x%08lx, pipe=%s:EP%d/DV%d:%s, "
>> - "hcpriv=0x%08lx, transfer_flags=0x%x,",
>> + " dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
>> + hcpriv=0x%08lx, transfer_flags=0x%x,",
>
> This is syntactically wrong and won't compile. You cannot have an
> unescaped newline inside a string literal. (Applies to two later
> chunks, too.)
>
>> @@ -566,8 +566,8 @@ static int atread_submit(struct cardstate *cs, int
>> timeout)
>>
>> if (basstate & BS_SUSPEND) {
>> dev_notice(cs->dev,
>> -"HD_READ_ATMESSAGE not submitted, "
>> -"suspend in progress\n");
>> +"HD_READ_ATMESSAGE not submitted,\
>> +suspend in progress\n");
>
> This makes the message less readable by inserting lots of whitespace
> after the comma. (Applies to five later chunks, too.)
>
>> @@ -2312,13 +2312,13 @@ static int gigaset_probe(struct usb_interface
>> *interface,
>> /* Reject application specific interfaces
>>*/
>> if (hostif->desc.bInterfaceClass != 255) {
>> - dev_warn(>dev, "%s: bInterfaceClass == %d\n",
>> + dev_warn(>dev, "%s: bInterfaceClass == %d\n",\
>>__func__, hostif->desc.bInterfaceClass);
>> return -ENODEV;
>> }
>>
>> dev_info(>dev,
>> - "%s: Device matched (Vendor: 0x%x, Product: 0x%x)\n",
>> + "%s: Device matched (Vendor: 0x%x, Product: 0x%x)\n",\
>>__func__, le16_to_cpu(udev->descriptor.idVendor),
>>le16_to_cpu(udev->descriptor.idProduct));
>
> This looks strange, and not like correct coding style. Why would you
> want to escape the end of line after a function argument?
>
>
>> --- a/drivers/isdn/gigaset/capi.c
>> +++ b/drivers/isdn/gigaset/capi.c
>
>> @@ -1370,7 +1373,7 @@ static void do_connect_req(struct gigaset_capi_ctr
>> *iif,
>> cmsg->adr.adrPLCI |= (bcs->channel + 1) << 8;
>>
>> /* build command table */
>> - commands = kzalloc(AT_NUM * (sizeof *commands), GFP_KERNEL);
>> + commands = kzalloc(AT_NUM * (sizeof(*commands)), GFP_KERNEL);
>
> Extra pair of parentheses around sizeof(*commands) is unnecessary.
> (Applies to one later chunk, too.)
>
>> --- a/drivers/isdn/gigaset/common.c
>> +++ b/drivers/isdn/gigaset/common.c
>> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const
>> unsigned char *msg,
>> {
>> unsigned char outbuf[80];
>> unsigned char c;
>> - size_t space = sizeof outbuf - 1;
>> + size_t space = sizeof(outbuf - 1);
>
> This is wrong. The sizeof operator must be applied to the array
> variable outbuf, not to the expression (outbuf - 1).
>
>> --- a/drivers/isdn/gigaset/ev-layer.c
>> +++ b/drivers/isdn/gigaset/ev-layer.c
>
>> @@ -1083,7 +1079,7 @@ static void do_action(int action, struct cardstate *cs,
>>
>> int channel;
>>
>> - unsigned char *s, *e;
>> + unsigned char *s;
>> int i;
>> unsigned long val;
>>
>> @@ -1355,8 +1351,20 @@ static void do_action(int action, struct cardstate
>> *cs,
>> }
>>
>> for (i = 0; i < 4; ++i) {
>> - val = simple_strtoul(s, (char **) , 10);
>> - if (val > INT_MAX || e == s)
>> + unsigned long *e;
>> +
>> + val = kstrtoul(s, 10, e);
>> + if (val == -EINVAL) {
>> + dev_err(cs->dev, "Parsing error on converting
>> string to\
>> + unsigned long\n");
>> +
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
Dear Tilman,
Thanks for the feedback. I made a lot of mistakes... Sorry for wasting your
time with this patch. I'll work on improving it and dividing it up
into smaller chunks
as recommended by other kernel developers and will try to fix the issues.
With kind regards,
Bas
2015-01-03 16:01 GMT+01:00 Tilman Schmidt til...@imap.cc:
[I only just noticed that my first reply got terribly mangled by my
mailer, so here it is again, hopefully more readable this time.]
Hello Bas,
I have several objections to your patch.
Am 31.12.2014 um 18:34 schrieb Bas Peters:
I have not been able to test the code as I do not have access to the
hardware but since no new features were really added I don't think that
should pose a problem.
It's always problematic to change code you cannot test.
At the very least, if you do coding style cleanups you should test
whether the result still compiles and generates the same code as before.
--- a/drivers/isdn/gigaset/bas-gigaset.c
+++ b/drivers/isdn/gigaset/bas-gigaset.c
@@ -261,11 +261,12 @@ static inline void dump_urb(enum debuglevel level,
const char *tag,
{
#ifdef CONFIG_GIGASET_DEBUG
int i;
+
gig_dbg(level, %s urb(0x%08lx)-{, tag, (unsigned long) urb);
if (urb) {
gig_dbg(level,
- dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
- hcpriv=0x%08lx, transfer_flags=0x%x,,
+ dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
+ hcpriv=0x%08lx, transfer_flags=0x%x,,
This is syntactically wrong and won't compile. You cannot have an
unescaped newline inside a string literal. (Applies to two later
chunks, too.)
@@ -566,8 +566,8 @@ static int atread_submit(struct cardstate *cs, int
timeout)
if (basstate BS_SUSPEND) {
dev_notice(cs-dev,
-HD_READ_ATMESSAGE not submitted,
-suspend in progress\n);
+HD_READ_ATMESSAGE not submitted,\
+suspend in progress\n);
This makes the message less readable by inserting lots of whitespace
after the comma. (Applies to five later chunks, too.)
@@ -2312,13 +2312,13 @@ static int gigaset_probe(struct usb_interface
*interface,
/* Reject application specific interfaces
*/
if (hostif-desc.bInterfaceClass != 255) {
- dev_warn(udev-dev, %s: bInterfaceClass == %d\n,
+ dev_warn(udev-dev, %s: bInterfaceClass == %d\n,\
__func__, hostif-desc.bInterfaceClass);
return -ENODEV;
}
dev_info(udev-dev,
- %s: Device matched (Vendor: 0x%x, Product: 0x%x)\n,
+ %s: Device matched (Vendor: 0x%x, Product: 0x%x)\n,\
__func__, le16_to_cpu(udev-descriptor.idVendor),
le16_to_cpu(udev-descriptor.idProduct));
This looks strange, and not like correct coding style. Why would you
want to escape the end of line after a function argument?
--- a/drivers/isdn/gigaset/capi.c
+++ b/drivers/isdn/gigaset/capi.c
@@ -1370,7 +1373,7 @@ static void do_connect_req(struct gigaset_capi_ctr
*iif,
cmsg-adr.adrPLCI |= (bcs-channel + 1) 8;
/* build command table */
- commands = kzalloc(AT_NUM * (sizeof *commands), GFP_KERNEL);
+ commands = kzalloc(AT_NUM * (sizeof(*commands)), GFP_KERNEL);
Extra pair of parentheses around sizeof(*commands) is unnecessary.
(Applies to one later chunk, too.)
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const
unsigned char *msg,
{
unsigned char outbuf[80];
unsigned char c;
- size_t space = sizeof outbuf - 1;
+ size_t space = sizeof(outbuf - 1);
This is wrong. The sizeof operator must be applied to the array
variable outbuf, not to the expression (outbuf - 1).
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -1083,7 +1079,7 @@ static void do_action(int action, struct cardstate *cs,
int channel;
- unsigned char *s, *e;
+ unsigned char *s;
int i;
unsigned long val;
@@ -1355,8 +1351,20 @@ static void do_action(int action, struct cardstate
*cs,
}
for (i = 0; i 4; ++i) {
- val = simple_strtoul(s, (char **) e, 10);
- if (val INT_MAX || e == s)
+ unsigned long *e;
+
+ val = kstrtoul(s, 10, e);
+ if (val == -EINVAL) {
+ dev_err(cs-dev, Parsing error on converting
string to\
+ unsigned long\n);
+ break;
+ }
+ if (val == -ERANGE) {
+ dev_err(cs-dev, Overflow error converting
string to\
+
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
[I only just noticed that my first reply got terribly mangled by my
mailer, so here it is again, hopefully more readable this time.]
Hello Bas,
I have several objections to your patch.
Am 31.12.2014 um 18:34 schrieb Bas Peters:
> I have not been able to test the code as I do not have access to the
> hardware but since no new features were really added I don't think that
> should pose a problem.
It's always problematic to change code you cannot test.
At the very least, if you do coding style cleanups you should test
whether the result still compiles and generates the same code as before.
> --- a/drivers/isdn/gigaset/bas-gigaset.c
> +++ b/drivers/isdn/gigaset/bas-gigaset.c
> @@ -261,11 +261,12 @@ static inline void dump_urb(enum debuglevel level,
> const char *tag,
> {
> #ifdef CONFIG_GIGASET_DEBUG
> int i;
> +
> gig_dbg(level, "%s urb(0x%08lx)->{", tag, (unsigned long) urb);
> if (urb) {
> gig_dbg(level,
> - " dev=0x%08lx, pipe=%s:EP%d/DV%d:%s, "
> - "hcpriv=0x%08lx, transfer_flags=0x%x,",
> + " dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
> + hcpriv=0x%08lx, transfer_flags=0x%x,",
This is syntactically wrong and won't compile. You cannot have an
unescaped newline inside a string literal. (Applies to two later
chunks, too.)
> @@ -566,8 +566,8 @@ static int atread_submit(struct cardstate *cs, int
> timeout)
>
> if (basstate & BS_SUSPEND) {
> dev_notice(cs->dev,
> -"HD_READ_ATMESSAGE not submitted, "
> -"suspend in progress\n");
> +"HD_READ_ATMESSAGE not submitted,\
> +suspend in progress\n");
This makes the message less readable by inserting lots of whitespace
after the comma. (Applies to five later chunks, too.)
> @@ -2312,13 +2312,13 @@ static int gigaset_probe(struct usb_interface
> *interface,
> /* Reject application specific interfaces
>*/
> if (hostif->desc.bInterfaceClass != 255) {
> - dev_warn(>dev, "%s: bInterfaceClass == %d\n",
> + dev_warn(>dev, "%s: bInterfaceClass == %d\n",\
>__func__, hostif->desc.bInterfaceClass);
> return -ENODEV;
> }
>
> dev_info(>dev,
> - "%s: Device matched (Vendor: 0x%x, Product: 0x%x)\n",
> + "%s: Device matched (Vendor: 0x%x, Product: 0x%x)\n",\
>__func__, le16_to_cpu(udev->descriptor.idVendor),
>le16_to_cpu(udev->descriptor.idProduct));
This looks strange, and not like correct coding style. Why would you
want to escape the end of line after a function argument?
> --- a/drivers/isdn/gigaset/capi.c
> +++ b/drivers/isdn/gigaset/capi.c
> @@ -1370,7 +1373,7 @@ static void do_connect_req(struct gigaset_capi_ctr *iif,
> cmsg->adr.adrPLCI |= (bcs->channel + 1) << 8;
>
> /* build command table */
> - commands = kzalloc(AT_NUM * (sizeof *commands), GFP_KERNEL);
> + commands = kzalloc(AT_NUM * (sizeof(*commands)), GFP_KERNEL);
Extra pair of parentheses around sizeof(*commands) is unnecessary.
(Applies to one later chunk, too.)
> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const
> unsigned char *msg,
> {
> unsigned char outbuf[80];
> unsigned char c;
> - size_t space = sizeof outbuf - 1;
> + size_t space = sizeof(outbuf - 1);
This is wrong. The sizeof operator must be applied to the array
variable outbuf, not to the expression (outbuf - 1).
> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
> @@ -1083,7 +1079,7 @@ static void do_action(int action, struct cardstate *cs,
>
> int channel;
>
> - unsigned char *s, *e;
> + unsigned char *s;
> int i;
> unsigned long val;
>
> @@ -1355,8 +1351,20 @@ static void do_action(int action, struct cardstate *cs,
> }
>
> for (i = 0; i < 4; ++i) {
> - val = simple_strtoul(s, (char **) , 10);
> - if (val > INT_MAX || e == s)
> + unsigned long *e;
> +
> + val = kstrtoul(s, 10, e);
> + if (val == -EINVAL) {
> + dev_err(cs->dev, "Parsing error on converting
> string to\
> + unsigned long\n");
> + break;
> + }
> + if (val == -ERANGE) {
> + dev_err(cs->dev, "Overflow error converting
> string to\
> + unsigned long\n");
> + break;
> + }
> + if (val > INT_MAX || *e == s)
> break;
> if (i == 3) {
>
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
[I only just noticed that my first reply got terribly mangled by my
mailer, so here it is again, hopefully more readable this time.]
Hello Bas,
I have several objections to your patch.
Am 31.12.2014 um 18:34 schrieb Bas Peters:
I have not been able to test the code as I do not have access to the
hardware but since no new features were really added I don't think that
should pose a problem.
It's always problematic to change code you cannot test.
At the very least, if you do coding style cleanups you should test
whether the result still compiles and generates the same code as before.
--- a/drivers/isdn/gigaset/bas-gigaset.c
+++ b/drivers/isdn/gigaset/bas-gigaset.c
@@ -261,11 +261,12 @@ static inline void dump_urb(enum debuglevel level,
const char *tag,
{
#ifdef CONFIG_GIGASET_DEBUG
int i;
+
gig_dbg(level, %s urb(0x%08lx)-{, tag, (unsigned long) urb);
if (urb) {
gig_dbg(level,
- dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
- hcpriv=0x%08lx, transfer_flags=0x%x,,
+ dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
+ hcpriv=0x%08lx, transfer_flags=0x%x,,
This is syntactically wrong and won't compile. You cannot have an
unescaped newline inside a string literal. (Applies to two later
chunks, too.)
@@ -566,8 +566,8 @@ static int atread_submit(struct cardstate *cs, int
timeout)
if (basstate BS_SUSPEND) {
dev_notice(cs-dev,
-HD_READ_ATMESSAGE not submitted,
-suspend in progress\n);
+HD_READ_ATMESSAGE not submitted,\
+suspend in progress\n);
This makes the message less readable by inserting lots of whitespace
after the comma. (Applies to five later chunks, too.)
@@ -2312,13 +2312,13 @@ static int gigaset_probe(struct usb_interface
*interface,
/* Reject application specific interfaces
*/
if (hostif-desc.bInterfaceClass != 255) {
- dev_warn(udev-dev, %s: bInterfaceClass == %d\n,
+ dev_warn(udev-dev, %s: bInterfaceClass == %d\n,\
__func__, hostif-desc.bInterfaceClass);
return -ENODEV;
}
dev_info(udev-dev,
- %s: Device matched (Vendor: 0x%x, Product: 0x%x)\n,
+ %s: Device matched (Vendor: 0x%x, Product: 0x%x)\n,\
__func__, le16_to_cpu(udev-descriptor.idVendor),
le16_to_cpu(udev-descriptor.idProduct));
This looks strange, and not like correct coding style. Why would you
want to escape the end of line after a function argument?
--- a/drivers/isdn/gigaset/capi.c
+++ b/drivers/isdn/gigaset/capi.c
@@ -1370,7 +1373,7 @@ static void do_connect_req(struct gigaset_capi_ctr *iif,
cmsg-adr.adrPLCI |= (bcs-channel + 1) 8;
/* build command table */
- commands = kzalloc(AT_NUM * (sizeof *commands), GFP_KERNEL);
+ commands = kzalloc(AT_NUM * (sizeof(*commands)), GFP_KERNEL);
Extra pair of parentheses around sizeof(*commands) is unnecessary.
(Applies to one later chunk, too.)
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const
unsigned char *msg,
{
unsigned char outbuf[80];
unsigned char c;
- size_t space = sizeof outbuf - 1;
+ size_t space = sizeof(outbuf - 1);
This is wrong. The sizeof operator must be applied to the array
variable outbuf, not to the expression (outbuf - 1).
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -1083,7 +1079,7 @@ static void do_action(int action, struct cardstate *cs,
int channel;
- unsigned char *s, *e;
+ unsigned char *s;
int i;
unsigned long val;
@@ -1355,8 +1351,20 @@ static void do_action(int action, struct cardstate *cs,
}
for (i = 0; i 4; ++i) {
- val = simple_strtoul(s, (char **) e, 10);
- if (val INT_MAX || e == s)
+ unsigned long *e;
+
+ val = kstrtoul(s, 10, e);
+ if (val == -EINVAL) {
+ dev_err(cs-dev, Parsing error on converting
string to\
+ unsigned long\n);
+ break;
+ }
+ if (val == -ERANGE) {
+ dev_err(cs-dev, Overflow error converting
string to\
+ unsigned long\n);
+ break;
+ }
+ if (val INT_MAX || *e == s)
break;
if (i == 3) {
if (*e)
@@ -1364,7 +1372,7 @@ static void do_action(int action, struct cardstate *cs,
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Hello Bas,
I have several objections to your patch.
Am 31.12.2014 um 18:34 schrieb Bas Peters:
> I have not been able to test the code as I do not have access to
> the hardware but since no new features were really added I don't
> think that should pose a problem.
It's always problematic to change code you cannot test.
At the very least, if you do coding style cleanups you should test
whether the result still compiles and generates the same code as before.
> --- a/drivers/isdn/gigaset/bas-gigaset.c +++
> b/drivers/isdn/gigaset/bas-gigaset.c @@ -261,11 +261,12 @@ static
> inline void dump_urb(enum debuglevel level, const char *tag, {
> #ifdef CONFIG_GIGASET_DEBUG int i; + gig_dbg(level, "%s
> urb(0x%08lx)->{", tag, (unsigned long) urb); if (urb) {
> gig_dbg(level, - " dev=0x%08lx, pipe=%s:EP%d/DV%d:%s, "
> -
> "hcpriv=0x%08lx, transfer_flags=0x%x,", + " dev=0x%08lx,
> pipe=%s:EP%d/DV%d:%s, + hcpriv=0x%08lx,
> transfer_flags=0x%x,",
This is syntactically wrong and won't compile. You cannot have an
unescaped newline inside a string literal.
> @@ -2312,13 +2312,13 @@ static int gigaset_probe(struct
> usb_interface *interface, /* Reject application specific
> interfaces */ if (hostif->desc.bInterfaceClass != 255) { -
> dev_warn(>dev, "%s: bInterfaceClass == %d\n", +
> dev_warn(>dev, "%s: bInterfaceClass == %d\n",\ __func__,
> hostif->desc.bInterfaceClass); return -ENODEV; }
>
> dev_info(>dev, - "%s: Device matched (Vendor: 0x%x,
> Product: 0x%x)\n", + "%s: Device matched (Vendor: 0x%x, Product:
> 0x%x)\n",\ __func__, le16_to_cpu(udev->descriptor.idVendor),
> le16_to_cpu(udev->descriptor.idProduct));
This looks strange, and not like correct coding style. Why would you
want to escape the end of line after a function argument?
> --- a/drivers/isdn/gigaset/common.c +++
> b/drivers/isdn/gigaset/common.c @@ -53,7 +53,7 @@ void
> gigaset_dbg_buffer(enum debuglevel level, const unsigned char
> *msg, { unsigned char outbuf[80]; unsigned char c; - size_t space =
> sizeof outbuf - 1; + size_t space = sizeof(outbuf - 1);
This is wrong. The sizeof operator must be applied to the array
variable outbuf, not to the expression (outbuf - 1).
> --- a/drivers/isdn/gigaset/ev-layer.c +++
> b/drivers/isdn/gigaset/ev-layer.c
> @@ -1355,8 +1351,20 @@ static void do_action(int action, struct
> cardstate *cs, }
>
> for (i = 0; i < 4; ++i) { - val = simple_strtoul(s, (char
> **) ,
> 10); -if (val > INT_MAX || e == s) +
> unsigned long *e; + +
> val = kstrtoul(s, 10, e); + if (val == -EINVAL) { +
> dev_err(cs->dev, "Parsing error on converting string to\ +
> unsigned long\n"); + break; +
> } + if (val == -ERANGE) { +
> dev_err(cs->dev, "Overflow error converting string to\ +
> unsigned long\n"); + break; +
> } + if (val > INT_MAX || *e ==
> s) break; if (i == 3) { if (*e)
This cannot work. The pointer variable e gets dereferenced without
ever being initialized. The type mismatches when declaring e as
pointing to an unsigned long but comparing *e to s in one place and to
a character literal in another point make me wonder which semantics
you had in mind for e in the first place.
Also your error messages are not helpful for someone reading the log
and trying to find out what went wrong, and not very readable because
of the big stretch of whitespace you insert between the words "to" and
"unsigned". In fact I'm not even convinced it's a good idea to emit a
log message at all here.
> --- a/drivers/isdn/gigaset/gigaset.h +++
> b/drivers/isdn/gigaset/gigaset.h @@ -94,8 +94,7 @@ enum debuglevel
> { #define gig_dbg(level, format, arg...)
> \ do { \ if
> (unlikely(((enum debuglevel)gigaset_debuglevel) & (level))) \ -
> printk(KERN_DEBUG KBUILD_MODNAME ": " format "\n", \ -
>##
> arg); \ +
> dev_dbg(cs->dev, KBUILD_MODNAME ": " format "\n")\
> } while (0) #define DEBUG_DEFAULT (DEBUG_TRANSCMD | DEBUG_CMD |
> DEBUG_USBREQ)
>
This will not work when
- - there is no cs variable in the context where the macro is used or
- - cs->dev doesn't contain a valid device pointer or
- - the format string references additional arguments,
all of which actually occur in the driver.
> --- a/drivers/isdn/gigaset/i4l.c +++ b/drivers/isdn/gigaset/i4l.c
> @@ -624,14 +624,14 @@ int gigaset_isdn_regdev(struct cardstate *cs,
> const char *isdnid) { isdn_if *iif;
>
> - iif = kmalloc(sizeof *iif, GFP_KERNEL); + iif =
> kmalloc(sizeof(*iif, GFP_KERNEL)); if (!iif) { pr_err("out of
> memory\n"); return -ENOMEM;
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Hello Bas,
I have several objections to your patch.
Am 31.12.2014 um 18:34 schrieb Bas Peters:
I have not been able to test the code as I do not have access to
the hardware but since no new features were really added I don't
think that should pose a problem.
It's always problematic to change code you cannot test.
At the very least, if you do coding style cleanups you should test
whether the result still compiles and generates the same code as before.
--- a/drivers/isdn/gigaset/bas-gigaset.c +++
b/drivers/isdn/gigaset/bas-gigaset.c @@ -261,11 +261,12 @@ static
inline void dump_urb(enum debuglevel level, const char *tag, {
#ifdef CONFIG_GIGASET_DEBUG int i; + gig_dbg(level, %s
urb(0x%08lx)-{, tag, (unsigned long) urb); if (urb) {
gig_dbg(level, -dev=0x%08lx, pipe=%s:EP%d/DV%d:%s,
-
hcpriv=0x%08lx, transfer_flags=0x%x,, + dev=0x%08lx,
pipe=%s:EP%d/DV%d:%s, + hcpriv=0x%08lx,
transfer_flags=0x%x,,
This is syntactically wrong and won't compile. You cannot have an
unescaped newline inside a string literal.
@@ -2312,13 +2312,13 @@ static int gigaset_probe(struct
usb_interface *interface, /* Reject application specific
interfaces */ if (hostif-desc.bInterfaceClass != 255) { -
dev_warn(udev-dev, %s: bInterfaceClass == %d\n, +
dev_warn(udev-dev, %s: bInterfaceClass == %d\n,\ __func__,
hostif-desc.bInterfaceClass); return -ENODEV; }
dev_info(udev-dev, - %s: Device matched (Vendor: 0x%x,
Product: 0x%x)\n, + %s: Device matched (Vendor: 0x%x, Product:
0x%x)\n,\ __func__, le16_to_cpu(udev-descriptor.idVendor),
le16_to_cpu(udev-descriptor.idProduct));
This looks strange, and not like correct coding style. Why would you
want to escape the end of line after a function argument?
--- a/drivers/isdn/gigaset/common.c +++
b/drivers/isdn/gigaset/common.c @@ -53,7 +53,7 @@ void
gigaset_dbg_buffer(enum debuglevel level, const unsigned char
*msg, { unsigned char outbuf[80]; unsigned char c; - size_t space =
sizeof outbuf - 1; + size_t space = sizeof(outbuf - 1);
This is wrong. The sizeof operator must be applied to the array
variable outbuf, not to the expression (outbuf - 1).
--- a/drivers/isdn/gigaset/ev-layer.c +++
b/drivers/isdn/gigaset/ev-layer.c
@@ -1355,8 +1351,20 @@ static void do_action(int action, struct
cardstate *cs, }
for (i = 0; i 4; ++i) { - val = simple_strtoul(s, (char
**) e,
10); -if (val INT_MAX || e == s) +
unsigned long *e; + +
val = kstrtoul(s, 10, e); + if (val == -EINVAL) { +
dev_err(cs-dev, Parsing error on converting string to\ +
unsigned long\n); + break; +
} + if (val == -ERANGE) { +
dev_err(cs-dev, Overflow error converting string to\ +
unsigned long\n); + break; +
} + if (val INT_MAX || *e ==
s) break; if (i == 3) { if (*e)
This cannot work. The pointer variable e gets dereferenced without
ever being initialized. The type mismatches when declaring e as
pointing to an unsigned long but comparing *e to s in one place and to
a character literal in another point make me wonder which semantics
you had in mind for e in the first place.
Also your error messages are not helpful for someone reading the log
and trying to find out what went wrong, and not very readable because
of the big stretch of whitespace you insert between the words to and
unsigned. In fact I'm not even convinced it's a good idea to emit a
log message at all here.
--- a/drivers/isdn/gigaset/gigaset.h +++
b/drivers/isdn/gigaset/gigaset.h @@ -94,8 +94,7 @@ enum debuglevel
{ #define gig_dbg(level, format, arg...)
\ do { \ if
(unlikely(((enum debuglevel)gigaset_debuglevel) (level))) \ -
printk(KERN_DEBUG KBUILD_MODNAME : format \n, \ -
##
arg); \ +
dev_dbg(cs-dev, KBUILD_MODNAME : format \n)\
} while (0) #define DEBUG_DEFAULT (DEBUG_TRANSCMD | DEBUG_CMD |
DEBUG_USBREQ)
This will not work when
- - there is no cs variable in the context where the macro is used or
- - cs-dev doesn't contain a valid device pointer or
- - the format string references additional arguments,
all of which actually occur in the driver.
--- a/drivers/isdn/gigaset/i4l.c +++ b/drivers/isdn/gigaset/i4l.c
@@ -624,14 +624,14 @@ int gigaset_isdn_regdev(struct cardstate *cs,
const char *isdnid) { isdn_if *iif;
- iif = kmalloc(sizeof *iif, GFP_KERNEL); + iif =
kmalloc(sizeof(*iif, GFP_KERNEL)); if (!iif) { pr_err(out of
memory\n); return -ENOMEM;
You're calling kmalloc with too few arguments here.
--- a/drivers/isdn/gigaset/proc.c +++
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
Bas, On Wed, Dec 31, 2014 at 07:04:30PM +0100, Bas Peters wrote: > 2014-12-31 18:49 GMT+01:00 Jeremiah Mahler : > > Bas, > > > > On Wed, Dec 31, 2014 at 06:34:58PM +0100, Bas Peters wrote: > >> Fixed many checkpatch.pl complaints, ranging from whitespace issues to > >> reportedly deprecated function and macro usage. > >> > > One patch should fix one type of problem. This needs to be broken up > > in to individual patches. > > > >> I have not been able to test the code as I do not have access to the > >> hardware but since no new features were really added I don't think that > >> should pose a problem. > >> > >> There are still some checkpatch complaints, particularly concerning > >> potentially unnecessary 'out of memory' messages. I will provide patches > >> for these complaints when I figure out the reason behind it and what to > >> do about it. > >> > >> NOTE: This is my first patch (ever). I have attempted to follow all > >> guidelines provided, but I probably will have missed some. If you have > >> any comments regarding the quality of this patch or suggestions as to > >> what I could do better in the future, please let me know. > >> > > You are ambitious. I would suggest trying a smaller patch first to > > make sure you are doing everything right. > > With 'smaller patch', do you refer to less files at once or a different > driver? > Is it generally preferred to send patches that relate to the same > issue (changes to a single file, > grouping of patches to tackle the same issue, such as conversion of a > specific function) over > patch(sets) that deal with an entire driver? > > Thanks for the advice! > Your patch should solve one problem. This could result in a single file being changed or many being changed. For simple checkpatch errors I usually group all of one type of error for one file as a patch. The goal is to make it easy to review. If you fixed 10 different types of issues in one patch it would difficult to review because I would have to remember whether the change I am looking at was for issue 3 or 8 or 5 or ...? Also, if the code had a bug, a bisect will point me to the patch. But then I have to figure out which of the 10 changes in that one patch created the problem. This is much easier if there was only one change in that one patch. Also have a look at Documentation/SubmittingPatches. Specifically the section "Separate your changes". > > > >> Signed-off-by: Bas Peters > >> --- > >> drivers/isdn/gigaset/asyncdata.c | 9 +++-- > >> drivers/isdn/gigaset/bas-gigaset.c | 80 > >> -- > >> drivers/isdn/gigaset/capi.c| 5 ++- > >> drivers/isdn/gigaset/common.c | 8 ++-- > >> drivers/isdn/gigaset/ev-layer.c| 38 +++--- > >> drivers/isdn/gigaset/gigaset.h | 4 +- > >> drivers/isdn/gigaset/i4l.c | 12 +++--- > >> drivers/isdn/gigaset/interface.c | 10 ++--- > >> drivers/isdn/gigaset/isocdata.c| 3 ++ > >> drivers/isdn/gigaset/proc.c| 17 +--- > >> drivers/isdn/gigaset/usb-gigaset.c | 46 +++--- > >> 11 files changed, 141 insertions(+), 91 deletions(-) > >> > >> diff --git a/drivers/isdn/gigaset/asyncdata.c > >> b/drivers/isdn/gigaset/asyncdata.c > > [...] > > > > -- > > - Jeremiah Mahler -- - Jeremiah Mahler -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
2014-12-31 18:49 GMT+01:00 Jeremiah Mahler : > Bas, > > On Wed, Dec 31, 2014 at 06:34:58PM +0100, Bas Peters wrote: >> Fixed many checkpatch.pl complaints, ranging from whitespace issues to >> reportedly deprecated function and macro usage. >> > One patch should fix one type of problem. This needs to be broken up > in to individual patches. > >> I have not been able to test the code as I do not have access to the >> hardware but since no new features were really added I don't think that >> should pose a problem. >> >> There are still some checkpatch complaints, particularly concerning >> potentially unnecessary 'out of memory' messages. I will provide patches >> for these complaints when I figure out the reason behind it and what to >> do about it. >> >> NOTE: This is my first patch (ever). I have attempted to follow all >> guidelines provided, but I probably will have missed some. If you have >> any comments regarding the quality of this patch or suggestions as to >> what I could do better in the future, please let me know. >> > You are ambitious. I would suggest trying a smaller patch first to > make sure you are doing everything right. With 'smaller patch', do you refer to less files at once or a different driver? Is it generally preferred to send patches that relate to the same issue (changes to a single file, grouping of patches to tackle the same issue, such as conversion of a specific function) over patch(sets) that deal with an entire driver? Thanks for the advice! > >> Signed-off-by: Bas Peters >> --- >> drivers/isdn/gigaset/asyncdata.c | 9 +++-- >> drivers/isdn/gigaset/bas-gigaset.c | 80 >> -- >> drivers/isdn/gigaset/capi.c| 5 ++- >> drivers/isdn/gigaset/common.c | 8 ++-- >> drivers/isdn/gigaset/ev-layer.c| 38 +++--- >> drivers/isdn/gigaset/gigaset.h | 4 +- >> drivers/isdn/gigaset/i4l.c | 12 +++--- >> drivers/isdn/gigaset/interface.c | 10 ++--- >> drivers/isdn/gigaset/isocdata.c| 3 ++ >> drivers/isdn/gigaset/proc.c| 17 +--- >> drivers/isdn/gigaset/usb-gigaset.c | 46 +++--- >> 11 files changed, 141 insertions(+), 91 deletions(-) >> >> diff --git a/drivers/isdn/gigaset/asyncdata.c >> b/drivers/isdn/gigaset/asyncdata.c > [...] > > -- > - Jeremiah Mahler -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
Bas, On Wed, Dec 31, 2014 at 06:34:58PM +0100, Bas Peters wrote: > Fixed many checkpatch.pl complaints, ranging from whitespace issues to > reportedly deprecated function and macro usage. > One patch should fix one type of problem. This needs to be broken up in to individual patches. > I have not been able to test the code as I do not have access to the > hardware but since no new features were really added I don't think that > should pose a problem. > > There are still some checkpatch complaints, particularly concerning > potentially unnecessary 'out of memory' messages. I will provide patches > for these complaints when I figure out the reason behind it and what to > do about it. > > NOTE: This is my first patch (ever). I have attempted to follow all > guidelines provided, but I probably will have missed some. If you have > any comments regarding the quality of this patch or suggestions as to > what I could do better in the future, please let me know. > You are ambitious. I would suggest trying a smaller patch first to make sure you are doing everything right. > Signed-off-by: Bas Peters > --- > drivers/isdn/gigaset/asyncdata.c | 9 +++-- > drivers/isdn/gigaset/bas-gigaset.c | 80 > -- > drivers/isdn/gigaset/capi.c| 5 ++- > drivers/isdn/gigaset/common.c | 8 ++-- > drivers/isdn/gigaset/ev-layer.c| 38 +++--- > drivers/isdn/gigaset/gigaset.h | 4 +- > drivers/isdn/gigaset/i4l.c | 12 +++--- > drivers/isdn/gigaset/interface.c | 10 ++--- > drivers/isdn/gigaset/isocdata.c| 3 ++ > drivers/isdn/gigaset/proc.c| 17 +--- > drivers/isdn/gigaset/usb-gigaset.c | 46 +++--- > 11 files changed, 141 insertions(+), 91 deletions(-) > > diff --git a/drivers/isdn/gigaset/asyncdata.c > b/drivers/isdn/gigaset/asyncdata.c [...] -- - Jeremiah Mahler -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
Bas, On Wed, Dec 31, 2014 at 06:34:58PM +0100, Bas Peters wrote: Fixed many checkpatch.pl complaints, ranging from whitespace issues to reportedly deprecated function and macro usage. One patch should fix one type of problem. This needs to be broken up in to individual patches. I have not been able to test the code as I do not have access to the hardware but since no new features were really added I don't think that should pose a problem. There are still some checkpatch complaints, particularly concerning potentially unnecessary 'out of memory' messages. I will provide patches for these complaints when I figure out the reason behind it and what to do about it. NOTE: This is my first patch (ever). I have attempted to follow all guidelines provided, but I probably will have missed some. If you have any comments regarding the quality of this patch or suggestions as to what I could do better in the future, please let me know. You are ambitious. I would suggest trying a smaller patch first to make sure you are doing everything right. Signed-off-by: Bas Peters baspeter...@gmail.com --- drivers/isdn/gigaset/asyncdata.c | 9 +++-- drivers/isdn/gigaset/bas-gigaset.c | 80 -- drivers/isdn/gigaset/capi.c| 5 ++- drivers/isdn/gigaset/common.c | 8 ++-- drivers/isdn/gigaset/ev-layer.c| 38 +++--- drivers/isdn/gigaset/gigaset.h | 4 +- drivers/isdn/gigaset/i4l.c | 12 +++--- drivers/isdn/gigaset/interface.c | 10 ++--- drivers/isdn/gigaset/isocdata.c| 3 ++ drivers/isdn/gigaset/proc.c| 17 +--- drivers/isdn/gigaset/usb-gigaset.c | 46 +++--- 11 files changed, 141 insertions(+), 91 deletions(-) diff --git a/drivers/isdn/gigaset/asyncdata.c b/drivers/isdn/gigaset/asyncdata.c [...] -- - Jeremiah Mahler -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
2014-12-31 18:49 GMT+01:00 Jeremiah Mahler jmmah...@gmail.com: Bas, On Wed, Dec 31, 2014 at 06:34:58PM +0100, Bas Peters wrote: Fixed many checkpatch.pl complaints, ranging from whitespace issues to reportedly deprecated function and macro usage. One patch should fix one type of problem. This needs to be broken up in to individual patches. I have not been able to test the code as I do not have access to the hardware but since no new features were really added I don't think that should pose a problem. There are still some checkpatch complaints, particularly concerning potentially unnecessary 'out of memory' messages. I will provide patches for these complaints when I figure out the reason behind it and what to do about it. NOTE: This is my first patch (ever). I have attempted to follow all guidelines provided, but I probably will have missed some. If you have any comments regarding the quality of this patch or suggestions as to what I could do better in the future, please let me know. You are ambitious. I would suggest trying a smaller patch first to make sure you are doing everything right. With 'smaller patch', do you refer to less files at once or a different driver? Is it generally preferred to send patches that relate to the same issue (changes to a single file, grouping of patches to tackle the same issue, such as conversion of a specific function) over patch(sets) that deal with an entire driver? Thanks for the advice! Signed-off-by: Bas Peters baspeter...@gmail.com --- drivers/isdn/gigaset/asyncdata.c | 9 +++-- drivers/isdn/gigaset/bas-gigaset.c | 80 -- drivers/isdn/gigaset/capi.c| 5 ++- drivers/isdn/gigaset/common.c | 8 ++-- drivers/isdn/gigaset/ev-layer.c| 38 +++--- drivers/isdn/gigaset/gigaset.h | 4 +- drivers/isdn/gigaset/i4l.c | 12 +++--- drivers/isdn/gigaset/interface.c | 10 ++--- drivers/isdn/gigaset/isocdata.c| 3 ++ drivers/isdn/gigaset/proc.c| 17 +--- drivers/isdn/gigaset/usb-gigaset.c | 46 +++--- 11 files changed, 141 insertions(+), 91 deletions(-) diff --git a/drivers/isdn/gigaset/asyncdata.c b/drivers/isdn/gigaset/asyncdata.c [...] -- - Jeremiah Mahler -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers: isdn: gigaset: checkpatch cleanup
Bas, On Wed, Dec 31, 2014 at 07:04:30PM +0100, Bas Peters wrote: 2014-12-31 18:49 GMT+01:00 Jeremiah Mahler jmmah...@gmail.com: Bas, On Wed, Dec 31, 2014 at 06:34:58PM +0100, Bas Peters wrote: Fixed many checkpatch.pl complaints, ranging from whitespace issues to reportedly deprecated function and macro usage. One patch should fix one type of problem. This needs to be broken up in to individual patches. I have not been able to test the code as I do not have access to the hardware but since no new features were really added I don't think that should pose a problem. There are still some checkpatch complaints, particularly concerning potentially unnecessary 'out of memory' messages. I will provide patches for these complaints when I figure out the reason behind it and what to do about it. NOTE: This is my first patch (ever). I have attempted to follow all guidelines provided, but I probably will have missed some. If you have any comments regarding the quality of this patch or suggestions as to what I could do better in the future, please let me know. You are ambitious. I would suggest trying a smaller patch first to make sure you are doing everything right. With 'smaller patch', do you refer to less files at once or a different driver? Is it generally preferred to send patches that relate to the same issue (changes to a single file, grouping of patches to tackle the same issue, such as conversion of a specific function) over patch(sets) that deal with an entire driver? Thanks for the advice! Your patch should solve one problem. This could result in a single file being changed or many being changed. For simple checkpatch errors I usually group all of one type of error for one file as a patch. The goal is to make it easy to review. If you fixed 10 different types of issues in one patch it would difficult to review because I would have to remember whether the change I am looking at was for issue 3 or 8 or 5 or ...? Also, if the code had a bug, a bisect will point me to the patch. But then I have to figure out which of the 10 changes in that one patch created the problem. This is much easier if there was only one change in that one patch. Also have a look at Documentation/SubmittingPatches. Specifically the section Separate your changes. Signed-off-by: Bas Peters baspeter...@gmail.com --- drivers/isdn/gigaset/asyncdata.c | 9 +++-- drivers/isdn/gigaset/bas-gigaset.c | 80 -- drivers/isdn/gigaset/capi.c| 5 ++- drivers/isdn/gigaset/common.c | 8 ++-- drivers/isdn/gigaset/ev-layer.c| 38 +++--- drivers/isdn/gigaset/gigaset.h | 4 +- drivers/isdn/gigaset/i4l.c | 12 +++--- drivers/isdn/gigaset/interface.c | 10 ++--- drivers/isdn/gigaset/isocdata.c| 3 ++ drivers/isdn/gigaset/proc.c| 17 +--- drivers/isdn/gigaset/usb-gigaset.c | 46 +++--- 11 files changed, 141 insertions(+), 91 deletions(-) diff --git a/drivers/isdn/gigaset/asyncdata.c b/drivers/isdn/gigaset/asyncdata.c [...] -- - Jeremiah Mahler -- - Jeremiah Mahler -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
