Drivers: isdn: gigaset: checkpatch cleanup

PATCH In review Bas Peters 3 participants 7 messages Dec 31, 2014 – Jan 5, 2015

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Bas Peters

author

2014-12-31 17:34 UTC #0 lore raw

PATCH 1/1 Drivers: isdn: gigaset: checkpatch cleanup

Fixed many checkpatch.pl complaints, ranging from whitespace issues to reportedly deprecated function and macro usage.

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.

Signed-off-by Bas Peters <[email protected]>

---
 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(-)

drivers/isdn/gigaset/asyncdata.c +6 -3

index c90dca5..6120c2b 100644
@@ -25,9 +25,12 @@
  */
 static inline int muststuff(unsigned char c)
 {
-	if (c < PPP_TRANS) return 1;
-	if (c == PPP_FLAG) return 1;
-	if (c == PPP_ESCAPE) return 1;
+	if (c < PPP_TRANS)
+		return 1;
+	if (c == PPP_FLAG)
+		return 1;
+	if (c == PPP_ESCAPE)
+		return 1;
 	/* other possible candidates: */
 	/* 0x91: XON with parity set */
 	/* 0x93: XOFF with parity set */

drivers/isdn/gigaset/bas-gigaset.c +43 ~~-37~~

178 lines not on this page · raw message

drivers/isdn/gigaset/capi.c +4 -1

index ccec777..2503d02 100644
@@ -164,6 +164,7 @@ static inline void ignore_cstruct_param(struct cardstate *cs, _cstruct param,
 static int encode_ie(char *in, u8 *out, int maxlen)
 {
 	int l = 0;
+
 	while (*in) {
 		if (!isxdigit(in[0]) || !isxdigit(in[1]) || l >= maxlen)
 			return -1;

@@ -181,6 +182,7 @@ static int encode_ie(char *in, u8 *out, int maxlen)
 static void decode_ie(u8 *in, char *out)
 {
 	int i = *in;
+
 	while (i-- > 0) {
 		/* ToDo: conversion to upper case necessary? */
 		*out++ = toupper(hex_asc_hi(*++in));

@@ -983,6 +985,7 @@ void gigaset_isdn_start(struct cardstate *cs)
 void gigaset_isdn_stop(struct cardstate *cs)
 {
 	struct gigaset_capi_ctr *iif = cs->iif;
+
 	capi_ctr_down(&iif->ctr);
 }

@@ -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);
 	if (!commands)
 		goto oom;

drivers/isdn/gigaset/common.c +5 -3

index 7c78144..f6ee247 100644
@@ -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);
 	unsigned char *out = outbuf;
 	size_t numin = len;

@@ -434,6 +434,7 @@ static void make_valid(struct cardstate *cs, unsigned mask)
 {
 	unsigned long flags;
 	struct gigaset_driver *drv = cs->driver;
+
 	spin_lock_irqsave(&drv->lock, flags);
 	cs->flags |= mask;
 	spin_unlock_irqrestore(&drv->lock, flags);

@@ -443,6 +444,7 @@ static void make_invalid(struct cardstate *cs, unsigned mask)
 {
 	unsigned long flags;
 	struct gigaset_driver *drv = cs->driver;
+
 	spin_lock_irqsave(&drv->lock, flags);
 	cs->flags &= ~mask;
 	spin_unlock_irqrestore(&drv->lock, flags);

@@ -1077,7 +1079,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	unsigned long flags;
 	unsigned i;
 
-	drv = kmalloc(sizeof *drv, GFP_KERNEL);
+	drv = kmalloc(sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return NULL;

@@ -1090,7 +1092,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	drv->owner = owner;
 	INIT_LIST_HEAD(&drv->list);
 
-	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
+	drv->cs = kmalloc(minors * sizeof(*drv->cs), GFP_KERNEL);
 	if (!drv->cs)
 		goto error;

drivers/isdn/gigaset/ev-layer.c +23 ~~-15~~

index c8ced12..84b1700 100644
@@ -147,8 +147,7 @@
 
 /* 100: init, 200: dle0, 250:dle1, 300: get cid (dial), 350: "hup" (no cid),
  * 400: hup, 500: reset, 600: dial, 700: ring */
-struct reply_t gigaset_tab_nocid[] =
-{
+struct reply_t gigaset_tab_nocid[] = {
 /* resp_code, min_ConState, max_ConState, parameter, new_ConState, timeout,
  * action, command */

@@ -256,8 +255,7 @@ struct reply_t gigaset_tab_nocid[] =
 
 /* 600: start dialing, 650: dial in progress, 800: connection is up, 700: ring,
  * 400: hup, 750: accepted icall */
-struct reply_t gigaset_tab_cid[] =
-{
+struct reply_t gigaset_tab_cid[] = {
 /* resp_code, min_ConState, max_ConState, parameter, new_ConState, timeout,
  * action, command */

@@ -355,8 +353,7 @@ static const struct resp_type_t {
 	int	resp_code;
 	int	type;
 }
-resp_type[] =
-{
+resp_type[] = {
 	{"OK",		RSP_OK,		RT_NOTHING},
 	{"ERROR",	RSP_ERROR,	RT_NOTHING},
 	{"ZSAU",	RSP_ZSAU,	RT_ZSAU},

@@ -378,8 +375,7 @@ static const struct zsau_resp_t {
 	char	*str;
 	int	code;
 }
-zsau_resp[] =
-{
+zsau_resp[] = {
 	{"OUTGOING_CALL_PROCEEDING",	ZSAU_PROCEEDING},
 	{"CALL_DELIVERED",		ZSAU_CALL_DELIVERED},
 	{"ACTIVE",			ZSAU_ACTIVE},

@@ -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,
 			} else if (*e != '.')
 				break;
 			else
-				s = e + 1;
+				s = *e + 1;
 			cs->fwver[i] = val;
 		}
 		if (i != 4) {

@@ -1447,7 +1455,7 @@ static void do_action(int action, struct cardstate *cs,
 		else if (cs->gotfwver != 1) {
 			cs->cmd_result = -ENOENT;
 		} else {
-			memcpy(ev->arg, cs->fwver, sizeof cs->fwver);
+			memcpy(ev->arg, cs->fwver, sizeof(cs->fwver));
 			cs->cmd_result = 0;
 		}
 		cs->waiting = 0;

@@ -1576,8 +1584,8 @@ static void process_event(struct cardstate *cs, struct event_t *ev)
 		rcode = rep->resp_code;
 		if (rcode == RSP_LAST) {
 			/* found nothing...*/
-			dev_warn(cs->dev, "%s: rcode=RSP_LAST: "
-				 "resp_code %d in ConState %d!\n",
+			dev_warn(cs->dev, "%s: rcode=RSP_LAST:\
+					 resp_code %d in ConState %d!\n",
 				 __func__, ev->type, at_state->ConState);
 			return;
 		}

drivers/isdn/gigaset/gigaset.h +2 -2

index eb63a0f..e5c6c98 100644
@@ -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)

@@ -769,6 +768,7 @@ int gigaset_enterconfigmode(struct cardstate *cs);
 static inline void gigaset_schedule_event(struct cardstate *cs)
 {
 	unsigned long flags;
+
 	spin_lock_irqsave(&cs->lock, flags);
 	if (cs->running)
 		tasklet_schedule(&cs->event_tasklet);

drivers/isdn/gigaset/i4l.c +6 -6

index 2d75329..57b7827 100644
@@ -243,7 +243,7 @@ static int command_from_LL(isdn_ctrl *cntrl)
 		dev_kfree_skb(bcs->rx_skb);
 		gigaset_new_rx_skb(bcs);
 
-		commands = kzalloc(AT_NUM * (sizeof *commands), GFP_ATOMIC);
+		commands = kzalloc(AT_NUM * (sizeof(*commands)), GFP_ATOMIC);
 		if (!commands) {
 			gigaset_free_channel(bcs);
 			dev_err(cs->dev, "ISDN_CMD_DIAL: out of memory\n");

@@ -487,12 +487,12 @@ int gigaset_isdn_icall(struct at_state_t *at_state)
 	}
 	if (at_state->str_var[STR_NMBR]) {
 		strlcpy(response.parm.setup.phone, at_state->str_var[STR_NMBR],
-			sizeof response.parm.setup.phone);
+			sizeof(response.parm.setup.phone));
 	} else
 		response.parm.setup.phone[0] = 0;
 	if (at_state->str_var[STR_ZCPN]) {
 		strlcpy(response.parm.setup.eazmsn, at_state->str_var[STR_ZCPN],
-			sizeof response.parm.setup.eazmsn);
+			sizeof(response.parm.setup.eazmsn));
 	} else
 		response.parm.setup.eazmsn[0] = 0;

@@ -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;
 	}
 
-	if (snprintf(iif->id, sizeof iif->id, "%s_%u", isdnid, cs->minor_index)
-	    >= sizeof iif->id) {
+	if (snprintf(iif->id, sizeof(iif->id, "%s_%u", isdnid, cs->minor_index))
+	    >= sizeof(iif->id)) {
 		pr_err("ID too long: %s\n", isdnid);
 		kfree(iif);
 		return -EINVAL;

drivers/isdn/gigaset/interface.c +5 -5

index 600c79b..6da5d1c 100644
@@ -67,10 +67,10 @@ static int if_version(struct cardstate *cs, unsigned arg[4])
 
 	switch (cmd) {
 	case GIGVER_DRIVER:
-		memcpy(arg, version, sizeof version);
+		memcpy(arg, version, sizeof(version));
 		return 0;
 	case GIGVER_COMPAT:
-		memcpy(arg, compat, sizeof compat);
+		memcpy(arg, compat, sizeof(compat));
 		return 0;
 	case GIGVER_FWBASE:
 		cs->waiting = 1;

@@ -212,13 +212,13 @@ static int if_ioctl(struct tty_struct *tty,
 			break;
 		case GIGASET_VERSION:
 			retval = copy_from_user(version,
-						(unsigned __user *) arg, sizeof version)
+						(unsigned __user *) arg, sizeof(version))
 				? -EFAULT : 0;
 			if (retval >= 0)
 				retval = if_version(cs, version);
 			if (retval >= 0)
 				retval = copy_to_user((unsigned __user *) arg,
-						      version, sizeof version)
+						      version, sizeof(version))
 					? -EFAULT : 0;
 			break;
 		default:

@@ -510,7 +510,7 @@ void gigaset_if_init(struct cardstate *cs)
 	if (!IS_ERR(cs->tty_dev))
 		dev_set_drvdata(cs->tty_dev, cs);
 	else {
-		pr_warning("could not register device to the tty subsystem\n");
+		pr_warn("could not register device to the tty subsystem\n");
 		cs->tty_dev = NULL;
 	}
 	mutex_unlock(&cs->mutex);

drivers/isdn/gigaset/isocdata.c +3 -0

index bc29f1d..32ed9c5 100644
@@ -79,6 +79,7 @@ static inline int isowbuf_startwrite(struct isowbuf_t *iwb)
 static inline int isowbuf_donewrite(struct isowbuf_t *iwb)
 {
 	int write = iwb->write;
+
 	atomic_inc(&iwb->writesem);
 	return write;
 }

@@ -93,6 +94,7 @@ static inline int isowbuf_donewrite(struct isowbuf_t *iwb)
 static inline void isowbuf_putbits(struct isowbuf_t *iwb, u32 data, int nbits)
 {
 	int write = iwb->write;
+
 	data <<= iwb->wbits;
 	data |= iwb->data[write];
 	nbits += iwb->wbits;

@@ -770,6 +772,7 @@ static inline void hdlc_unpack(unsigned char *src, unsigned count,
 					/* stuff bit at position lead1,
 					 * no interior stuffing */
 					unsigned char mask = (1 << lead1) - 1;
+
 					c = (c & mask) | ((c & ~mask) >> 1);
 					inbyte |= c << inbits;
 					inbits += 7;

drivers/isdn/gigaset/proc.c +11 -6

index e3f9d0f..10368a7 100644
@@ -27,13 +27,18 @@ static ssize_t set_cidmode(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
 	struct cardstate *cs = dev_get_drvdata(dev);
-	long int value;
-	char *end;
+	long int *value;
+	int result;
 
-	value = simple_strtol(buf, &end, 0);
-	while (*end)
-		if (!isspace(*end++))
-			return -EINVAL;
+	result = kstrtol(buf, 0, &value);
+	if (result == -ERANGE)
+		/* Overflow error */
+		dev_err(cs->dev, "Overflow error on conversion from string to\
+				 long\n");
+	if (result == -EINVAL)
+		/* Parsing error  */
+		dev_err(cs->dev, "Parsing error on conversion from string to\
+				 long\n");
 	if (value < 0 || value > 1)
 		return -EINVAL;

drivers/isdn/gigaset/usb-gigaset.c +33 ~~-13~~

60 lines not on this page · raw message

Signature · 1 line

1.8.4.5

Jeremiah Mahler

2014-12-31 17:49 UTC #1 lore raw

Bas,

Quoted 3 lines · Bas Peters

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.

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 16 lines

Signed-off-by Bas Peters <[email protected]>

---
 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(-)

drivers/isdn/gigaset/asyncdata.c +0 -0

[...]

Signature · 1 line

- Jeremiah Mahler

Bas Peters

author

2014-12-31 18:04 UTC #2 lore raw

2014-12-31 18:49 GMT+01:00 Jeremiah Mahler <[email protected]>:

Quoted 18 lines

Bas,
Quoted 3 lines · Bas Peters
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.
Quoted 9 lines
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.

Quoted 7 lines

Quoted 5 lines
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!

Quoted 21 lines

drivers/isdn/gigaset/asyncdata.c

Quoted 17 lines

Signed-off-by Bas Peters <[email protected]>

---
 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(-)

drivers/isdn/gigaset/asyncdata.c +0 -0

[...]

--
- Jeremiah Mahler

Jeremiah Mahler

2014-12-31 18:32 UTC #3 lore raw

Bas,

Quoted 27 lines · Bas Peters

2014-12-31 18:49 GMT+01:00 Jeremiah Mahler <[email protected]>:
Quoted 18 lines
Bas,
Quoted 3 lines · Bas Peters
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.
Quoted 9 lines
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.
Quoted 7 lines
Quoted 5 lines
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.

Quoted 9 lines

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".

Quoted 21 lines

drivers/isdn/gigaset/asyncdata.c

Quoted 17 lines

Signed-off-by Bas Peters <[email protected]>

---
 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(-)

drivers/isdn/gigaset/asyncdata.c +0 -0

[...]

--
- Jeremiah Mahler

Signature · 1 line

- Jeremiah Mahler

Tilman Schmidt

2015-01-01 18:46 UTC #4 lore raw

In reply to Bas Peters

-----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:

Quoted 3 lines

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.

Quoted 8 lines

--- 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.

Quoted 11 lines

(quoted diff) +0 -0
@@ -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?

Quoted 5 lines

--- 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).

Quoted 7 lines

--- a/drivers/isdn/gigaset/ev-layer.c +++
b/drivers/isdn/gigaset/ev-layer.c
(quoted diff) +0 -0
@@ -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,

Quoted 7 lines

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.

Quoted 9 lines

--- 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.

Quoted 7 lines

--- a/drivers/isdn/gigaset/i4l.c +++ b/drivers/isdn/gigaset/i4l.c
(quoted diff) +0 -1
@@ -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.

Quoted 8 lines

--- a/drivers/isdn/gigaset/proc.c +++ b/drivers/isdn/gigaset/proc.c @@ -27,13 +27,18 @@ static ssize_t set_cidmode(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct cardstate *cs =
dev_get_drvdata(dev); - long int value; - char *end; + long int
*value; + int result;
(quoted diff) +0 -1
-	value = simple_strtol(buf, &end, 0); -	while (*end) -		if

Quoted 6 lines

(!isspace(*end++)) - return -EINVAL; + result = kstrtol(buf, 0,
&value); + if (result == -ERANGE) + /* Overflow error */ +
dev_err(cs->dev, "Overflow error on conversion from string to\ +
long\n"); + if (result == -EINVAL) + /* Parsing error */ +
dev_err(cs->dev, "Parsing error on conversion from string to\ + long\n"); if (value < 0 || value > 1) return -EINVAL;

This changes semantics. Your code will not accept the same input as the original code, and it will emit messages of its own instead of just returning an error code to the caller as it should.

Quoted 7 lines

--- a/drivers/isdn/gigaset/usb-gigaset.c +++
b/drivers/isdn/gigaset/usb-gigaset.c
default: rate = 9600; - dev_err(cs->dev, "unsupported baudrate
request 0x%x," - " using default of B9600\n", cflag); +
dev_err(cs->dev, "unsupported baudrate request 0x%x,\ + using
default of B9600\n", cflag);

This makes the message much less readable by inserting a long stretch of whitespace after the comma.

In sum: NACK.

Regards,
Tilman

- --
Tilman Schmidt E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJUpZYFAAoJEFPuqx0v+F+qEcsH/1yyu8A8tHPhiW60DzQWhCj7 kxKw7gUS24ATLEEl5jUmrua2xPM0Exg7FknBSYRmNmOEj8j3sl7mQ0dDzDcJgOgI BaDXV5YqnnqppmYkdT7OMykAuhdt2rk1w4khc2EjyyKrAdGJyB+j3ROgRDtG0wsY zI/Uz7yKe540cwVWc6VCNQvS7cVEasQZnJzzTGBcPW35RjTYpvWEieJ/yY3tphIe TQRl+SrKgiwGuzi0p886Vk8Mu4cfHHO5/EXyzpVdMVg6wxwxNs+YeW5xRf/mjzQe YyygRKUA4VtZTno/rabdA2QvLkdDMHoiUNYa0InmBpAlLVol8OLh5mTit9yozwY= =uU+S
-----END PGP SIGNATURE-----

Tilman Schmidt

2015-01-03 15:01 UTC #5 lore raw

In reply to Bas Peters

[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:

Quoted 3 lines

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.

Quoted 4 lines

drivers/isdn/gigaset/bas-gigaset.c +0 -0

@@ -261,11 +261,12 @@ static inline void dump_urb(enum debuglevel level, const char *tag,
 {

drivers/isdn/gigaset/bas-gigaset.c static inline void dump_urb(enum debuglevel level, const char *tag, · line 269

Quoted 10 lines

drivers/isdn/gigaset/bas-gigaset.c +3 -2

 #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.)

drivers/isdn/gigaset/bas-gigaset.c static int atread_submit(struct cardstate *cs, int timeout) · line 570

Quoted 8 lines

drivers/isdn/gigaset/bas-gigaset.c +2 -2

@@ -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.)

Quoted 5 lines

drivers/isdn/gigaset/bas-gigaset.c +0 -1

@@ -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",

drivers/isdn/gigaset/bas-gigaset.c static int gigaset_probe(struct usb_interface *interface, · line 2323

Quoted 10 lines

drivers/isdn/gigaset/bas-gigaset.c +2 -1

+		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?

drivers/isdn/gigaset/capi.c static void do_connect_req(struct gigaset_capi_ctr *iif, · line 1376

Quoted 9 lines

drivers/isdn/gigaset/capi.c +1 -1

@@ -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.)

drivers/isdn/gigaset/common.c void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg, · line 56

Quoted 8 lines

drivers/isdn/gigaset/common.c +1 -1

@@ -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).

Quoted 36 lines

drivers/isdn/gigaset/ev-layer.c +15 -3

@@ -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)

drivers/isdn/gigaset/ev-layer.c static void do_action(int action, struct cardstate *cs, · line 1378

Quoted 8 lines

drivers/isdn/gigaset/ev-layer.c +1 -1

@@ -1364,7 +1372,7 @@ static void do_action(int action, struct cardstate *cs,
 			} else if (*e != '.')
 				break;
 			else
-				s = e + 1;
+				s = *e + 1;
 			cs->fwver[i] = val;
 		}
 		if (i != 4) {

drivers/isdn/gigaset/gigaset.h enum debuglevel { · line 98

Quoted 10 lines

drivers/isdn/gigaset/gigaset.h +1 -2

@@ -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)

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.

drivers/isdn/gigaset/i4l.c int gigaset_isdn_regdev(struct cardstate *cs, const char *isdnid) · line 627

Quoted 9 lines

drivers/isdn/gigaset/i4l.c +1 -1

@@ -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));

You're calling kmalloc with too few arguments here.

drivers/isdn/gigaset/i4l.c int gigaset_isdn_regdev(struct cardstate *cs, const char *isdnid) · line 634

Quoted 9 lines

drivers/isdn/gigaset/i4l.c +2 -2

 	if (!iif) {
 		pr_err("out of memory\n");
 		return -ENOMEM;
 	}
 
-	if (snprintf(iif->id, sizeof iif->id, "%s_%u", isdnid, cs->minor_index)
-	    >= sizeof iif->id) {
+	if (snprintf(iif->id, sizeof(iif->id, "%s_%u", isdnid, cs->minor_index))
+	    >= sizeof(iif->id)) {

You're calling snprintf with too few arguments here.

Quoted 17 lines

drivers/isdn/gigaset/proc.c +4 -6

@@ -27,13 +27,18 @@ static ssize_t set_cidmode(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
 	struct cardstate *cs = dev_get_drvdata(dev);
-	long int value;
-	char *end;
+	long int *value;
+	int result;
 
-	value = simple_strtol(buf, &end, 0);
-	while (*end)
-		if (!isspace(*end++))
-			return -EINVAL;
+	result = kstrtol(buf, 0, &value);
+	if (result == -ERANGE)

drivers/isdn/gigaset/proc.c static ssize_t set_cidmode(struct device *dev, struct device_attribute *attr, · line 44

Quoted 10 lines

drivers/isdn/gigaset/proc.c +7 -0

+		/* Overflow error */
+		dev_err(cs->dev, "Overflow error on conversion from string to\
+				 long\n");
+	if (result == -EINVAL)
+		/* Parsing error  */
+		dev_err(cs->dev, "Parsing error on conversion from string to\
+				 long\n");
 	if (value < 0 || value > 1)
 		return -EINVAL;

This changes semantics. Your code will not accept the same input as the original code, and it will emit messages of its own instead of just returning an error code to the caller as it should.

In sum: NACK.

Regards,
Tilman

Signature · 4 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

Bas Peters

author

2015-01-05 12:34 UTC #6 lore raw

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 <[email protected]>:

Quoted 231 lines

[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:
Quoted 3 lines
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.
Quoted 4 lines
drivers/isdn/gigaset/bas-gigaset.c +0 -0
@@ -261,11 +261,12 @@ static inline void dump_urb(enum debuglevel level, const char *tag,
 {
drivers/isdn/gigaset/bas-gigaset.c static inline void dump_urb(enum debuglevel level, const char *tag, · line 269
Quoted 10 lines
drivers/isdn/gigaset/bas-gigaset.c +3 -2
 #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.)
drivers/isdn/gigaset/bas-gigaset.c static int atread_submit(struct cardstate *cs, int timeout) · line 570
Quoted 8 lines
drivers/isdn/gigaset/bas-gigaset.c +2 -2
@@ -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.)
Quoted 5 lines
drivers/isdn/gigaset/bas-gigaset.c +0 -1
@@ -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",
drivers/isdn/gigaset/bas-gigaset.c static int gigaset_probe(struct usb_interface *interface, · line 2323
Quoted 10 lines
drivers/isdn/gigaset/bas-gigaset.c +2 -1
+             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?
drivers/isdn/gigaset/capi.c static void do_connect_req(struct gigaset_capi_ctr *iif, · line 1376
Quoted 9 lines
drivers/isdn/gigaset/capi.c +1 -1
@@ -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.)
drivers/isdn/gigaset/common.c void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg, · line 56
Quoted 8 lines
drivers/isdn/gigaset/common.c +1 -1
@@ -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).
Quoted 36 lines
drivers/isdn/gigaset/ev-layer.c +15 -3
@@ -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)
drivers/isdn/gigaset/ev-layer.c static void do_action(int action, struct cardstate *cs, · line 1378
Quoted 8 lines
drivers/isdn/gigaset/ev-layer.c +1 -1
@@ -1364,7 +1372,7 @@ static void do_action(int action, struct cardstate *cs,
                      } else if (*e != '.')
                              break;
                      else
-                             s = e + 1;
+                             s = *e + 1;
                      cs->fwver[i] = val;
              }
              if (i != 4) {
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.
drivers/isdn/gigaset/gigaset.h enum debuglevel { · line 98
Quoted 10 lines
drivers/isdn/gigaset/gigaset.h +1 -2
@@ -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)
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.
drivers/isdn/gigaset/i4l.c int gigaset_isdn_regdev(struct cardstate *cs, const char *isdnid) · line 627
Quoted 9 lines
drivers/isdn/gigaset/i4l.c +1 -1
@@ -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));
You're calling kmalloc with too few arguments here.
drivers/isdn/gigaset/i4l.c int gigaset_isdn_regdev(struct cardstate *cs, const char *isdnid) · line 634
Quoted 9 lines
drivers/isdn/gigaset/i4l.c +2 -2
      if (!iif) {
              pr_err("out of memory\n");
              return -ENOMEM;
      }
 
-     if (snprintf(iif->id, sizeof iif->id, "%s_%u", isdnid, cs->minor_index)
-         >= sizeof iif->id) {
+     if (snprintf(iif->id, sizeof(iif->id, "%s_%u", isdnid, cs->minor_index))
+         >= sizeof(iif->id)) {
You're calling snprintf with too few arguments here.
Quoted 17 lines
drivers/isdn/gigaset/proc.c +4 -6
@@ -27,13 +27,18 @@ static ssize_t set_cidmode(struct device *dev, struct device_attribute *attr,
                         const char *buf, size_t count)
 {
      struct cardstate *cs = dev_get_drvdata(dev);
-     long int value;
-     char *end;
+     long int *value;
+     int result;
 
-     value = simple_strtol(buf, &end, 0);
-     while (*end)
-             if (!isspace(*end++))
-                     return -EINVAL;
+     result = kstrtol(buf, 0, &value);
+     if (result == -ERANGE)
drivers/isdn/gigaset/proc.c static ssize_t set_cidmode(struct device *dev, struct device_attribute *attr, · line 44
Quoted 10 lines
drivers/isdn/gigaset/proc.c +7 -0
+             /* Overflow error */
+             dev_err(cs->dev, "Overflow error on conversion from string to\
+                              long\n");
+     if (result == -EINVAL)
+             /* Parsing error  */
+             dev_err(cs->dev, "Parsing error on conversion from string to\
+                              long\n");
      if (value < 0 || value > 1)
              return -EINVAL;
 
This changes semantics. Your code will not accept the same input as the original code, and it will emit messages of its own instead of just returning an error code to the caller as it should.
In sum: NACK.
Regards,
Tilman
--
Tilman Schmidt E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite)