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