Discussion:
Debug flag parameter for SCSI tape driver
(too old to reply)
Laurence Oberman
2014-06-10 20:51:15 UTC
Permalink
Raw Message
Hello

I am tired of building modules to enable SCSI tape driver debug so I am hoping this patch is acceptable.
Tested using kernel 3.14.6

Usage example:
modprobe st debug_flag=1

diff -Nur a/st.c b/st.c
--- a/st.c 2014-06-10 16:45:18.522354105 -0400
+++ b/st.c 2014-06-10 16:45:33.953765908 -0400
@@ -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 DEBUG 1 in source");
+

/* 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
@@ -4277,7 +4284,9 @@
static int __init init_st(void)
{
int err;
-
+ debugging = (debug_flag > 0) ? debug_flag : DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag = %d\n",debugging);
validate_options();

printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",

Thanks
Laurence
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Laurence Oberman
2014-06-10 23:48:33 UTC
Permalink
Raw Message
Hello

Take 2 of this patch, changed module description and subject line.

This patch adds a debug_flag parameter that can be set on module load, and allows the DEBUG facility without a module recompile.
Usage: mpdprobe st debug_flag=1

Signed-off-by: Laurence Oberman <***@redhat.com>

diff -Nur a/st.c b/st.c
--- a/st.c 2014-06-10 16:45:18.522354105 -0400
+++ b/st.c 2014-06-10 19:40:39.774387990 -0400
@@ -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
@@ -4277,7 +4284,9 @@
static int __init init_st(void)
{
int err;
-
+ debugging = (debug_flag > 0) ? debug_flag : DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag = %d\n",debugging);
validate_options();

printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",


Thanks
Laurence
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kai Mäkisara (Kolumbus)
2014-06-11 18:03:15 UTC
Permalink
Raw Message
Post by Laurence Oberman
Hello
=20
Take 2 of this patch, changed module description and subject line.
=20
This patch adds a debug_flag parameter that can be set on module load=
, and allows the DEBUG facility without a module recompile.
Post by Laurence Oberman
Usage: mpdprobe st debug_flag=3D1
=20
=20
What is wrong with the existing methods to control debugging? You can e=
nable and disable debugging for each device with ioctl() (as described =
in the driver documentation). You can use mt-st to do this from command=
line.

Your patch just allows one to change the default for all devices. The r=
eal problem may be that the distributions don=92t compile the debugging=
code into the drivets but your patch does not change this.

Kai

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Laurence Oberman
2014-06-11 18:24:25 UTC
Permalink
Raw Message
Kai,
Thank you for considering this.

With #define DEBUG 0
We still include

#define DEB(a)
#define DEBC(a)

With the debug_flag we then provide the needed debug I am looking for a=
t module load time.
But I agree that it enables it for all devices and that may not be opti=
mal
I don't change the default, I just allow the parameter to control it.

In the last few issues I have been working I have had to recompile and =
provide the st module to get what I needed captured for debugging so I =
decided to try the patch submission.

Thank You
Laurence

----- Original Message -----
=46rom: "Kai M=C3=A4kisara (Kolumbus)" <***@kolumbus.fi>
To: "Laurence Oberman" <***@redhat.com>
Cc: linux-***@vger.kernel.org
Sent: Wednesday, June 11, 2014 2:03:15 PM
Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver
Post by Laurence Oberman
Hello
=20
Take 2 of this patch, changed module description and subject line.
=20
This patch adds a debug_flag parameter that can be set on module load=
, and allows the DEBUG facility without a module recompile.
Post by Laurence Oberman
Usage: mpdprobe st debug_flag=3D1
=20
=20
What is wrong with the existing methods to control debugging? You can e=
nable and disable debugging for each device with ioctl() (as described =
in the driver documentation). You can use mt-st to do this from command=
line.

Your patch just allows one to change the default for all devices. The r=
eal problem may be that the distributions don=E2=80=99t compile the deb=
ugging code into the drivets but your patch does not change this.

Kai

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dale R. Worley
2014-06-11 19:35:01 UTC
Permalink
Raw Message
This thread leads me to ask: Do people use ANSI-formatted tapes any
more? That is, tape volumes with miltiple files, header and trailer
labels, etc.?

Dale
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Laurence Oberman
2014-06-11 19:54:29 UTC
Permalink
Raw Message
Kai,

Its likely not worth doing this, I cross checked and indeed many distro=
s have this compiled out.
So lets leave it as is.

Thanks
Laurence

----- Original Message -----
=46rom: "Laurence Oberman" <***@redhat.com>
To: "Kai M=C3=A4kisara (Kolumbus)" <***@kolumbus.fi>
Cc: linux-***@vger.kernel.org
Sent: Wednesday, June 11, 2014 2:24:25 PM
Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver

Kai,
Thank you for considering this.

With #define DEBUG 0
We still include

#define DEB(a)
#define DEBC(a)

With the debug_flag we then provide the needed debug I am looking for a=
t module load time.
But I agree that it enables it for all devices and that may not be opti=
mal
I don't change the default, I just allow the parameter to control it.

In the last few issues I have been working I have had to recompile and =
provide the st module to get what I needed captured for debugging so I =
decided to try the patch submission.

Thank You
Laurence

----- Original Message -----
=46rom: "Kai M=C3=A4kisara (Kolumbus)" <***@kolumbus.fi>
To: "Laurence Oberman" <***@redhat.com>
Cc: linux-***@vger.kernel.org
Sent: Wednesday, June 11, 2014 2:03:15 PM
Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver
Post by Laurence Oberman
Hello
=20
Take 2 of this patch, changed module description and subject line.
=20
This patch adds a debug_flag parameter that can be set on module load=
, and allows the DEBUG facility without a module recompile.
Post by Laurence Oberman
Usage: mpdprobe st debug_flag=3D1
=20
=20
What is wrong with the existing methods to control debugging? You can e=
nable and disable debugging for each device with ioctl() (as described =
in the driver documentation). You can use mt-st to do this from command=
line.

Your patch just allows one to change the default for all devices. The r=
eal problem may be that the distributions don=E2=80=99t compile the deb=
ugging code into the drivets but your patch does not change this.

Kai

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dale R. Worley
2014-06-13 21:18:02 UTC
Permalink
Raw Message
Are ANSI-formatted tapes used on Linux? That is, tapes that contain
multiple files and use the the ANSI (= ECMA-13) tape labels. My
uderstanding is that a lot of the modern cartridge tapes record only
fixed-length blocks and/or that is how they are always used these
days.

Dale
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Laurence Oberman
2014-10-17 20:20:29 UTC
Permalink
Raw Message
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.

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.

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 <***@redhat.com>

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);
+
+
printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
verstr, st_fixed_buffer_size, st_max_sg_segs);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Laurence Oberman
2014-10-17 20:41:48 UTC
Permalink
Raw Message
Oops, patch was defaulting to 1.

Here is v2 properly defining DEBUG 1 and defaulting to 0 unless debug_f=
lag=3D1

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=20

Signed-off-by: Laurence Oberman <***@redhat.com>


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:42:45.992809531 -0400
@@ -56,7 +56,8 @@
=20
/* The driver prints some debugging information on the console if DEBU=
G
is defined and non-zero. */
-#define DEBUG 0
+#define DEBUG 1
+#define NO_DEBUG 0
=20
#define ST_DEB_MSG KERN_NOTICE
#if DEBUG
@@ -80,6 +81,7 @@
static int try_direct_io =3D TRY_DIRECT_IO;
static int try_rdio =3D 1;
static int try_wdio =3D 1;
+static int debug_flag =3D 0;
=20
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 segmen=
ts 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 an=
d tape drive (1)");
+module_param_named(debug_flag, debug_flag, int, 0);
+MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=3D=
1");
+
=20
/* 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
@@ -4306,6 +4314,11 @@
=20
validate_options();
=20
+ debugging =3D (debug_flag > 0) ? debug_flag : NO_DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag =3D=
%d\n",debugging);
+
+
printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
verstr, st_fixed_buffer_size, st_max_sg_segs);


----- Original Message -----
=46rom: "Laurence Oberman" <***@redhat.com>
To: "Kai M=C3=A4kisara (Kolumbus)" <***@kolumbus.fi>, "Rob Eve=
rs" <***@redhat.com>
Cc: linux-***@vger.kernel.org
Sent: Friday, October 17, 2014 4:20:29 PM
Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver - 2=
nd request

Hello Kai

You have seen this patch before. The first time around, given that we d=
on't enable DEBUG by default, I let it go.
However we have been looking into defining DEBUG 1 by default here at R=
edhat 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.

Note that with DEBUG 0, as you know you need to change that and recompi=
le.=20
That is exactly what I am trying to avoid with Enterprise customers.

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=3D1

Signed-off-by: Laurence Oberman <***@redhat.com>

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 @@
=20
/* The driver prints some debugging information on the console if DEBU=
G
is defined and non-zero. */
-#define DEBUG 0
+#define DEBUG 1
=20
#define ST_DEB_MSG KERN_NOTICE
#if DEBUG
@@ -80,6 +80,7 @@
static int try_direct_io =3D TRY_DIRECT_IO;
static int try_rdio =3D 1;
static int try_wdio =3D 1;
+static int debug_flag =3D 0;
=20
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 segmen=
ts 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 an=
d tape drive (1)");
+module_param_named(debug_flag, debug_flag, int, 0);
+MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=3D=
1");
+
=20
/* 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 @@
=20
validate_options();
=20
+ debugging =3D (debug_flag > 0) ? debug_flag : DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag =3D=
%d\n",debugging);
+
+
printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
verstr, st_fixed_buffer_size, st_max_sg_segs);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kai Mäkisara (Kolumbus)
2014-10-19 08:54:10 UTC
Permalink
Raw Message
Hello,

I am responding to this, but noticed your next, fixed version.
Post by Laurence Oberman
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.
Post by Laurence Oberman
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.
Post by Laurence Oberman
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
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.
Post by Laurence Oberman
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Laurence Oberman
2014-10-19 13:44:25 UTC
Permalink
Raw Message
Hello Kai

Thanks.=20

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=3D1

Signed-off-by: Laurence Oberman <***@redhat.com>

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 @@
=20
DEBUGGING HINTS
=20
-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 o=
ff with=20
+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=3D1 to the module l=
oad=20
+options, the debugging output is not voluminous.
=20
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 @@
=20
/* The driver prints some debugging information on the console if DEBU=
G
is defined and non-zero. */
-#define DEBUG 0
+#define DEBUG 1
+#define NO_DEBUG 0
=20
#define ST_DEB_MSG KERN_NOTICE
#if DEBUG
@@ -80,6 +81,7 @@
static int try_direct_io =3D TRY_DIRECT_IO;
static int try_rdio =3D 1;
static int try_wdio =3D 1;
+static int debug_flag =3D 0;
=20
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 segmen=
ts 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 an=
d tape drive (1)");
+module_param_named(debug_flag, debug_flag, int, 0);
+MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=3D=
1");
+
=20
/* 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);
=20
+ debugging =3D (debug_flag > 0) ? debug_flag : NO_DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag =3D=
%d\n",debugging);
+
err =3D class_register(&st_sysfs_class);
if (err) {
pr_err("Unable register sysfs class for SCSI tapes\n");


----- Original Message -----
=46rom: "Kai M=C3=A4kisara (Kolumbus)" <***@kolumbus.fi>
To: "Laurence Oberman" <***@redhat.com>
Cc: "Rob Evers" <***@redhat.com>, linux-***@vger.kernel.org
Sent: Sunday, October 19, 2014 4:54:10 AM
Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver - 2=
nd request

Hello,

I am responding to this, but noticed your next, fixed version.
Post by Laurence Oberman
=20
Hello Kai
=20
You have seen this patch before. The first time around, given that we=
don't enable DEBUG by default, I let it go.
Post by Laurence Oberman
However we have been looking into defining DEBUG 1 by default here at=
Redhat and then setting the default to disabled.
Post by Laurence Oberman
=20
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.
=20
Yes. I certainly think defining DEBUG 1 and changing the default to zer=
o should be done if it is useful for supporting users. The runtime over=
head 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 co=
mpiled in, debugging can be enabled and disabled for each device by the=
MTIOCTOP ioctl (e.g., mtst -f tape_device stsetoptions debug). The mod=
ule option enables debugging for all tape devices. However, if you thin=
k this additional module option is useful, I am not against it. It does=
not remove the possibility for controlling debugging for each device f=
or those who want to do it that way.

Anyway, you should modify the documentation (Documentation/scsi/st.txt)=
according to the changes.
Post by Laurence Oberman
Note that with DEBUG 0, as you know you need to change that and recom=
pile.=20
Post by Laurence Oberman
That is exactly what I am trying to avoid with Enterprise customers.
=20
I have also noticed this when someone has asked me about some tape prob=
lems.
Post by Laurence Oberman
This patch adds a debug_flag parameter that can be set on module load=
, and allows the DEBUG facility without a module recompile.
Post by Laurence Oberman
Note that now DEBUG 1 is the default with this patch.
=20
Usage: modprobe st debug_flag=3D1
=20
=20
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 @@
=20
/* The driver prints some debugging information on the console if DEB=
UG
Post by Laurence Oberman
is defined and non-zero. */
-#define DEBUG 0
+#define DEBUG 1
=20
#define ST_DEB_MSG KERN_NOTICE
#if DEBUG
@@ -80,6 +80,7 @@
static int try_direct_io =3D TRY_DIRECT_IO;
static int try_rdio =3D 1;
static int try_wdio =3D 1;
+static int debug_flag =3D 0;
=20
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 segme=
nts to use (256)");
Post by Laurence Oberman
module_param_named(try_direct_io, try_direct_io, int, 0);
MODULE_PARM_DESC(try_direct_io, "Try direct I/O between user buffer a=
nd tape drive (1)");
Post by Laurence Oberman
+module_param_named(debug_flag, debug_flag, int, 0);
+MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debuggin=
g=3D1");
Post by Laurence Oberman
+
=20
/* 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 @@
=20
validate_options();
=20
+ debugging =3D (debug_flag > 0) ? debug_flag : DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag =3D=
%d\n",debugging);
Post by Laurence Oberman
+
+
I think you have an extra newline here?

I also think that the kernel log would look nicer if the print below wo=
uld be before setting the option. The driver would introduce itself fir=
st and print the debug flag status after that.
Post by Laurence Oberman
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" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kai Mäkisara (Kolumbus)
2014-10-19 19:37:21 UTC
Permalink
Raw Message
Post by Laurence Oberman
=20
Hello Kai
=20
Thanks.=20
=20
Here is v3
=20
This patch adds a debug_flag parameter that can be set on module load=
, and allows the DEBUG facility without a module recompile.
Post by Laurence Oberman
Note that now DEBUG 1 is the default with this patch.
=20
Usage: modprobe st debug_flag=3D1
=20
Acked-by: Kai M=C3=A4kisara <***@kolumbus.fi>

Thanks,
Kai
Post by Laurence Oberman
=20
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 @@
=20
DEBUGGING HINTS
=20
-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=20
Post by Laurence Oberman
+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=3D1 to the module=
load=20
Post by Laurence Oberman
+options, the debugging output is not voluminous.
=20
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
=20
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 @@
=20
/* The driver prints some debugging information on the console if DEB=
UG
Post by Laurence Oberman
is defined and non-zero. */
-#define DEBUG 0
+#define DEBUG 1
+#define NO_DEBUG 0
=20
#define ST_DEB_MSG KERN_NOTICE
#if DEBUG
@@ -80,6 +81,7 @@
static int try_direct_io =3D TRY_DIRECT_IO;
static int try_rdio =3D 1;
static int try_wdio =3D 1;
+static int debug_flag =3D 0;
=20
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 segme=
nts to use (256)");
Post by Laurence Oberman
module_param_named(try_direct_io, try_direct_io, int, 0);
MODULE_PARM_DESC(try_direct_io, "Try direct I/O between user buffer a=
nd tape drive (1)");
Post by Laurence Oberman
+module_param_named(debug_flag, debug_flag, int, 0);
+MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debuggin=
g=3D1");
Post by Laurence Oberman
+
=20
/* 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);
=20
+ debugging =3D (debug_flag > 0) ? debug_flag : NO_DEBUG;
+ if (debugging)
+ printk(KERN_INFO "st: Debugging enabled debug_flag =3D=
%d\n",debugging);
Post by Laurence Oberman
+
err =3D class_register(&st_sysfs_class);
if (err) {
pr_err("Unable register sysfs class for SCSI tapes\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" i=
n
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-10-23 17:11:33 UTC
Permalink
Raw Message
Thanks,

I've applied this patch to the core-for-3.19 branch after fixing a few
whitespace issues.

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