On 2019-08-12 10:23 a.m., Christoph Hellwig wrote:
On Wed, Aug 07, 2019 at 01:42:35PM +0200, Douglas Gilbert wrote:
Replace SCSI_LOG_TIMEOUT macros with SG_LOG macros across the driver.
The definition of SG_LOG calls SCSI_LOG_TIMEOUT if given and derived
pointers are non-zero, calls pr_info otherwise. SG_LOGS additionally
prints the sg device name and the thread id. The thread id is very
useful, even in single threaded invocations because the driver not
only uses the invocer's thread but also uses work queues and the
main callback (i.e. sg_rq_end_io()) may hit any thread. Some
interesting cases arise when the callback hits its invocer's
thread.
SG_LOGS takes 48 bytes on the stack to build this printf format
string: "sg%u: tid=%d" whose size is clearly bounded above by
the maximum size of those two integers.
Protecting against the 'current' pointer being zero is for safety
and the case where the boot device is SCSI and the sg driver is
built into the kernel. Also when debugging, getting a message
from a compromised kernel can be very useful in pinpointing the
location of the failure.
The simple fact that the SG_LOG macro is shorter than
SCSI_LOG_TIMEOUT macro allow more error message "payload" per line.
Also replace #if and #ifdef conditional compilations with
the IS_ENABLED macro.
Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
---
drivers/scsi/sg.c | 252 +++++++++++++++++++++++-----------------------
1 file changed, 125 insertions(+), 127 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 6615777931f7..d14ba4a5441c 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -57,6 +57,15 @@ static char *sg_version_date = "20190606";
#define SG_MAX_DEVS 32768
+/* Comment out the following line to compile out SCSI_LOGGING stuff */
+#define SG_DEBUG 1
+
+#if !IS_ENABLED(SG_DEBUG)
+#if IS_ENABLED(DEBUG) /* If SG_DEBUG not defined, check for DEBUG */
+#define SG_DEBUG DEBUG
+#endif
+#endif
IS_ENABLED is mostly useful for checking it from C-level if statements.
No need to use this from cpp. But even more importantly we generally
try to avoid cpp checks that aren't driven from Kconfig. Please make
these an actual CONFIG_ options.
Another reviewer (called James) didn't like #ifdef_s (at file scope).
Surely it is simpler to allow an experienced C programmer to add
#define DEBUG 1
or
#define SG_DEBUG 1
to the driver source code than to add to the existing Linux config
nightmare with a new config parameter:
CONFIG_CHR_DEV_SG_DEBUG
$ grep "^CONF" <linux-stable>/.config | wc
2480
on my system.
Linux: where too much is never enough.
static int sg_read_oxfer(struct sg_request *srp, char __user *outp,
- int num_read_xfer);
+ int num_xfer);
This looks like a random unrelated change.
Separate patch maybe :-)
-#define SZ_SG_HEADER sizeof(struct sg_header)
-#define SZ_SG_IO_HDR sizeof(sg_io_hdr_t)
-#define SZ_SG_IOVEC sizeof(sg_iovec_t)
-#define SZ_SG_REQ_INFO sizeof(sg_req_info_t)
+#define SZ_SG_HEADER ((int)sizeof(struct sg_header)) /* v1 and v2 header */
+#define SZ_SG_IO_HDR ((int)sizeof(struct sg_io_hdr)) /* v3 header */
+#define SZ_SG_REQ_INFO ((int)sizeof(struct sg_req_info))
Doesn't look related to the patch. But more importantly there should
be no point to cast or even have the macros wrapping the sizeof to
start with.
Well I find it useful as I spend a fair bit of time re-reading
my code. To me a dozen upper case letters is easier to decode
than a sea of parentheses.
And the tide is moving back against "unsigned" types, at least in
C++. It is now regarded as a mistake the way STL introduced lots
of "unsigned" types and they are looking at whether they can move
some of them back to int.
Closer to home, I use ints wherever possible and the kernel seems
to favour them with negated errno return values. However the compiler
mindlessly warns about signed/unsigned comparisons when an int is
compared to a sizeof(). [If it had half a brain, it would check the
_value_ of the sizeof, and realize that often, even on an 8 bit
machine, there is no reason for the warning.]
Then there is the "Fortran rule" in checkpatch.pl (i.e. no lines
than 80 characters). "(int)sizeof(struct sg_io_hdr)" is 30 characters
long and that length often pushes simple conditionals like:
if (A > B)
onto two lines which IMO is stupid.
Doug Gilbert
