Re: [PATCH]: add debug flag parameter for SCSI tape driver - 2nd request

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Kai

Thanks. 

Here is v3

This patch adds a debug_flag parameter that can be set on module load, and allows the DEBUG facility without a module recompile.
Note that now DEBUG 1 is the default with this patch.

Usage: modprobe st debug_flag=1

Signed-off-by: Laurence Oberman <loberman@xxxxxxxxxx>

diff -Nur a/Documentation/scsi/st.txt b/Documentation/scsi/st.txt
--- a/Documentation/scsi/st.txt	2014-10-19 09:36:52.243863716 -0400
+++ b/Documentation/scsi/st.txt	2014-10-19 09:43:19.222863447 -0400
@@ -506,9 +506,11 @@
 
 DEBUGGING HINTS
 
-To enable debugging messages, edit st.c and #define DEBUG 1. As seen
-above, debugging can be switched off with an ioctl if debugging is
-compiled into the driver. The debugging output is not voluminous.
+Debugging code is now compiled in by default but debugging is turned off with 
+the kernel module parameter debug_flag defaulting to 0.
+Debugging can still be switched on and off with an ioctl.
+To enable debug at module load time add debug_flag=1 to the module load 
+options, the debugging output is not voluminous.
 
 If the tape seems to hang, I would be very interested to hear where
 the driver is waiting. With the command 'ps -l' you can see the state

diff -Nur a/drivers/scsi/st.c b/drivers/scsi/st.c
--- a/drivers/scsi/st.c	2014-10-19 09:35:45.673863756 -0400
+++ b/drivers/scsi/st.c	2014-10-19 09:35:30.621863483 -0400
@@ -56,7 +56,8 @@
 
 /* The driver prints some debugging information on the console if DEBUG
    is defined and non-zero. */
-#define DEBUG 0
+#define DEBUG 1
+#define NO_DEBUG 0
 
 #define ST_DEB_MSG  KERN_NOTICE
 #if DEBUG
@@ -80,6 +81,7 @@
 static int try_direct_io = TRY_DIRECT_IO;
 static int try_rdio = 1;
 static int try_wdio = 1;
+static int debug_flag = 0;
 
 static struct class st_sysfs_class;
 static const struct attribute_group *st_dev_groups[];
@@ -100,6 +102,9 @@
 MODULE_PARM_DESC(max_sg_segs, "Maximum number of scatter/gather segments to use (256)");
 module_param_named(try_direct_io, try_direct_io, int, 0);
 MODULE_PARM_DESC(try_direct_io, "Try direct I/O between user buffer and tape drive (1)");
+module_param_named(debug_flag, debug_flag, int, 0);
+MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=1");
+
 
 /* Extra parameters for testing */
 module_param_named(try_rdio, try_rdio, int, 0);
@@ -124,6 +129,9 @@
 	},
 	{
 		"try_direct_io", &try_direct_io
+        },
+        {
+                "debug_flag", &debug_flag
 	}
 };
 #endif
@@ -4309,6 +4317,10 @@
 	printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
 		verstr, st_fixed_buffer_size, st_max_sg_segs);
 
+        debugging = (debug_flag > 0) ? debug_flag : NO_DEBUG;
+         if (debugging)
+                printk(KERN_INFO "st: Debugging enabled debug_flag = %d\n",debugging);
+
 	err = class_register(&st_sysfs_class);
 	if (err) {
 		pr_err("Unable register sysfs class for SCSI tapes\n");


----- Original Message -----
From: "Kai Mäkisara (Kolumbus)" <kai.makisara@xxxxxxxxxxx>
To: "Laurence Oberman" <loberman@xxxxxxxxxx>
Cc: "Rob Evers" <revers@xxxxxxxxxx>, linux-scsi@xxxxxxxxxxxxxxx
Sent: Sunday, October 19, 2014 4:54:10 AM
Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver - 2nd request

Hello,

I am responding to this, but noticed your next, fixed version.

> On 17.10.2014, at 23.20, Laurence Oberman <loberman@xxxxxxxxxx> wrote:
> 
> Hello Kai
> 
> You have seen this patch before. The first time around, given that we don't enable DEBUG by default, I let it go.
> However we have been looking into defining DEBUG 1 by default here at Redhat and then setting the default to disabled.
> 
> Are you open to considering changing the driver based on this patch.
> i.e. default DEFINE 1 and adding this code with default set to off.
> 
Yes. I certainly think defining DEBUG 1 and changing the default to zero should be done if it is useful for supporting users. The runtime overhead is negligible and the extra code does not matter nowadays (it did matter, at least theoretically, for years ago).

I am not so sure about the module option. When the debugging code is compiled in, debugging can be enabled and disabled for each device by the MTIOCTOP ioctl (e.g., mtst -f tape_device stsetoptions debug). The module option enables debugging for all tape devices. However, if you think this additional module option is useful, I am not against it. It does not remove the possibility for controlling debugging for each device for those who want to do it that way.

Anyway, you should modify the documentation (Documentation/scsi/st.txt) according to the changes.

> Note that with DEBUG 0, as you know you need to change that and recompile. 
> That is exactly what I am trying to avoid with Enterprise customers.
> 
I have also noticed this when someone has asked me about some tape problems.

> This patch adds a debug_flag parameter that can be set on module load, and allows the DEBUG facility without a module recompile.
> Note that now DEBUG 1 is the default with this patch.
> 
> Usage: modprobe st debug_flag=1
> 
> Signed-off-by: Laurence Oberman <loberman@xxxxxxxxxx>
> 
> diff -Nur a/st.c b/st.c
> --- a/st.c	2014-10-17 16:15:54.103810627 -0400
> +++ b/st.c	2014-10-17 16:22:12.303810392 -0400
> @@ -56,7 +56,7 @@
> 
> /* The driver prints some debugging information on the console if DEBUG
>    is defined and non-zero. */
> -#define DEBUG 0
> +#define DEBUG 1
> 
> #define ST_DEB_MSG  KERN_NOTICE
> #if DEBUG
> @@ -80,6 +80,7 @@
> static int try_direct_io = TRY_DIRECT_IO;
> static int try_rdio = 1;
> static int try_wdio = 1;
> +static int debug_flag = 0;
> 
> static struct class st_sysfs_class;
> static const struct attribute_group *st_dev_groups[];
> @@ -100,6 +101,9 @@
> MODULE_PARM_DESC(max_sg_segs, "Maximum number of scatter/gather segments to use (256)");
> module_param_named(try_direct_io, try_direct_io, int, 0);
> MODULE_PARM_DESC(try_direct_io, "Try direct I/O between user buffer and tape drive (1)");
> +module_param_named(debug_flag, debug_flag, int, 0);
> +MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=1");
> +
> 
> /* Extra parameters for testing */
> module_param_named(try_rdio, try_rdio, int, 0);
> @@ -124,6 +128,9 @@
> 	},
> 	{
> 		"try_direct_io", &try_direct_io
> +        },
> +        {
> +                "debug_flag", &debug_flag
> 	}
> };
> #endif
> @@ -4306,6 +4313,11 @@
> 
> 	validate_options();
> 
> +        debugging = (debug_flag > 0) ? debug_flag : DEBUG;
> +         if (debugging)
> +                printk(KERN_INFO "st: Debugging enabled debug_flag = %d\n",debugging);
> +
> +

I think you have an extra newline here?

I also think that the kernel log would look nicer if the print below would be before setting the option. The driver would introduce itself first and print the debug flag status after that.

> 	printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
> 		verstr, st_fixed_buffer_size, st_max_sg_segs);

Thanks,
Kai

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux