Re: [PATCH 01/10] scsi: Use real functions for logging

[Hannes Reinecke <hare@suse.de>]

On 11/11/2014 06:38 PM, Elliott, Robert (Server Storage) wrote:
> 
> 
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:hare@suse.de]
>> 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@suse.de>
>> + *
>> + * 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.
> 
Ok.

>> +	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))
> 
Ah. I wasn't quite sure what find_first_zero_bit would return
on failure. But yeah, that check seems to be appropriate.

>> +	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.
> 
Sigh.

>> +	}
>> +	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?
> 
Probably.

>> +		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.
> 
Ok.

>> +
>> +	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.
> 
Okay, will be fixing things up for the next revision.

Thanks for the review.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html