[PATCH]: MUX n_gsm debug print improvements

[PATCH]: MUX n_gsm debug print improvements

[Gwenn Bourrée]

Dear kernel tty maintainers,

Please review the following patch:

From 0ac5da0a4653f43ce4b0761a2be8073185c549bb Mon Sep 17 00:00:00 2001
From: Gwenn Bourree <gwenn.bourree@intel.com>
Date: Mon, 29 Jun 2015 16:09:06 +0200
Subject: [PATCH] Add Debug define

Improve the debug print out and make be clearest

Signed-off-by: Gwenn Bourree <gwenn.bourree@intel.com>
Signed-off-by: Mustapha Ben Zoubeir <mustaphax.ben.zoubeir@intel.com>
Signed-off-by: Nicolas LOUIS <nicolasx.louis@intel.com>
Reviewed-by: Ravindran, Arun <arun.ravindran@intel.com

---
 drivers/tty/n_gsm.c | 49
++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 382d3fc..d6e0ea0 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -66,17 +66,18 @@
 static int debug;
 module_param(debug, int, 0600);
 
-/* Defaults: these are from the specification */
+#define GSMDBG_VERBOSE_PACKET_REPORT(x) ((x) &  1)
+#define GSMDBG_FORCE_CARRIER(x)         ((x) &  2)
+#define GSMDBG_DATA_FULL_REPORT(x)      ((x) &  4)
+#define GSMDBG_DLCI_STREAM_REPORT(x)    ((x) &  8)
+#define GSMDBG_DLCI_DATA_REPORT(x)      ((x) & 16)
+#define GSMDBG_DATA_LEN_REPORT(x)       ((x) & 32)
 
-#define T1	10		/* 100mS */
-#define T2	34		/* 333mS */
-#define N2	3		/* Retry 3 times */
+/* Defaults: these are from the specification */
 
-/* Use long timers for testing at low speed with debug on */
-#ifdef DEBUG_TIMING
-#define T1	100
-#define T2	200
-#endif
+#define T1     10              /* 100mS */
+#define T2     34              /* 333mS */
+#define N2     3               /* Retry 3 times */
 
 /*
  * Semi-arbitrary buffer size limits. 0710 is normally run with 32-64
byte
@@ -461,7 +462,7 @@ static u8 gsm_encode_modem(const struct gsm_dlci
*dlci)
 static void gsm_print_packet(const char *hdr, int addr, int cr,
 					u8 control, const u8 *data, int dlen)
 {
-	if (!(debug & 1))
+	if (!GSMDBG_VERBOSE_PACKET_REPORT(debug))
 		return;
 
 	pr_info("%s %d) %c: ", hdr, addr, "RC"[cr]);
@@ -700,7 +701,7 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 			len = msg->len + 2;
 		}
 
-		if (debug & 4)
+		if (GSMDBG_DATA_FULL_REPORT(debug))
 			print_hex_dump_bytes("gsm_data_kick: ",
 					     DUMP_PREFIX_OFFSET,
 					     gsm->txframe, len);
@@ -1426,7 +1427,7 @@ static int gsm_control_wait(struct gsm_mux *gsm,
struct gsm_control *control)
 static void gsm_dlci_close(struct gsm_dlci *dlci)
 {
 	del_timer(&dlci->t1);
-	if (debug & 8)
+	if (GSMDBG_DLCI_STREAM_REPORT(debug))
 		pr_debug("DLCI %d goes closed.\n", dlci->addr);
 	dlci->state = DLCI_CLOSED;
 	if (dlci->addr != 0) {
@@ -1453,7 +1454,7 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 	del_timer(&dlci->t1);
 	/* This will let a tty open continue */
 	dlci->state = DLCI_OPEN;
-	if (debug & 8)
+	if (GSMDBG_DLCI_STREAM_REPORT(debug))
 		pr_debug("DLCI %d goes open.\n", dlci->addr);
 	wake_up(&dlci->gsm->event);
 }
@@ -1556,8 +1557,8 @@ static void gsm_dlci_data(struct gsm_dlci *dlci,
u8 *data, int clen)
 	unsigned int modem = 0;
 	int len = clen;
 
-	if (debug & 16)
-		pr_debug("%d bytes for tty\n", len);
+	if (GSMDBG_DLCI_DATA_REPORT(debug))
+		pr_debug("%s: %d bytes for tty\n", __func__, len);
 	switch (dlci->adaption)  {
 	/* Unsupported types */
 	/* Packetised interruptible data */
@@ -1758,7 +1759,7 @@ static void gsm_queue(struct gsm_mux *gsm)
 	}
 	if (gsm->fcs != GOOD_FCS) {
 		gsm->bad_fcs++;
-		if (debug & 4)
+		if (GSMDBG_DATA_FULL_REPORT(debug))
 			pr_debug("BAD FCS %02x\n", gsm->fcs);
 		return;
 	}
@@ -1922,6 +1923,7 @@ static void gsm0_receive(struct gsm_mux *gsm,
unsigned char c)
 			gsm->state = GSM_SEARCH;
 			break;
 		}
+		pr_debug("wait for GSM0_SOF, while got 0x%x\n", (u32)c);
 		break;
 	}
 }
@@ -2209,9 +2211,12 @@ static int gsmld_output(struct gsm_mux *gsm, u8
*data, int len)
 		set_bit(TTY_DO_WRITE_WAKEUP, &gsm->tty->flags);
 		return -ENOSPC;
 	}
-	if (debug & 4)
-		print_hex_dump_bytes("gsmld_output: ", DUMP_PREFIX_OFFSET,
+	if (GSMDBG_DATA_FULL_REPORT(debug))
+		print_hex_dump_bytes(__func__, DUMP_PREFIX_OFFSET,
 				     data, len);
+	else if (GSMDBG_DATA_LEN_REPORT(debug))
+		pr_debug("n_gsm: >> %d bytes\n", len);
+
 	gsm->tty->ops->write(gsm->tty, data, len);
 	return len;
 }
@@ -2276,9 +2281,11 @@ static void gsmld_receive_buf(struct tty_struct
*tty, const unsigned char *cp,
 	int i;
 	char flags = TTY_NORMAL;
 
-	if (debug & 4)
-		print_hex_dump_bytes("gsmld_receive: ", DUMP_PREFIX_OFFSET,
+	if (GSMDBG_DATA_FULL_REPORT(debug))
+		print_hex_dump_bytes(__func__, DUMP_PREFIX_OFFSET,
 				     cp, count);
+	else if (GSMDBG_DATA_LEN_REPORT(debug))
+		pr_debug("n_gsm: << %d bytes\n", count);
 
 	for (i = count, dp = cp, f = fp; i; i--, dp++) {
 		if (f)
@@ -2871,7 +2878,7 @@ static int gsm_carrier_raised(struct tty_port
*port)
 	/* Not yet open so no carrier info */
 	if (dlci->state != DLCI_OPEN)
 		return 0;
-	if (debug & 2)
+	if (GSMDBG_FORCE_CARRIER(debug))
 		return 1;
 	return dlci->modem_rx & TIOCM_CD;
 }


---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH]: MUX n_gsm debug print improvements

[Greg KH]

On Mon, Jun 29, 2015 at 05:54:58PM +0200, Gwenn Bourrée wrote:
> 
> Dear kernel tty maintainers,
> 
> Please review the following patch:
> 
> From 0ac5da0a4653f43ce4b0761a2be8073185c549bb Mon Sep 17 00:00:00 2001
> From: Gwenn Bourree <gwenn.bourree-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Mon, 29 Jun 2015 16:09:06 +0200
> Subject: [PATCH] Add Debug define
> 
> Improve the debug print out and make be clearest
> 
> Signed-off-by: Gwenn Bourree <gwenn.bourree-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mustapha Ben Zoubeir <mustaphax.ben.zoubeir-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Nicolas LOUIS <nicolasx.louis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Ravindran, Arun <arun.ravindran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Any reason you didn't cc the linux-serial mailing list?  And linux-api
only cares about external api changes, which I don't think you did here.

Also, what's with the odd format, I would have to hand-edit this in
order to apply it properly.  Please fix it up and send it without the
"Dear..." lines, that's not needed at all, nor is the "From" lines.

thanks,

greg k-h

Re: [PATCH]: MUX n_gsm debug print improvements

[Greg KH]

On Mon, Jun 29, 2015 at 05:54:58PM +0200, Gwenn Bourrée wrote:
> 
> Dear kernel tty maintainers,
> 
> Please review the following patch:
> 
> From 0ac5da0a4653f43ce4b0761a2be8073185c549bb Mon Sep 17 00:00:00 2001
> From: Gwenn Bourree <gwenn.bourree-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Mon, 29 Jun 2015 16:09:06 +0200
> Subject: [PATCH] Add Debug define
> 
> Improve the debug print out and make be clearest
> 
> Signed-off-by: Gwenn Bourree <gwenn.bourree-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mustapha Ben Zoubeir <mustaphax.ben.zoubeir-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Nicolas LOUIS <nicolasx.louis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Ravindran, Arun <arun.ravindran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> 
> ---
>  drivers/tty/n_gsm.c | 49
> ++++++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)

Also, your patch is line-wrapped and also base64 encoded, making it
impossible to edit, or apply.  Please fix up your email client to use
the proper configuration, as I'm sure Intel has documentation for how to
do.

In fact, it has documentation for how to do all of this, why aren't you
following it?

thanks,

greg k-h

Re: [PATCH]: MUX n_gsm debug print improvements

[Marcel Holtmann]

Hi Gwenn,

> Please review the following patch:
> 
> From 0ac5da0a4653f43ce4b0761a2be8073185c549bb Mon Sep 17 00:00:00 2001
> From: Gwenn Bourree <gwenn.bourree-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Mon, 29 Jun 2015 16:09:06 +0200
> Subject: [PATCH] Add Debug define
> 
> Improve the debug print out and make be clearest
> 
> Signed-off-by: Gwenn Bourree <gwenn.bourree-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mustapha Ben Zoubeir <mustaphax.ben.zoubeir-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Nicolas LOUIS <nicolasx.louis-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Ravindran, Arun <arun.ravindran-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> 
> ---
> drivers/tty/n_gsm.c | 49
> ++++++++++++++++++++++++++++---------------------
> 1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 382d3fc..d6e0ea0 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -66,17 +66,18 @@
> static int debug;
> module_param(debug, int, 0600);
> 
> -/* Defaults: these are from the specification */
> +#define GSMDBG_VERBOSE_PACKET_REPORT(x) ((x) &  1)
> +#define GSMDBG_FORCE_CARRIER(x)         ((x) &  2)
> +#define GSMDBG_DATA_FULL_REPORT(x)      ((x) &  4)
> +#define GSMDBG_DLCI_STREAM_REPORT(x)    ((x) &  8)
> +#define GSMDBG_DLCI_DATA_REPORT(x)      ((x) & 16)
> +#define GSMDBG_DATA_LEN_REPORT(x)       ((x) & 32)

what is actually wrong with just using dynamic debug. That lets you easily switch on certain debug statements. I would actually remove all of this and rely completely on dynamic debug instead.

Anyway, there are also BIT(0), BIT(1) etc. choices that would be a lot clearer than 1, 2, 4 etc.

Regards

Marcel