> -----Original Message----- > From: Hannes Reinecke [mailto:hare@xxxxxxx] > Sent: Tuesday, 04 November, 2014 2:07 AM ... > diff --git a/drivers/scsi/scsi_logging.c > b/drivers/scsi/scsi_logging.c ... > @@ -0,0 +1,119 @@ > +/* > + * scsi_logging.c > + * > + * Copyright (C) 2014 SUSE Linux Products GmbH > + * Copyright (C) 2014 Hannes Reinecke <hare@xxxxxxx> > + * > + * This file is released under the GPLv2 > + */ > + > +#include <linux/kernel.h> > +#include <linux/atomic.h> > + > +#include <scsi/scsi.h> > +#include <scsi/scsi_cmnd.h> > +#include <scsi/scsi_device.h> > +#include <scsi/scsi_dbg.h> > + > +#define SCSI_LOG_SPOOLSIZE 4096 > +#define SCSI_LOG_BUFSIZE 128 > + > +struct scsi_log_buf { > + char buffer[SCSI_LOG_SPOOLSIZE]; > + unsigned long map; > +}; > + > +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log); > + > +static char *scsi_log_reserve_buffer(size_t *len) > +{ > + struct scsi_log_buf *buf; > + unsigned long map_bits = SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE; > + unsigned long idx = 0; > + > + WARN_ON(map_bits > BITS_PER_LONG); Since SCSI_LOG_SPOOLSIZE, SCSI_LOG_BUFSIZE, and BITS_PER_LONG are constants, that can be a compile-time check. > + preempt_disable(); > + buf = this_cpu_ptr(&scsi_format_log); > + idx = find_first_zero_bit(&buf->map, map_bits); If this fails to find a bit, it returns map_bits. This could result in the next test_and_set_bit call accessing an address that is outside the bounds of buf->map. A safety check seems prudent before the test_and_set_bit: if (likely(idx < map_bits)) > + while (test_and_set_bit(idx, &buf->map)) { > + idx = find_next_zero_bit(&buf->map, map_bits, idx); > + if (idx >= map_bits) { > + break; > + } scripts/checkpatch.pl -f doesn't like the {} on that. > + } > + if (WARN_ON(idx >= map_bits)) { > + preempt_enable(); > + return NULL; > + } > + *len = SCSI_LOG_BUFSIZE; > + return buf->buffer + idx * SCSI_LOG_BUFSIZE; > +} > + > +static void scsi_log_release_buffer(char *bufptr) > +{ > + struct scsi_log_buf *buf; > + unsigned long idx; > + int ret; > + > + buf = this_cpu_ptr(&scsi_format_log); > + if (bufptr < buf->buffer + SCSI_LOG_SPOOLSIZE) { Should that also check that bufptr > buf->buffer? > + idx = (bufptr - buf->buffer) / SCSI_LOG_BUFSIZE; > + ret = test_and_clear_bit(idx, &buf->map); > + WARN_ON(!ret); > + } > + preempt_enable(); > +} > + > +int sdev_prefix_printk(const char *level, const struct scsi_device > *sdev, > + const char *name, const char *fmt, ...) > +{ > + va_list args; > + char *logbuf; > + size_t off = 0, logbuf_len; > + int ret; > + > + if (!sdev) > + return 0; > + > + logbuf = scsi_log_reserve_buffer(&logbuf_len); > + if (!logbuf) > + return 0; > + > + if (name) > + off += scnprintf(logbuf + off, logbuf_len - off, > + "[%s] ", name); > + va_start(args, fmt); > + off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args); > + va_end(args); > + ret = dev_printk(level, &sdev->sdev_gendev, "%s", logbuf); > + scsi_log_release_buffer(logbuf); > + return ret; > +} > +EXPORT_SYMBOL_GPL(sdev_prefix_printk); > + > +int scmd_printk(const char *level, const struct scsi_cmnd *scmd, > + const char *fmt, ...) > +{ > + struct gendisk *disk = scmd->request->rq_disk; > + va_list args; > + char *logbuf; > + size_t off = 0, logbuf_len; > + int ret; > + > + if (!scmd || scmd->cmnd == NULL) > + return 0; !scmd->cmnd seems more common in neighboring code. > + > + logbuf = scsi_log_reserve_buffer(&logbuf_len); > + if (!logbuf) > + return 0; > + if (disk) > + off += scnprintf(logbuf + off, logbuf_len - off, > + "[%s] ", disk->disk_name); > + va_start(args, fmt); > + off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args); > + va_end(args); > + ret = dev_printk(level, &scmd->device->sdev_gendev, "%s", > logbuf); > + scsi_log_release_buffer(logbuf); > + return ret; > +} > +EXPORT_SYMBOL_GPL(scmd_printk); ... dev_printk is EXPORT_SYMBOL. The former macro versions of sdev_printk and scmd_printk just called dev_printk, and not being symbols, did not change that. So, I think these function versions should use EXPORT_SYMBOL, so the hated binary drivers can still call them. --- Rob Elliott HP Server Storage -- 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
