[PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
to provide enhanced support for Unit Attention conditions, as well as
detection of reported sense data overflow conditions and some changes
to sense data processing.  It also adds a uevent when the reported
capacity changes on an sd device.

There was some discussion about this a couple of years ago on the linux-scsi
mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
Although one approach is to send all SCSI sense data to a userspace daemon
for processing, this patch set does not take that approach due to the
difficulty in reliably delivering all of the data.  An interesting UA
condition might not be delivered due to a flood of media errors, for example.

The mechanism used is to flag when certain UA ASC/ASCQ codes are received
that report asynchronous changes to the storage device configuration.
An appropriate uevent is then generated for the scsi_device or scsi_target
object.  An aggregation mechanism is used to avoid generating uevents at
too high a rate, and to coalesce multiple UAs reported by LUNs on the
same target for a REPORTED LUNS DATA HAS CHANGED sense code.

The changes are (mostly) enabled by a new kernel config option
CONFIG_SCSI_ENHANCED_UA.  If this config option is not used, no new
uevents are generated.  There are some changes to kernel logging messages
if CONFIG_SCSI_ENHANCED_UA is enabled, because the existing messages
explicitly stated that the kernel did not do anything with the information.

Note that checkpatch is reporting errors on patch 7/9 relating to macros
in scsi_sysfs.c -- I believe these errors are incorrect and have sent a
message to the checkpatch maintainer.  The macros were derived from
existing ones already in the file.

Ewan D. Milne (9):
  [SCSI] Detect overflow of sense data buffer
  [SCSI] Generate uevent on sd capacity change
  [SCSI] Add a kernel config option for enhanced Unit Attention support
  [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to
    sdev_event
  [SCSI] Add support for scsi_target events
  [SCSI] Generate uevents for certain Unit Attention codes
  [SCSI] Add sysfs support for enhanced Unit Attention handling
  [SCSI] Add sense and Unit Attention generation to scsi_debug
  [SCSI] Streamline detection of FM/EOM/ILI status

 drivers/scsi/Kconfig       |  12 +++
 drivers/scsi/scsi_debug.c  | 168 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_error.c  | 198 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_lib.c    | 178 +++++++++++++++++++++++++++++++++++++---
 drivers/scsi/scsi_priv.h   |  10 ++-
 drivers/scsi/scsi_scan.c   |  54 ++++++-------
 drivers/scsi/scsi_sysfs.c  | 148 ++++++++++++++++++++++++++++++---
 drivers/scsi/sd.c          |   9 +++
 include/scsi/scsi_cmnd.h   |   4 +
 include/scsi/scsi_device.h |  64 ++++++++++++++-
 include/scsi/scsi_eh.h     |   7 ++
 11 files changed, 773 insertions(+), 79 deletions(-)

-- 
1.7.11.7

[PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added code to SCSI mid-layer sense processing to detect the SDAT_OVFL
bit in Fixed or Descriptor format sense data, and log a kernel message
if the device reported that the sense data did not fit in the buffer.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_debug.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_error.c |  9 ++++++++-
 include/scsi/scsi_eh.h    |  1 +
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..88f6d16 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -230,6 +230,7 @@ struct sdebug_dev_info {
 	char reset;
 	char stopped;
 	char used;
+	char sense_pending;
 };
 
 struct sdebug_host_info {
@@ -3205,6 +3206,32 @@ static ssize_t sdebug_map_show(struct device_driver *ddp, char *buf)
 }
 DRIVER_ATTR(map, S_IRUGO, sdebug_map_show, NULL);
 
+static ssize_t sdebug_sense_overflow_store(struct device_driver *ddp,
+					   const char *buf, size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, NO_SENSE, 0, 0);
+				if (scsi_debug_dsense)
+					devip->sense_buff[4] = 0x80;
+				else
+					devip->sense_buff[2] = 0x10;
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(sense_overflow, S_IWUSR, NULL, sdebug_sense_overflow_store);
 
 /* Note: The following function creates attribute files in the
    /sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
@@ -3239,11 +3266,15 @@ static int do_create_driverfs_files(void)
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_guard);
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_ato);
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_map);
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_sense_overflow);
 	return ret;
 }
 
 static void do_remove_driverfs_files(void)
 {
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_sense_overflow);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_map);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_ato);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_guard);
@@ -3926,6 +3957,10 @@ write:
 		errsts = check_condition_result;
 		break;
 	}
+	if (!errsts && devip->sense_pending) {
+		devip->sense_pending = 0;
+		errsts = check_condition_result;
+	}
 	return schedule_resp(SCpnt, devip, done, errsts,
 			     (delay_override ? 0 : scsi_debug_delay));
 }
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d0f71e5..20669836 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	if (! scsi_command_normalize_sense(scmd, &sshdr))
 		return FAILED;	/* no valid sense data */
 
+	if (sshdr.overflow)
+		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
+
 	if (scsi_sense_is_deferred(&sshdr))
 		return NEEDS_RETRY;
 
@@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 			sshdr->asc = sense_buffer[2];
 		if (sb_len > 3)
 			sshdr->ascq = sense_buffer[3];
+		if (sb_len > 4)
+			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
 		if (sb_len > 7)
 			sshdr->additional_length = sense_buffer[7];
 	} else {
 		/*
 		 * fixed format
 		 */
-		if (sb_len > 2)
+		if (sb_len > 2) {
+			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
 			sshdr->sense_key = (sense_buffer[2] & 0xf);
+		}
 		if (sb_len > 7) {
 			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
 					 sb_len : (sense_buffer[7] + 8);
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 06a8790..ddf58cf 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -25,6 +25,7 @@ struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
 	u8 byte5;
 	u8 byte6;
 	u8 additional_length;	/* always 0 for fixed sense format */
+	unsigned int overflow:1;	/* sense data overflow */
 };
 
 static inline int scsi_sense_valid(struct scsi_sense_hdr *sshdr)
-- 
1.7.11.7

[PATCH RFC 2/9] [SCSI] Generate uevent on sd capacity change

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

In sd_read_capacity(), detect if the capacity is different from the
previously read value, and generate a uevent if this is the case.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 5 ++++-
 drivers/scsi/scsi_sysfs.c  | 2 ++
 drivers/scsi/sd.c          | 9 +++++++++
 include/scsi/scsi_device.h | 3 ++-
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6dfb978..eba68de 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2186,7 +2186,9 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 	case SDEV_EVT_MEDIA_CHANGE:
 		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
 		break;
-
+	case SDEV_EVT_CAPACITY_CHANGE:
+		envp[idx++] = "SDEV_CAPACITY_CHANGE=1";
+		break;
 	default:
 		/* do nothing */
 		break;
@@ -2280,6 +2282,7 @@ struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 	/* evt_type-specific initialization, if any */
 	switch (evt_type) {
 	case SDEV_EVT_MEDIA_CHANGE:
+	case SDEV_EVT_CAPACITY_CHANGE:
 	default:
 		/* do nothing */
 		break;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 04c2a27..ba7da3b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -712,6 +712,7 @@ sdev_store_evt_##name(struct device *dev, struct device_attribute *attr,\
 #define REF_EVT(name) &dev_attr_evt_##name.attr
 
 DECLARE_EVT(media_change, MEDIA_CHANGE)
+DECLARE_EVT(capacity_change, CAPACITY_CHANGE)
 
 /* Default template for device attributes.  May NOT be modified */
 static struct attribute *scsi_sdev_attrs[] = {
@@ -731,6 +732,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_ioerr_cnt.attr,
 	&dev_attr_modalias.attr,
 	REF_EVT(media_change),
+	REF_EVT(capacity_change),
 	NULL
 };
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6f0a4c6..9f2d00a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2017,6 +2017,15 @@ got_data:
 					  "%u-byte physical blocks\n",
 					  sdkp->physical_block_size);
 		}
+
+		/*
+		 * Don't report a capacity change event unless we have a
+		 * valid "old" value.
+		 */
+		if (!sdkp->first_scan && (old_capacity != 0) &&
+		    (old_capacity != sdkp->capacity))
+			sdev_evt_send_simple(sdp, SDEV_EVT_CAPACITY_CHANGE,
+					     GFP_KERNEL);
 	}
 
 	/* Rescale capacity to 512-byte units */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6efb2e1..e4c964e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -51,8 +51,9 @@ enum scsi_device_state {
 
 enum scsi_device_event {
 	SDEV_EVT_MEDIA_CHANGE	= 1,	/* media has changed */
+	SDEV_EVT_CAPACITY_CHANGE	= 2,	/* capacity has changed */
 
-	SDEV_EVT_LAST		= SDEV_EVT_MEDIA_CHANGE,
+	SDEV_EVT_LAST		= SDEV_EVT_CAPACITY_CHANGE,
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
 
-- 
1.7.11.7

[PATCH RFC 3/9] [SCSI] Add a kernel config option for enhanced Unit Attention support

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added CONFIG_SCSI_ENHANCED_UA kernel config option to enable changes
in the SCSI mid-layer which detect and report certain Unit Attention
conditions reported by devices.  These changes are primarily useful
when storage arrays that can be reconfigured are being used, so the
config option would normally not be used unless it was needed.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index e955978..52836e7 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -280,6 +280,18 @@ config SCSI_WAIT_SCAN
 # disabling it, whereas people who accidentally switch it off may wonder why
 # their mkinitrd gets into trouble.
 
+config SCSI_ENHANCED_UA
+       bool "Enhanced SCSI Unit Attention handling"
+       depends on SCSI
+       help
+	 Certain SCSI devices report changes via a UNIT ATTENTION code.
+	 (For example, the addition or removal of LUNs from a target, or
+	 changing the number of logical blocks on a a LUN.)  This option
+	 enables reporting of these changes via udev events, so that the
+	 device can be rescanned to find out what has changed.  This is
+	 primarily useful when storage arrays that can be reconfigured
+	 are attached to the system, otherwise you can say N here.
+
 menu "SCSI Transports"
 	depends on SCSI
 
-- 
1.7.11.7

[PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

The names of the struct and some of the functions for scsi_device
events are too generic and do not match the comments in the source.
Changed all of the names to begin with sdev_ in order to avoid
naming issues and confusion with scsi_target events to be added.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 22 +++++++++++-----------
 drivers/scsi/scsi_priv.h   |  1 +
 drivers/scsi/scsi_scan.c   |  3 +--
 drivers/scsi/scsi_sysfs.c  |  4 ++--
 include/scsi/scsi_device.h |  6 +++---
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eba68de..9de1186 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(scsi_device_set_state);
  *	@sdev: associated SCSI device
  *	@evt: event to emit
  *
- *	Send a single uevent (scsi_event) to the associated scsi_device.
+ *	Send a single uevent (sdev_event) to the associated scsi_device.
  */
-static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
+static void sdev_evt_emit(struct scsi_device *sdev, struct sdev_event *evt)
 {
 	int idx = 0;
 	char *envp[3];
@@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
  *	Dispatch queued events to their associated scsi_device kobjects
  *	as uevents.
  */
-void scsi_evt_thread(struct work_struct *work)
+void sdev_evt_thread(struct work_struct *work)
 {
 	struct scsi_device *sdev;
 	LIST_HEAD(event_list);
@@ -2214,7 +2214,7 @@ void scsi_evt_thread(struct work_struct *work)
 	sdev = container_of(work, struct scsi_device, event_work);
 
 	while (1) {
-		struct scsi_event *evt;
+		struct sdev_event *evt;
 		struct list_head *this, *tmp;
 		unsigned long flags;
 
@@ -2226,9 +2226,9 @@ void scsi_evt_thread(struct work_struct *work)
 			break;
 
 		list_for_each_safe(this, tmp, &event_list) {
-			evt = list_entry(this, struct scsi_event, node);
+			evt = list_entry(this, struct sdev_event, node);
 			list_del(&evt->node);
-			scsi_evt_emit(sdev, evt);
+			sdev_evt_emit(sdev, evt);
 			kfree(evt);
 		}
 	}
@@ -2241,7 +2241,7 @@ void scsi_evt_thread(struct work_struct *work)
  *
  *	Assert scsi device event asynchronously.
  */
-void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt)
+void sdev_evt_send(struct scsi_device *sdev, struct sdev_event *evt)
 {
 	unsigned long flags;
 
@@ -2267,12 +2267,12 @@ EXPORT_SYMBOL_GPL(sdev_evt_send);
  *	@evt_type: type of event to allocate
  *	@gfpflags: GFP flags for allocation
  *
- *	Allocates and returns a new scsi_event.
+ *	Allocates and returns a new sdev_event.
  */
-struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
+struct sdev_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 				  gfp_t gfpflags)
 {
-	struct scsi_event *evt = kzalloc(sizeof(struct scsi_event), gfpflags);
+	struct sdev_event *evt = kzalloc(sizeof(struct sdev_event), gfpflags);
 	if (!evt)
 		return NULL;
 
@@ -2303,7 +2303,7 @@ EXPORT_SYMBOL_GPL(sdev_evt_alloc);
 void sdev_evt_send_simple(struct scsi_device *sdev,
 			  enum scsi_device_event evt_type, gfp_t gfpflags)
 {
-	struct scsi_event *evt = sdev_evt_alloc(evt_type, gfpflags);
+	struct sdev_event *evt = sdev_evt_alloc(evt_type, gfpflags);
 	if (!evt) {
 		sdev_printk(KERN_ERR, sdev, "event %d eaten due to OOM\n",
 			    evt_type);
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 07ce3f5..658a51a 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -90,6 +90,7 @@ extern void scsi_exit_queue(void);
 struct request_queue;
 struct request;
 extern struct kmem_cache *scsi_sdb_cache;
+extern void sdev_evt_thread(struct work_struct *work);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 2e5fe58..8f444aa 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -244,7 +244,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	struct scsi_device *sdev;
 	int display_failure_msg = 1, ret;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
-	extern void scsi_evt_thread(struct work_struct *work);
 	extern void scsi_requeue_run_queue(struct work_struct *work);
 
 	sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
@@ -267,7 +266,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
-	INIT_WORK(&sdev->event_work, scsi_evt_thread);
+	INIT_WORK(&sdev->event_work, sdev_evt_thread);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ba7da3b..b22210d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -353,9 +353,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	cancel_work_sync(&sdev->event_work);
 
 	list_for_each_safe(this, tmp, &sdev->event_list) {
-		struct scsi_event *evt;
+		struct sdev_event *evt;
 
-		evt = list_entry(this, struct scsi_event, node);
+		evt = list_entry(this, struct sdev_event, node);
 		list_del(&evt->node);
 		kfree(evt);
 	}
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e4c964e..dad0a37 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -57,7 +57,7 @@ enum scsi_device_event {
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
 
-struct scsi_event {
+struct sdev_event {
 	enum scsi_device_event	evt_type;
 	struct list_head	node;
 
@@ -360,9 +360,9 @@ extern int scsi_get_vpd_page(struct scsi_device *, u8 page, unsigned char *buf,
 			     int buf_len);
 extern int scsi_device_set_state(struct scsi_device *sdev,
 				 enum scsi_device_state state);
-extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
+extern struct sdev_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 					  gfp_t gfpflags);
-extern void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt);
+extern void sdev_evt_send(struct scsi_device *sdev, struct sdev_event *evt);
 extern void sdev_evt_send_simple(struct scsi_device *sdev,
 			  enum scsi_device_event evt_type, gfp_t gfpflags);
 extern int scsi_device_quiesce(struct scsi_device *sdev);
-- 
1.7.11.7

[PATCH RFC 5/9] [SCSI] Add support for scsi_target events

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added capability to generate uevents on scsi_target objects.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 135 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_priv.h   |   3 +
 drivers/scsi/scsi_scan.c   |  17 ++++++
 include/scsi/scsi_device.h |  34 ++++++++++++
 4 files changed, 189 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9de1186..056030a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2393,6 +2393,141 @@ scsi_target_resume(struct scsi_target *starget)
 }
 EXPORT_SYMBOL(scsi_target_resume);
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+/**
+ *	starget_evt_emit - emit a single SCSI target uevent
+ *	@starget: associated SCSI target
+ *	@evt: event to emit
+ *
+ *	Send a single uevent (starget_event) to the associated scsi_target.
+ */
+static void starget_evt_emit(struct scsi_target *starget,
+			     struct starget_event *evt)
+{
+	int idx = 0;
+	char *envp[3];
+
+	switch (evt->evt_type) {
+	case STARGET_EVT_LUN_CHANGE_REPORTED:
+		envp[idx++] = "STARGET_LUN_CHANGE_REPORTED=1";
+		break;
+	default:
+		/* do nothing */
+		break;
+	}
+
+	envp[idx++] = NULL;
+
+	kobject_uevent_env(&starget->dev.kobj, KOBJ_CHANGE, envp);
+}
+
+/**
+ *	starget_evt_thread - send a uevent for each scsi event
+ *	@work: work struct for scsi_target
+ *
+ *	Dispatch queued events to their associated scsi_target kobjects
+ *	as uevents.
+ */
+void starget_evt_thread(struct work_struct *work)
+{
+	struct scsi_target *starget;
+	LIST_HEAD(event_list);
+
+	starget = container_of(work, struct scsi_target, event_work);
+
+	while (1) {
+		struct starget_event *evt;
+		struct list_head *this, *tmp;
+		unsigned long flags;
+
+		spin_lock_irqsave(&starget->list_lock, flags);
+		list_splice_init(&starget->event_list, &event_list);
+		spin_unlock_irqrestore(&starget->list_lock, flags);
+
+		if (list_empty(&event_list))
+			break;
+
+		list_for_each_safe(this, tmp, &event_list) {
+			evt = list_entry(this, struct starget_event, node);
+			list_del(&evt->node);
+			starget_evt_emit(starget, evt);
+			kfree(evt);
+		}
+	}
+}
+
+/**
+ *	starget_evt_send - send asserted event to uevent thread
+ *	@starget: scsi_target event occurred on
+ *	@evt: event to send
+ *
+ *	Assert scsi target event asynchronously.
+ */
+void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&starget->list_lock, flags);
+	list_add_tail(&evt->node, &starget->event_list);
+	schedule_work(&starget->event_work);
+	spin_unlock_irqrestore(&starget->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(starget_evt_send);
+
+/**
+ *	starget_evt_alloc - allocate a new scsi_target event
+ *	@evt_type: type of event to allocate
+ *	@gfpflags: GFP flags for allocation
+ *
+ *	Allocates and returns a new starget_event.
+ */
+struct starget_event *starget_evt_alloc(enum scsi_target_event evt_type,
+					gfp_t gfpflags)
+{
+	struct starget_event *evt = kzalloc(sizeof(struct starget_event),
+					    gfpflags);
+	if (!evt)
+		return NULL;
+
+	evt->evt_type = evt_type;
+	INIT_LIST_HEAD(&evt->node);
+
+	/* evt_type-specific initialization, if any */
+	switch (evt_type) {
+	case STARGET_EVT_LUN_CHANGE_REPORTED:
+	default:
+		/* do nothing */
+		break;
+	}
+
+	return evt;
+}
+EXPORT_SYMBOL_GPL(starget_evt_alloc);
+
+/**
+ *	starget_evt_send_simple - send asserted event to uevent thread
+ *	@starget: scsi_target event occurred on
+ *	@evt_type: type of event to send
+ *	@gfpflags: GFP flags for allocation
+ *
+ *	Assert scsi target event asynchronously, given an event type.
+ */
+void starget_evt_send_simple(struct scsi_target *starget,
+			     enum scsi_target_event evt_type, gfp_t gfpflags)
+{
+	struct starget_event *evt = starget_evt_alloc(evt_type, gfpflags);
+	if (!evt) {
+		starget_printk(KERN_ERR, starget, "event %d eaten due to OOM\n",
+			       evt_type);
+		return;
+	}
+
+	starget_evt_send(starget, evt);
+}
+EXPORT_SYMBOL_GPL(starget_evt_send_simple);
+
+#endif
+
 /**
  * scsi_internal_device_block - internal function to put a device temporarily into the SDEV_BLOCK state
  * @sdev:	device to block
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 658a51a..e271792 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -91,6 +91,9 @@ struct request_queue;
 struct request;
 extern struct kmem_cache *scsi_sdb_cache;
 extern void sdev_evt_thread(struct work_struct *work);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+extern void starget_evt_thread(struct work_struct *work);
+#endif
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8f444aa..3b0e5b5 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -425,6 +425,11 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->can_queue = 0;
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	INIT_LIST_HEAD(&starget->event_list);
+	spin_lock_init(&starget->list_lock);
+	INIT_WORK(&starget->event_work, starget_evt_thread);
+#endif
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
@@ -472,7 +477,19 @@ static void scsi_target_reap_usercontext(struct work_struct *work)
 {
 	struct scsi_target *starget =
 		container_of(work, struct scsi_target, ew.work);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	struct list_head *this, *tmp;
+
+	cancel_work_sync(&starget->event_work);
+
+	list_for_each_safe(this, tmp, &starget->event_list) {
+		struct starget_event *evt;
 
+		evt = list_entry(this, struct starget_event, node);
+		list_del(&evt->node);
+		kfree(evt);
+	}
+#endif
 	transport_remove_device(&starget->dev);
 	device_del(&starget->dev);
 	scsi_target_destroy(starget);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index dad0a37..240aa6d 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -230,6 +230,24 @@ enum scsi_target_state {
 	STARGET_DEL,
 };
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+enum scsi_target_event {
+	STARGET_EVT_LUN_CHANGE_REPORTED	= 1,	/* UA reported */
+
+	STARGET_EVT_LAST	= STARGET_EVT_LUN_CHANGE_REPORTED,
+	STARGET_EVT_MAXBITS	= STARGET_EVT_LAST + 1
+};
+
+struct starget_event {
+	enum scsi_target_event	evt_type;
+	struct list_head	node;
+
+	/* put union of data structures, for non-simple event types,
+	 * here
+	 */
+};
+#endif
+
 /*
  * scsi_target: representation of a scsi target, for now, this is only
  * used for single_lun devices. If no one has active IO to the target,
@@ -252,6 +270,13 @@ struct scsi_target {
 						 * means no lun present. */
 	unsigned int		no_report_luns:1;	/* Don't use
 						 * REPORT LUNS for scanning. */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	DECLARE_BITMAP(supported_events, STARGET_EVT_MAXBITS);
+	struct list_head	event_list;	/* asserted events */
+	struct work_struct	event_work;
+	spinlock_t list_lock;
+#endif
+
 	/* commands actually active on LLD. protected by host lock. */
 	unsigned int		target_busy;
 	/*
@@ -369,6 +394,15 @@ extern int scsi_device_quiesce(struct scsi_device *sdev);
 extern void scsi_device_resume(struct scsi_device *sdev);
 extern void scsi_target_quiesce(struct scsi_target *);
 extern void scsi_target_resume(struct scsi_target *);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+extern struct starget_event *starget_evt_alloc(enum scsi_target_event evt_type,
+					       gfp_t gfpflags);
+extern void starget_evt_send(struct scsi_target *starget,
+			     struct starget_event *evt);
+extern void starget_evt_send_simple(struct scsi_target *starget,
+				    enum scsi_target_event evt_type,
+				    gfp_t gfpflags);
+#endif
 extern void scsi_scan_target(struct device *parent, unsigned int channel,
 			     unsigned int id, unsigned int lun, int rescan);
 extern void scsi_target_reap(struct scsi_target *);
-- 
1.7.11.7

[PATCH RFC 6/9] [SCSI] Generate uevents for certain Unit Attention codes

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Generate a uevent on the scsi_target object when the following
Unit Attention ASC/ASCQ code is received:

    3F/0E  REPORTED LUNS DATA HAS CHANGED

Generate a uevent on the scsi_device object when the following
Unit Attention ASC/ASCQ codes are received:

    2A/01  MODE PARAMETERS CHANGED
    2A/09  CAPACITY DATA HAS CHANGED
    38/07  THIN PROVISIONING SOFT THRESHOLD REACHED

All uevent generation is aggregated and rate-limited so that any
individual event is delivered no more than once every 2 seconds.

Log kernel messages when the following Unit Attention ASC/ASCQ
codes are received:

    2A/xx  PARAMETERS CHANGED
    3F/03  INQUIRY DATA HAS CHANGED
    3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED

Also changed the kernel log messages on existing Unit Attention
codes, to remove text that indicates that the conditions were not
handled in any way (since they are now handled in some way) and
reflect the wording used in SPC-4.

Also log a kernel message when the a scsi device reports a Unit
Attention queue overflow, indicating that status has been lost.

These changes are only enabled when the kernel config option
CONFIG_SCSI_ENHANCED_UA is set.  Otherwise, the behavior is the
same as before, including the existing kernel log messages.
However, the detection of these conditions was moved to be earlier
in scsi_check_sense(), because the code was not always reached.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_error.c  | 176 +++++++++++++++++++++++++++++++++++++++------
 drivers/scsi/scsi_lib.c    |  16 +++++
 drivers/scsi/scsi_priv.h   |   2 +
 drivers/scsi/scsi_scan.c   |   5 ++
 drivers/scsi/scsi_sysfs.c  |   3 +
 include/scsi/scsi_cmnd.h   |   4 ++
 include/scsi/scsi_device.h |  21 ++++++
 include/scsi/scsi_eh.h     |   3 +
 8 files changed, 209 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 20669836..3aa8706 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -244,6 +244,77 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	if (sshdr.overflow)
 		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	if (sshdr.ua_queue_overflow)
+		scmd_printk(KERN_WARNING, scmd,
+			    "Unit Attention queue overflow");
+#endif
+
+	if (sshdr.sense_key == UNIT_ATTENTION) {
+		if (sshdr.asc == 0x3f && sshdr.ascq == 0x03)
+			scmd_printk(KERN_WARNING, scmd,
+				    "Inquiry data has changed");
+		else if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			struct scsi_target *starget = scsi_target(sdev);
+			if (atomic_xchg(&starget->lun_change_reported, 1) == 0)
+				schedule_delayed_work(&starget->ua_dwork, 2*HZ);
+			scmd_printk(KERN_WARNING, scmd,
+				    "Reported LUNs data has changed");
+#else
+			scmd_printk(KERN_WARNING, scmd,
+				    "Warning! Received an indication that the "
+				    "LUN assignments on this target have "
+				    "changed. The Linux SCSI layer does not "
+				    "automatically remap LUN assignments.\n");
+#endif
+		} else if (sshdr.asc == 0x3f)
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			scmd_printk(KERN_WARNING, scmd,
+				    "Target operating conditions have changed");
+#else
+			scmd_printk(KERN_WARNING, scmd,
+				    "Warning! Received an indication that the "
+				    "operating parameters on this target have "
+				    "changed. The Linux SCSI layer does not "
+				    "automatically adjust these parameters.\n");
+#endif
+
+		if (sshdr.asc == 0x38 && sshdr.ascq == 0x07) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			if (atomic_xchg(&sdev->soft_threshold_reached, 1) == 0)
+				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
+			scmd_printk(KERN_WARNING, scmd,
+				    "Thin provisioning soft threshold reached");
+#else
+			scmd_printk(KERN_WARNING, scmd,
+				    "Warning! Received an indication that the "
+				    "LUN reached a thin provisioning soft "
+				    "threshold.\n");
+#endif
+		}
+
+		if (sshdr.asc == 0x2a && sshdr.ascq == 0x01) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			if (atomic_xchg(&sdev->mode_parameter_change_reported,
+					1) == 0)
+				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
+#endif
+			scmd_printk(KERN_WARNING, scmd,
+				    "Mode parameters changed");
+		} else if (sshdr.asc == 0x2a && sshdr.ascq == 0x09) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			if (atomic_xchg(&sdev->capacity_change_reported,
+					1) == 0)
+				schedule_delayed_work(&sdev->ua_dwork, 2 * HZ);
+#endif
+			scmd_printk(KERN_WARNING, scmd,
+				    "Capacity data has changed");
+		} else if (sshdr.asc == 0x2a)
+			scmd_printk(KERN_WARNING, scmd,
+				    "Parameters changed");
+	}
+
 	if (scsi_sense_is_deferred(&sshdr))
 		return NEEDS_RETRY;
 
@@ -321,26 +392,6 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		if (scmd->device->allow_restart &&
 		    (sshdr.asc == 0x04) && (sshdr.ascq == 0x02))
 			return FAILED;
-
-		if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e)
-			scmd_printk(KERN_WARNING, scmd,
-				    "Warning! Received an indication that the "
-				    "LUN assignments on this target have "
-				    "changed. The Linux SCSI layer does not "
-				    "automatically remap LUN assignments.\n");
-		else if (sshdr.asc == 0x3f)
-			scmd_printk(KERN_WARNING, scmd,
-				    "Warning! Received an indication that the "
-				    "operating parameters on this target have "
-				    "changed. The Linux SCSI layer does not "
-				    "automatically adjust these parameters.\n");
-
-		if (sshdr.asc == 0x38 && sshdr.ascq == 0x07)
-			scmd_printk(KERN_WARNING, scmd,
-				    "Warning! Received an indication that the "
-				    "LUN reached a thin provisioning soft "
-				    "threshold.\n");
-
 		/*
 		 * Pass the UA upwards for a determination in the completion
 		 * functions.
@@ -383,6 +434,52 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	}
 }
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+/**
+ *	sdev_ua_events - send a uevent for received unit attention events
+ *	@work: work struct for scsi_device
+ *
+ *	Dispatch aggregated events to their associated scsi_device kobjects
+ *	as uevents.
+ */
+void sdev_ua_events(struct work_struct *work)
+{
+	struct scsi_device *sdev;
+
+	sdev = container_of(work, struct scsi_device, ua_dwork.work);
+
+	if (atomic_xchg(&sdev->capacity_change_reported, 0) != 0)
+		sdev_evt_send_simple(sdev, SDEV_EVT_CAPACITY_CHANGE_REPORTED,
+				     GFP_KERNEL);
+	if (atomic_xchg(&sdev->soft_threshold_reached, 0) != 0)
+		sdev_evt_send_simple(sdev, SDEV_EVT_SOFT_THRESHOLD_REACHED,
+				     GFP_KERNEL);
+	if (atomic_xchg(&sdev->mode_parameter_change_reported, 0) != 0)
+		sdev_evt_send_simple(sdev,
+				     SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,
+				     GFP_KERNEL);
+}
+
+/**
+ *	starget_ua_events - send a uevent for received unit attention events
+ *	@work: work struct for scsi_target
+ *
+ *	Dispatch aggregated events to their associated scsi_target kobjects
+ *	as uevents.
+ */
+void starget_ua_events(struct work_struct *work)
+{
+	struct scsi_target *starget;
+
+	starget = container_of(work, struct scsi_target, ua_dwork.work);
+
+	if (atomic_xchg(&starget->lun_change_reported, 0) != 0)
+		starget_evt_send_simple(starget,
+					STARGET_EVT_LUN_CHANGE_REPORTED,
+					GFP_KERNEL);
+}
+#endif
+
 static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 {
 	struct scsi_host_template *sht = sdev->host->hostt;
@@ -2042,6 +2139,10 @@ EXPORT_SYMBOL(scsi_reset_provider);
 int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
                          struct scsi_sense_hdr *sshdr)
 {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	int desc_pos, desc_len, desc_type, addl_len;
+#endif
+
 	if (!sense_buffer || !sb_len)
 		return 0;
 
@@ -2064,8 +2165,41 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 			sshdr->ascq = sense_buffer[3];
 		if (sb_len > 4)
 			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
-		if (sb_len > 7)
+		if (sb_len > 7) {
 			sshdr->additional_length = sense_buffer[7];
+
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			desc_pos = 8;
+			desc_len = sshdr->additional_length;
+
+			if (desc_len > (sb_len - 8))
+				desc_len = sb_len - 8;
+/*
+ * desc_len should be either 0 or >= 2 here but it might not be, in which case
+ * we can't continue because we can't extract addl_len (and there is no data)
+ */
+			while (desc_len >= 2) {
+				desc_type = sense_buffer[desc_pos];
+				addl_len = sense_buffer[desc_pos + 1];
+
+				if ((sshdr->sense_key == UNIT_ATTENTION) &&
+				    (desc_type == 0x02) && (desc_len >= 5) &&
+				    (addl_len >= 3) &&
+/*
+ * addl_len is supposed to be 0x6 per the spec but we don't actually need that
+ * here, we just need it to be long enough so we can examine [desc_pos + 4]
+ */
+				    ((sense_buffer[desc_pos + 4] & 0x01) != 0))
+					sshdr->ua_queue_overflow = 1;
+
+				if (addl_len > (desc_len - 2))
+					addl_len = desc_len - 2;
+
+				desc_pos += (2 + addl_len);
+				desc_len -= (2 + addl_len);
+			}
+#endif
+		}
 	} else {
 		/*
 		 * fixed format
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 056030a..22f9acc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2189,6 +2189,17 @@ static void sdev_evt_emit(struct scsi_device *sdev, struct sdev_event *evt)
 	case SDEV_EVT_CAPACITY_CHANGE:
 		envp[idx++] = "SDEV_CAPACITY_CHANGE=1";
 		break;
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_CAPACITY_CHANGE_REPORTED=1";
+		break;
+	case SDEV_EVT_SOFT_THRESHOLD_REACHED:
+		envp[idx++] = "SDEV_SOFT_THRESHOLD_REACHED=1";
+		break;
+	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_MODE_PARAMETER_CHANGE_REPORTED=1";
+		break;
+#endif
 	default:
 		/* do nothing */
 		break;
@@ -2283,6 +2294,11 @@ struct sdev_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 	switch (evt_type) {
 	case SDEV_EVT_MEDIA_CHANGE:
 	case SDEV_EVT_CAPACITY_CHANGE:
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+	case SDEV_EVT_SOFT_THRESHOLD_REACHED:
+	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
+#endif
 	default:
 		/* do nothing */
 		break;
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index e271792..ddd2d31 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -92,7 +92,9 @@ struct request;
 extern struct kmem_cache *scsi_sdb_cache;
 extern void sdev_evt_thread(struct work_struct *work);
 #ifdef CONFIG_SCSI_ENHANCED_UA
+extern void sdev_ua_events(struct work_struct *work);
 extern void starget_evt_thread(struct work_struct *work);
+extern void starget_ua_events(struct work_struct *work);
 #endif
 
 /* scsi_proc.c */
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3b0e5b5..f252c14 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -268,6 +268,9 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	spin_lock_init(&sdev->list_lock);
 	INIT_WORK(&sdev->event_work, sdev_evt_thread);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	INIT_DELAYED_WORK(&sdev->ua_dwork, sdev_ua_events);
+#endif
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
 	sdev->sdev_target = starget;
@@ -429,6 +432,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	INIT_LIST_HEAD(&starget->event_list);
 	spin_lock_init(&starget->list_lock);
 	INIT_WORK(&starget->event_work, starget_evt_thread);
+	INIT_DELAYED_WORK(&starget->ua_dwork, starget_ua_events);
 #endif
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
@@ -481,6 +485,7 @@ static void scsi_target_reap_usercontext(struct work_struct *work)
 	struct list_head *this, *tmp;
 
 	cancel_work_sync(&starget->event_work);
+	cancel_delayed_work_sync(&starget->ua_dwork);
 
 	list_for_each_safe(this, tmp, &starget->event_list) {
 		struct starget_event *evt;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b22210d..aff1e29 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -351,6 +351,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	cancel_delayed_work_sync(&sdev->ua_dwork);
+#endif
 
 	list_for_each_safe(this, tmp, &sdev->event_list) {
 		struct sdev_event *evt;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 1e11985..4cb7a5e 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -103,7 +103,11 @@ struct scsi_cmnd {
 	struct request *request;	/* The command we are
 				   	   working on */
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+#define SCSI_SENSE_BUFFERSIZE	252
+#else
 #define SCSI_SENSE_BUFFERSIZE 	96
+#endif
 	unsigned char *sense_buffer;
 				/* obtained by REQUEST SENSE when
 				 * CHECK CONDITION is received on original
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 240aa6d..2935e61 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -52,8 +52,15 @@ enum scsi_device_state {
 enum scsi_device_event {
 	SDEV_EVT_MEDIA_CHANGE	= 1,	/* media has changed */
 	SDEV_EVT_CAPACITY_CHANGE	= 2,	/* capacity has changed */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	SDEV_EVT_CAPACITY_CHANGE_REPORTED	= 3,	/* UA reported */
+	SDEV_EVT_SOFT_THRESHOLD_REACHED	= 4,	/* UA soft threshold reached */
+	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED	= 5,	/* UA reported */
 
+	SDEV_EVT_LAST		= SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,
+#else
 	SDEV_EVT_LAST		= SDEV_EVT_CAPACITY_CHANGE,
+#endif
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
 
@@ -153,6 +160,16 @@ struct scsi_device {
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	atomic_t capacity_change_reported;	/* Device has reported that
+						   capacity has changed */
+	atomic_t soft_threshold_reached;	/* Device has reported that
+						   the thin provisioning soft
+						   threshold has been reached */
+	atomic_t mode_parameter_change_reported;/* Device has reported that
+						   mode parameter has changed */
+	struct delayed_work ua_dwork;
+#endif
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
@@ -271,6 +288,10 @@ struct scsi_target {
 	unsigned int		no_report_luns:1;	/* Don't use
 						 * REPORT LUNS for scanning. */
 #ifdef CONFIG_SCSI_ENHANCED_UA
+	atomic_t	lun_change_reported;	/* Target has reported that
+						   LUNs have changed */
+	struct delayed_work ua_dwork;
+
 	DECLARE_BITMAP(supported_events, STARGET_EVT_MAXBITS);
 	struct list_head	event_list;	/* asserted events */
 	struct work_struct	event_work;
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index ddf58cf..dc6dcfe 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -26,6 +26,9 @@ struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
 	u8 byte6;
 	u8 additional_length;	/* always 0 for fixed sense format */
 	unsigned int overflow:1;	/* sense data overflow */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	unsigned int ua_queue_overflow:1;	/* UA info lost by device */
+#endif
 };
 
 static inline int scsi_sense_valid(struct scsi_sense_hdr *sshdr)
-- 
1.7.11.7

[PATCH RFC 7/9] [SCSI] Add sysfs support for enhanced Unit Attention handling

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added support for additional scsi_device events in sysfs, as
well as support for scsi_target events.  Also added "rescan"
node in scsi_target sysfs to permit targets to be rescanned
from udev rules in a more straightforward way.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_priv.h  |   4 +-
 drivers/scsi/scsi_scan.c  |  29 +---------
 drivers/scsi/scsi_sysfs.c | 141 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 136 insertions(+), 38 deletions(-)

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index ddd2d31..21a73f2 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -136,7 +136,9 @@ extern int scsi_sysfs_add_host(struct Scsi_Host *);
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
 extern void scsi_sysfs_device_initialize(struct scsi_device *);
-extern int scsi_sysfs_target_initialize(struct scsi_device *);
+extern void scsi_sysfs_target_initialize(struct scsi_target *,
+					 struct Scsi_Host *,
+					 struct device *parent);
 extern struct scsi_transport_template blank_transport_template;
 extern void __scsi_remove_device(struct scsi_device *);
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f252c14..274de44 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -346,25 +346,6 @@ static void scsi_target_destroy(struct scsi_target *starget)
 	put_device(dev);
 }
 
-static void scsi_target_dev_release(struct device *dev)
-{
-	struct device *parent = dev->parent;
-	struct scsi_target *starget = to_scsi_target(dev);
-
-	kfree(starget);
-	put_device(parent);
-}
-
-static struct device_type scsi_target_type = {
-	.name =		"scsi_target",
-	.release =	scsi_target_dev_release,
-};
-
-int scsi_is_target_device(const struct device *dev)
-{
-	return dev->type == &scsi_target_type;
-}
-EXPORT_SYMBOL(scsi_is_target_device);
 
 static struct scsi_target *__scsi_find_target(struct device *parent,
 					      int channel, uint id)
@@ -416,15 +397,11 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 		printk(KERN_ERR "%s: allocation failure\n", __func__);
 		return NULL;
 	}
-	dev = &starget->dev;
-	device_initialize(dev);
-	starget->reap_ref = 1;
-	dev->parent = get_device(parent);
-	dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
-	dev->bus = &scsi_bus_type;
-	dev->type = &scsi_target_type;
 	starget->id = id;
 	starget->channel = channel;
+	scsi_sysfs_target_initialize(starget, shost, parent);
+	dev = &starget->dev;
+	starget->reap_ref = 1;
 	starget->can_queue = 0;
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index aff1e29..363e29f 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -502,7 +502,7 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 {									\
 	int ret;							\
 	struct scsi_device *sdev;					\
-	ret = scsi_sdev_check_buf_bit(buf);				\
+	ret = scsi_sysfs_check_buf_bit(buf);				\
 	if (ret >= 0)	{						\
 		sdev = to_scsi_device(dev);				\
 		sdev->field = ret;					\
@@ -513,10 +513,10 @@ sdev_store_##field (struct device *dev, struct device_attribute *attr,	\
 static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field);
 
 /*
- * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
+ * scsi_sysfs_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1",
  * else return -EINVAL.
  */
-static int scsi_sdev_check_buf_bit(const char *buf)
+static int scsi_sysfs_check_buf_bit(const char *buf)
 {
 	if ((buf[1] == '\0') || ((buf[1] == '\n') && (buf[2] == '\0'))) {
 		if (buf[0] == '1')
@@ -650,7 +650,8 @@ show_queue_type_field(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(queue_type, S_IRUGO, show_queue_type_field, NULL);
 
 static ssize_t
-show_iostat_counterbits(struct device *dev, struct device_attribute *attr, 				char *buf)
+show_iostat_counterbits(struct device *dev, struct device_attribute *attr,
+			char *buf)
 {
 	return snprintf(buf, 20, "%d\n", (int)sizeof(atomic_t) * 8);
 }
@@ -681,7 +682,7 @@ sdev_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR(modalias, S_IRUGO, sdev_show_modalias, NULL);
 
-#define DECLARE_EVT_SHOW(name, Cap_name)				\
+#define DECLARE_SDEV_EVT_SHOW(name, Cap_name)				\
 static ssize_t								\
 sdev_show_evt_##name(struct device *dev, struct device_attribute *attr,	\
 		     char *buf)						\
@@ -691,7 +692,7 @@ sdev_show_evt_##name(struct device *dev, struct device_attribute *attr,	\
 	return snprintf(buf, 20, "%d\n", val);				\
 }
 
-#define DECLARE_EVT_STORE(name, Cap_name)				\
+#define DECLARE_SDEV_EVT_STORE(name, Cap_name)				\
 static ssize_t								\
 sdev_store_evt_##name(struct device *dev, struct device_attribute *attr,\
 		      const char *buf, size_t count)			\
@@ -707,15 +708,20 @@ sdev_store_evt_##name(struct device *dev, struct device_attribute *attr,\
 	return count;							\
 }
 
-#define DECLARE_EVT(name, Cap_name)					\
-	DECLARE_EVT_SHOW(name, Cap_name)				\
-	DECLARE_EVT_STORE(name, Cap_name)				\
+#define DECLARE_SDEV_EVT(name, Cap_name)				\
+	DECLARE_SDEV_EVT_SHOW(name, Cap_name)				\
+	DECLARE_SDEV_EVT_STORE(name, Cap_name)				\
 	static DEVICE_ATTR(evt_##name, S_IRUGO, sdev_show_evt_##name,	\
 			   sdev_store_evt_##name);
 #define REF_EVT(name) &dev_attr_evt_##name.attr
 
-DECLARE_EVT(media_change, MEDIA_CHANGE)
-DECLARE_EVT(capacity_change, CAPACITY_CHANGE)
+DECLARE_SDEV_EVT(media_change, MEDIA_CHANGE)
+DECLARE_SDEV_EVT(capacity_change, CAPACITY_CHANGE)
+#ifdef CONFIG_SCSI_ENHANCED_UA
+DECLARE_SDEV_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
+DECLARE_SDEV_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED)
+DECLARE_SDEV_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
+#endif
 
 /* Default template for device attributes.  May NOT be modified */
 static struct attribute *scsi_sdev_attrs[] = {
@@ -736,6 +742,11 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_modalias.attr,
 	REF_EVT(media_change),
 	REF_EVT(capacity_change),
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	REF_EVT(capacity_change_reported),
+	REF_EVT(soft_threshold_reached),
+	REF_EVT(mode_parameter_change_reported),
+#endif
 	NULL
 };
 
@@ -1126,6 +1137,114 @@ int scsi_is_sdev_device(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_sdev_device);
 
+static ssize_t
+starget_store_rescan_field(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct scsi_target *starget = to_scsi_target(dev);
+
+	scsi_scan_target(starget->dev.parent, starget->channel, starget->id,
+			 SCAN_WILD_CARD, 1);
+	return count;
+}
+/* DEVICE_ATTR(rescan) clashes with dev_attr_rescan for sdev */
+struct device_attribute dev_attr_trescan =
+	__ATTR(rescan, S_IWUSR, NULL, starget_store_rescan_field);
+
+#define DECLARE_STARGET_EVT_SHOW(name, Cap_name)			\
+static ssize_t								\
+starget_show_evt_##name(struct device *dev,				\
+			struct device_attribute *attr, char *buf)	\
+{									\
+	struct scsi_target *starget = to_scsi_target(dev);		\
+	int val = test_bit(STARGET_EVT_##Cap_name,			\
+			   starget->supported_events);			\
+	return snprintf(buf, 20, "%d\n", val);				\
+}
+
+#define DECLARE_STARGET_EVT_STORE(name, Cap_name)			\
+static ssize_t								\
+starget_store_evt_##name(struct device *dev,				\
+			 struct device_attribute *attr,			\
+			 const char *buf, size_t count)			\
+{									\
+	struct scsi_target *starget = to_scsi_target(dev);		\
+	unsigned int val;						\
+	int err = kstrtouint(buf, 10, &val);				\
+	if (err != 0)							\
+		return err;						\
+	if (val == 0)							\
+		clear_bit(STARGET_EVT_##Cap_name,			\
+			  starget->supported_events);			\
+	else if (val == 1)						\
+		set_bit(STARGET_EVT_##Cap_name,				\
+			starget->supported_events);			\
+	else								\
+		return -EINVAL;						\
+	return count;							\
+}
+
+#define DECLARE_STARGET_EVT(name, Cap_name)				\
+	DECLARE_STARGET_EVT_SHOW(name, Cap_name)			\
+	DECLARE_STARGET_EVT_STORE(name, Cap_name)			\
+	static DEVICE_ATTR(evt_##name, S_IRUGO,				\
+			   starget_show_evt_##name,			\
+			   starget_store_evt_##name);
+
+#ifdef CONFIG_SCSI_ENHANCED_UA
+DECLARE_STARGET_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
+#endif
+
+static struct attribute *scsi_target_attrs[] = {
+	&dev_attr_trescan.attr,
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	REF_EVT(lun_change_reported),
+#endif
+	NULL
+};
+
+static struct attribute_group scsi_target_attr_group = {
+	.attrs =	scsi_target_attrs,
+};
+
+static const struct attribute_group *scsi_target_attr_groups[] = {
+	&scsi_target_attr_group,
+	NULL
+};
+
+static void scsi_target_dev_release(struct device *dev)
+{
+	struct device *parent = dev->parent;
+	struct scsi_target *starget = to_scsi_target(dev);
+
+	kfree(starget);
+	put_device(parent);
+}
+
+static struct device_type scsi_target_type = {
+	.name =		"scsi_target",
+	.release =	scsi_target_dev_release,
+	.groups =	scsi_target_attr_groups,
+};
+
+int scsi_is_target_device(const struct device *dev)
+{
+	return dev->type == &scsi_target_type;
+}
+EXPORT_SYMBOL(scsi_is_target_device);
+
+void scsi_sysfs_target_initialize(struct scsi_target *starget,
+				  struct Scsi_Host *shost,
+				  struct device *parent)
+{
+	device_initialize(&starget->dev);
+	starget->dev.parent = get_device(parent);
+	starget->dev.bus = &scsi_bus_type;
+	starget->dev.type = &scsi_target_type;
+	dev_set_name(&starget->dev, "target%d:%d:%d", shost->host_no,
+		     starget->channel, starget->id);
+}
+
 /* A blank transport template that is used in drivers that don't
  * yet implement Transport Attributes */
 struct scsi_transport_template blank_transport_template = { { { {NULL, }, }, }, };
-- 
1.7.11.7

[PATCH RFC 8/9] [SCSI] Add sense and Unit Attention generation to scsi_debug

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Added capability to scsi_debug to generate sense and Unit Attention
conditions to exercise the enhanced sense and Unit Attention handling.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_debug.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 88f6d16..8fe9bcd 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3051,10 +3051,27 @@ static ssize_t sdebug_max_luns_store(struct device_driver * ddp,
 				     const char * buf, size_t count)
 {
         int n;
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+#endif
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		scsi_debug_max_luns = n;
 		sdebug_max_tgts_luns();
+
+#ifdef CONFIG_SCSI_ENHANCED_UA
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x3f, 0x0e);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+#endif
 		return count;
 	}
 	return -EINVAL;
@@ -3101,12 +3118,28 @@ static ssize_t sdebug_virtual_gb_store(struct device_driver * ddp,
 				       const char * buf, size_t count)
 {
         int n;
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+#endif
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		scsi_debug_virtual_gb = n;
 
 		sdebug_capacity = get_sdebug_capacity();
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x2a, 0x09);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+#endif
 		return count;
 	}
 	return -EINVAL;
@@ -3233,6 +3266,90 @@ static ssize_t sdebug_sense_overflow_store(struct device_driver *ddp,
 }
 DRIVER_ATTR(sense_overflow, S_IWUSR, NULL, sdebug_sense_overflow_store);
 
+#ifdef CONFIG_SCSI_ENHANCED_UA
+static ssize_t sdebug_ua_overflow_store(struct device_driver *ddp,
+					const char *buf, size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				if (scsi_debug_dsense) {
+					mk_sense_buffer(devip, UNIT_ATTENTION,
+							0, 0);
+					devip->sense_buff[7] = 8;
+					devip->sense_buff[8] = 0x02;
+					devip->sense_buff[9] = 0x06;
+					devip->sense_buff[12] = 0x01;
+				}
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(ua_overflow, S_IWUSR, NULL, sdebug_ua_overflow_store);
+
+static ssize_t sdebug_soft_threshold_reached_store(struct device_driver *ddp,
+						   const char *buf,
+						   size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x38, 0x07);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(soft_threshold_reached, S_IWUSR, NULL,
+	    sdebug_soft_threshold_reached_store);
+
+static ssize_t sdebug_mode_parameters_changed_store(struct device_driver *ddp,
+						    const char *buf,
+						    size_t count)
+{
+	int n;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *devip;
+
+	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(devip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				mk_sense_buffer(devip, UNIT_ATTENTION,
+						0x2a, 0x01);
+				devip->sense_pending = 1;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
+		return count;
+	}
+	return -EINVAL;
+}
+DRIVER_ATTR(mode_parameters_changed, S_IWUSR, NULL,
+	    sdebug_mode_parameters_changed_store);
+#endif
+
 /* Note: The following function creates attribute files in the
    /sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
    files (over those found in the /sys/module/scsi_debug/parameters
@@ -3268,11 +3385,27 @@ static int do_create_driverfs_files(void)
 	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_map);
 	ret |= driver_create_file(&sdebug_driverfs_driver,
 				  &driver_attr_sense_overflow);
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_ua_overflow);
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_soft_threshold_reached);
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_mode_parameters_changed);
+#endif
 	return ret;
 }
 
 static void do_remove_driverfs_files(void)
 {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_mode_parameters_changed);
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_soft_threshold_reached);
+	driver_remove_file(&sdebug_driverfs_driver,
+			   &driver_attr_ua_overflow);
+#endif
 	driver_remove_file(&sdebug_driverfs_driver,
 			   &driver_attr_sense_overflow);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_map);
-- 
1.7.11.7

[PATCH RFC 9/9] [SCSI] Streamline detection of FM/EOM/ILI status

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Avoid duplicate tests when examining sense data for FM/EOM/ILI
bits.  Moved extraction of status to scsi_normalize_sense() if
the config option CONFIG_SCSI_ENHANCED_UA is used, because
descriptor format sense data is already being parsed there.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_error.c | 13 +++++++++++++
 include/scsi/scsi_eh.h    |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 3aa8706..c6344ef 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -332,6 +332,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	 * Previous logic looked for FILEMARK, EOM or ILI which are
 	 * mainly associated with tapes and returned SUCCESS.
 	 */
+#ifdef CONFIG_SCSI_ENHANCED_UA
+	if (sshdr.fm_eom_ili)
+		return SUCCESS;
+#else
 	if (sshdr.response_code == 0x70) {
 		/* fixed format */
 		if (scmd->sense_buffer[2] & 0xe0)
@@ -347,6 +351,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		    (scmd->sense_buffer[11] & 0xe0))
 			return SUCCESS;
 	}
+#endif
 
 	switch (sshdr.sense_key) {
 	case NO_SENSE:
@@ -2192,6 +2197,11 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 				    ((sense_buffer[desc_pos + 4] & 0x01) != 0))
 					sshdr->ua_queue_overflow = 1;
 
+				if ((desc_type == 0x04) && (desc_len >= 4) &&
+				    (addl_len >= 2) &&
+				    ((sense_buffer[desc_pos + 3] & 0xe0) != 0))
+					sshdr->fm_eom_ili = 1;
+
 				if (addl_len > (desc_len - 2))
 					addl_len = desc_len - 2;
 
@@ -2205,6 +2215,9 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 		 * fixed format
 		 */
 		if (sb_len > 2) {
+#ifdef CONFIG_SCSI_ENHANCED_UA
+			sshdr->fm_eom_ili = ((sense_buffer[2] & 0xe0) != 0);
+#endif
 			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
 			sshdr->sense_key = (sense_buffer[2] & 0xf);
 		}
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index dc6dcfe..136dc71 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -28,6 +28,9 @@ struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
 	unsigned int overflow:1;	/* sense data overflow */
 #ifdef CONFIG_SCSI_ENHANCED_UA
 	unsigned int ua_queue_overflow:1;	/* UA info lost by device */
+
+	unsigned int fm_eom_ili:1;	/* filemark, end of medium, or
+					   incorrect length indicator set */
 #endif
 };
 
-- 
1.7.11.7

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[James Bottomley]

On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>  	if (! scsi_command_normalize_sense(scmd, &sshdr))
>  		return FAILED;	/* no valid sense data */
>  
> +	if (sshdr.overflow)
> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> +
>  	if (scsi_sense_is_deferred(&sshdr))
>  		return NEEDS_RETRY;
>  
> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>  			sshdr->asc = sense_buffer[2];
>  		if (sb_len > 3)
>  			sshdr->ascq = sense_buffer[3];
> +		if (sb_len > 4)
> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
>  		if (sb_len > 7)
>  			sshdr->additional_length = sense_buffer[7];
>  	} else {
>  		/*
>  		 * fixed format
>  		 */
> -		if (sb_len > 2)
> +		if (sb_len > 2) {
> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
>  			sshdr->sense_key = (sense_buffer[2] & 0xf);
> +		}
>  		if (sb_len > 7) {
>  			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
>  					 sb_len : (sense_buffer[7] + 8);

This isn't the right way to do it:  The overflow bit is a recent
introduction in SPC-4.  The correct way to tell if we have an overflow
or not is to look at the additional sense length and compare it to the
allocation length; this will work for everything.

I'm not even convinced that overflow is important: for a lot of the
sense probes, we deliberately induce overflows by giving the request
sense command a short buffer.  Printing a warning in scsi_check_sense
will get very noisy very fast.

James

Re: [PATCH RFC 8/9] [SCSI] Add sense and Unit Attention generation to scsi_debug

[Douglas Gilbert]

On 13-01-18 11:27 AM, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
>
> Added capability to scsi_debug to generate sense and Unit Attention
> conditions to exercise the enhanced sense and Unit Attention handling.
>
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>   drivers/scsi/scsi_debug.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 133 insertions(+)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 88f6d16..8fe9bcd 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3051,10 +3051,27 @@ static ssize_t sdebug_max_luns_store(struct device_driver * ddp,
>   				     const char * buf, size_t count)
>   {
>           int n;
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	struct sdebug_host_info *sdbg_host;
> +	struct sdebug_dev_info *devip;
> +#endif
>
>   	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
>   		scsi_debug_max_luns = n;
>   		sdebug_max_tgts_luns();
> +
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +		spin_lock(&sdebug_host_list_lock);
> +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> +					    dev_list) {
> +				mk_sense_buffer(devip, UNIT_ATTENTION,
> +						0x3f, 0x0e);
> +				devip->sense_pending = 1;
> +			}
> +		}
> +		spin_unlock(&sdebug_host_list_lock);
> +#endif
>   		return count;
>   	}
>   	return -EINVAL;
> @@ -3101,12 +3118,28 @@ static ssize_t sdebug_virtual_gb_store(struct device_driver * ddp,
>   				       const char * buf, size_t count)
>   {
>           int n;
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	struct sdebug_host_info *sdbg_host;
> +	struct sdebug_dev_info *devip;
> +#endif
>
>   	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
>   		scsi_debug_virtual_gb = n;
>
>   		sdebug_capacity = get_sdebug_capacity();
>
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +		spin_lock(&sdebug_host_list_lock);
> +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> +					    dev_list) {
> +				mk_sense_buffer(devip, UNIT_ATTENTION,
> +						0x2a, 0x09);
> +				devip->sense_pending = 1;
> +			}
> +		}
> +		spin_unlock(&sdebug_host_list_lock);
> +#endif
>   		return count;
>   	}
>   	return -EINVAL;
> @@ -3233,6 +3266,90 @@ static ssize_t sdebug_sense_overflow_store(struct device_driver *ddp,
>   }
>   DRIVER_ATTR(sense_overflow, S_IWUSR, NULL, sdebug_sense_overflow_store);
>
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +static ssize_t sdebug_ua_overflow_store(struct device_driver *ddp,
> +					const char *buf, size_t count)
> +{
> +	int n;
> +	struct sdebug_host_info *sdbg_host;
> +	struct sdebug_dev_info *devip;
> +
> +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> +		spin_lock(&sdebug_host_list_lock);
> +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> +					    dev_list) {
> +				if (scsi_debug_dsense) {
> +					mk_sense_buffer(devip, UNIT_ATTENTION,
> +							0, 0);
> +					devip->sense_buff[7] = 8;
> +					devip->sense_buff[8] = 0x02;
> +					devip->sense_buff[9] = 0x06;
> +					devip->sense_buff[12] = 0x01;
> +				}
> +				devip->sense_pending = 1;
> +			}
> +		}
> +		spin_unlock(&sdebug_host_list_lock);
> +		return count;
> +	}
> +	return -EINVAL;
> +}
> +DRIVER_ATTR(ua_overflow, S_IWUSR, NULL, sdebug_ua_overflow_store);
> +
> +static ssize_t sdebug_soft_threshold_reached_store(struct device_driver *ddp,
> +						   const char *buf,
> +						   size_t count)
> +{
> +	int n;
> +	struct sdebug_host_info *sdbg_host;
> +	struct sdebug_dev_info *devip;
> +
> +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> +		spin_lock(&sdebug_host_list_lock);
> +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> +					    dev_list) {
> +				mk_sense_buffer(devip, UNIT_ATTENTION,
> +						0x38, 0x07);
> +				devip->sense_pending = 1;
> +			}
> +		}
> +		spin_unlock(&sdebug_host_list_lock);
> +		return count;
> +	}
> +	return -EINVAL;
> +}
> +DRIVER_ATTR(soft_threshold_reached, S_IWUSR, NULL,
> +	    sdebug_soft_threshold_reached_store);
> +
> +static ssize_t sdebug_mode_parameters_changed_store(struct device_driver *ddp,
> +						    const char *buf,
> +						    size_t count)
> +{
> +	int n;
> +	struct sdebug_host_info *sdbg_host;
> +	struct sdebug_dev_info *devip;
> +
> +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> +		spin_lock(&sdebug_host_list_lock);
> +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> +					    dev_list) {
> +				mk_sense_buffer(devip, UNIT_ATTENTION,
> +						0x2a, 0x01);
> +				devip->sense_pending = 1;
> +			}
> +		}
> +		spin_unlock(&sdebug_host_list_lock);
> +		return count;
> +	}
> +	return -EINVAL;
> +}
> +DRIVER_ATTR(mode_parameters_changed, S_IWUSR, NULL,
> +	    sdebug_mode_parameters_changed_store);
> +#endif
> +
>   /* Note: The following function creates attribute files in the
>      /sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
>      files (over those found in the /sys/module/scsi_debug/parameters
> @@ -3268,11 +3385,27 @@ static int do_create_driverfs_files(void)
>   	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_map);
>   	ret |= driver_create_file(&sdebug_driverfs_driver,
>   				  &driver_attr_sense_overflow);
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	ret |= driver_create_file(&sdebug_driverfs_driver,
> +				  &driver_attr_ua_overflow);
> +	ret |= driver_create_file(&sdebug_driverfs_driver,
> +				  &driver_attr_soft_threshold_reached);
> +	ret |= driver_create_file(&sdebug_driverfs_driver,
> +				  &driver_attr_mode_parameters_changed);
> +#endif
>   	return ret;
>   }
>
>   static void do_remove_driverfs_files(void)
>   {
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	driver_remove_file(&sdebug_driverfs_driver,
> +			   &driver_attr_mode_parameters_changed);
> +	driver_remove_file(&sdebug_driverfs_driver,
> +			   &driver_attr_soft_threshold_reached);
> +	driver_remove_file(&sdebug_driverfs_driver,
> +			   &driver_attr_ua_overflow);
> +#endif
>   	driver_remove_file(&sdebug_driverfs_driver,
>   			   &driver_attr_sense_overflow);
>   	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_map);

Ewan,
Looks good. Could you add MODULE_PARM_DESC macros (in
alphabetical order) for mode_parameters_changed,
soft_threshold_reached and ua_overflow?

Doug Gilbert

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Hannes Reinecke]

On 01/18/2013 05:46 PM, James Bottomley wrote:
> On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>   	if (! scsi_command_normalize_sense(scmd, &sshdr))
>>   		return FAILED;	/* no valid sense data */
>>
>> +	if (sshdr.overflow)
>> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
>> +
>>   	if (scsi_sense_is_deferred(&sshdr))
>>   		return NEEDS_RETRY;
>>
>> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>>   			sshdr->asc = sense_buffer[2];
>>   		if (sb_len > 3)
>>   			sshdr->ascq = sense_buffer[3];
>> +		if (sb_len > 4)
>> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
>>   		if (sb_len > 7)
>>   			sshdr->additional_length = sense_buffer[7];
>>   	} else {
>>   		/*
>>   		 * fixed format
>>   		 */
>> -		if (sb_len > 2)
>> +		if (sb_len > 2) {
>> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
>>   			sshdr->sense_key = (sense_buffer[2] & 0xf);
>> +		}
>>   		if (sb_len > 7) {
>>   			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
>>   					 sb_len : (sense_buffer[7] + 8);
>
> This isn't the right way to do it:  The overflow bit is a recent
> introduction in SPC-4.  The correct way to tell if we have an overflow
> or not is to look at the additional sense length and compare it to the
> allocation length; this will work for everything.
>
> I'm not even convinced that overflow is important: for a lot of the
> sense probes, we deliberately induce overflows by giving the request
> sense command a short buffer.  Printing a warning in scsi_check_sense
> will get very noisy very fast.
>
And indeed I would rather prefer to have it the other way round;
we're using a fixed sense_buffer within the SCSI stack, which might
not be large enough to hold all sense data.
So I would prefer to have an indicator on whether _the internal_ 
sense buffer overflowed; this would even give us some valid use-case 
now.
Plus we can add the sense buffer overflow bit to that if required.

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

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[James Bottomley]

On Mon, 2013-01-21 at 08:26 +0100, Hannes Reinecke wrote:
> On 01/18/2013 05:46 PM, James Bottomley wrote:
> > On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> >> --- a/drivers/scsi/scsi_error.c
> >> +++ b/drivers/scsi/scsi_error.c
> >> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> >>   	if (! scsi_command_normalize_sense(scmd, &sshdr))
> >>   		return FAILED;	/* no valid sense data */
> >>
> >> +	if (sshdr.overflow)
> >> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> >> +
> >>   	if (scsi_sense_is_deferred(&sshdr))
> >>   		return NEEDS_RETRY;
> >>
> >> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> >>   			sshdr->asc = sense_buffer[2];
> >>   		if (sb_len > 3)
> >>   			sshdr->ascq = sense_buffer[3];
> >> +		if (sb_len > 4)
> >> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
> >>   		if (sb_len > 7)
> >>   			sshdr->additional_length = sense_buffer[7];
> >>   	} else {
> >>   		/*
> >>   		 * fixed format
> >>   		 */
> >> -		if (sb_len > 2)
> >> +		if (sb_len > 2) {
> >> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
> >>   			sshdr->sense_key = (sense_buffer[2] & 0xf);
> >> +		}
> >>   		if (sb_len > 7) {
> >>   			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> >>   					 sb_len : (sense_buffer[7] + 8);
> >
> > This isn't the right way to do it:  The overflow bit is a recent
> > introduction in SPC-4.  The correct way to tell if we have an overflow
> > or not is to look at the additional sense length and compare it to the
> > allocation length; this will work for everything.
> >
> > I'm not even convinced that overflow is important: for a lot of the
> > sense probes, we deliberately induce overflows by giving the request
> > sense command a short buffer.  Printing a warning in scsi_check_sense
> > will get very noisy very fast.
> >
> And indeed I would rather prefer to have it the other way round;
> we're using a fixed sense_buffer within the SCSI stack, which might
> not be large enough to hold all sense data.
> So I would prefer to have an indicator on whether _the internal_ 
> sense buffer overflowed; this would even give us some valid use-case 
> now.
> Plus we can add the sense buffer overflow bit to that if required.

Probably the most useful field to add would be total_length.  That tells
you immediately (since you know the size of the buffer you sent in)
whether there's an overflow and whether you care.  Just knowing there's
an overflow isn't much help because you still don't know whether it was
significant or not.

James

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Douglas Gilbert]

On 13-01-21 02:26 AM, Hannes Reinecke wrote:
> On 01/18/2013 05:46 PM, James Bottomley wrote:
>> On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
>>> --- a/drivers/scsi/scsi_error.c
>>> +++ b/drivers/scsi/scsi_error.c
>>> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>>       if (! scsi_command_normalize_sense(scmd, &sshdr))
>>>           return FAILED;    /* no valid sense data */
>>>
>>> +    if (sshdr.overflow)
>>> +        scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
>>> +
>>>       if (scsi_sense_is_deferred(&sshdr))
>>>           return NEEDS_RETRY;
>>>
>>> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int
>>> sb_len,
>>>               sshdr->asc = sense_buffer[2];
>>>           if (sb_len > 3)
>>>               sshdr->ascq = sense_buffer[3];
>>> +        if (sb_len > 4)
>>> +            sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
>>>           if (sb_len > 7)
>>>               sshdr->additional_length = sense_buffer[7];
>>>       } else {
>>>           /*
>>>            * fixed format
>>>            */
>>> -        if (sb_len > 2)
>>> +        if (sb_len > 2) {
>>> +            sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
>>>               sshdr->sense_key = (sense_buffer[2] & 0xf);
>>> +        }
>>>           if (sb_len > 7) {
>>>               sb_len = (sb_len < (sense_buffer[7] + 8)) ?
>>>                        sb_len : (sense_buffer[7] + 8);
>>
>> This isn't the right way to do it:  The overflow bit is a recent
>> introduction in SPC-4.  The correct way to tell if we have an overflow
>> or not is to look at the additional sense length and compare it to the
>> allocation length; this will work for everything.
>>
>> I'm not even convinced that overflow is important: for a lot of the
>> sense probes, we deliberately induce overflows by giving the request
>> sense command a short buffer.  Printing a warning in scsi_check_sense
>> will get very noisy very fast.
>>
> And indeed I would rather prefer to have it the other way round;
> we're using a fixed sense_buffer within the SCSI stack, which might
> not be large enough to hold all sense data.
> So I would prefer to have an indicator on whether _the internal_ sense buffer
> overflowed; this would even give us some valid use-case now.
> Plus we can add the sense buffer overflow bit to that if required.

In a related move, t10.org added a "Maximum sense data length" field
in the Control mode page in SPC-4 revision 34 (21 February 2012).
The LU can indicate the maximum length it supports with the "maximum
supported sense data length" field in the Extended Inquiry VPD page.
And both the fixed and descriptor sense format now have a SDAT_OVFL
bit indicating whether the accompanying sense data has been truncated.

Doug Gilbert

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Ewan Milne]

On Fri, 2013-01-18 at 16:46 +0000, James Bottomley wrote:
> On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> >  	if (! scsi_command_normalize_sense(scmd, &sshdr))
> >  		return FAILED;	/* no valid sense data */
> >  
> > +	if (sshdr.overflow)
> > +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> > +
> >  	if (scsi_sense_is_deferred(&sshdr))
> >  		return NEEDS_RETRY;
> >  
> > @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> >  			sshdr->asc = sense_buffer[2];
> >  		if (sb_len > 3)
> >  			sshdr->ascq = sense_buffer[3];
> > +		if (sb_len > 4)
> > +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
> >  		if (sb_len > 7)
> >  			sshdr->additional_length = sense_buffer[7];
> >  	} else {
> >  		/*
> >  		 * fixed format
> >  		 */
> > -		if (sb_len > 2)
> > +		if (sb_len > 2) {
> > +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
> >  			sshdr->sense_key = (sense_buffer[2] & 0xf);
> > +		}
> >  		if (sb_len > 7) {
> >  			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> >  					 sb_len : (sense_buffer[7] + 8);
> 
> This isn't the right way to do it:  The overflow bit is a recent
> introduction in SPC-4.  The correct way to tell if we have an overflow
> or not is to look at the additional sense length and compare it to the
> allocation length; this will work for everything.

Unfortunately, I am not sure that the allocation length that was sent
to the device is always available.  I will look into this more closely
but it appeared to me that e.g. FC drivers like qla2xxx get the sense
data automatically from the HBA firmware.  In the case of that driver
the host sense buffer size looks like it is hard-coded to 32 bytes,
for all I know the firmware might only asking for 18 bytes.

Of course, for a normal REQUEST SENSE command where the allocation
length is in the CDB, it would indeed be easy to add a check against
the additional sense length.

> 
> I'm not even convinced that overflow is important: for a lot of the
> sense probes, we deliberately induce overflows by giving the request
> sense command a short buffer.  Printing a warning in scsi_check_sense
> will get very noisy very fast.

That would indeed be a problem.  I didn't see this behavior when testing
the changes but I'll need to investigate this further.

The purpose of detecting the sense data overflow was to provide some
visibility that a device is returning a large amount of sense data that
is currently being silently ignored.  In the case of descriptor format
sense data, it is possible that a descriptor we want to examine is
located after one or more other descriptors, and we might not get it
at all if the buffer isn't large enough.

> 
> James

Thanks for your comments.

> 
> 

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Ewan Milne]

On Mon, 2013-01-21 at 08:26 +0100, Hannes Reinecke wrote:
> On 01/18/2013 05:46 PM, James Bottomley wrote:
> > On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> >> --- a/drivers/scsi/scsi_error.c
> >> +++ b/drivers/scsi/scsi_error.c
> >> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> >>   	if (! scsi_command_normalize_sense(scmd, &sshdr))
> >>   		return FAILED;	/* no valid sense data */
> >>
> >> +	if (sshdr.overflow)
> >> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> >> +
> >>   	if (scsi_sense_is_deferred(&sshdr))
> >>   		return NEEDS_RETRY;
> >>
> >> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> >>   			sshdr->asc = sense_buffer[2];
> >>   		if (sb_len > 3)
> >>   			sshdr->ascq = sense_buffer[3];
> >> +		if (sb_len > 4)
> >> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
> >>   		if (sb_len > 7)
> >>   			sshdr->additional_length = sense_buffer[7];
> >>   	} else {
> >>   		/*
> >>   		 * fixed format
> >>   		 */
> >> -		if (sb_len > 2)
> >> +		if (sb_len > 2) {
> >> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
> >>   			sshdr->sense_key = (sense_buffer[2] & 0xf);
> >> +		}
> >>   		if (sb_len > 7) {
> >>   			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> >>   					 sb_len : (sense_buffer[7] + 8);
> >
> > This isn't the right way to do it:  The overflow bit is a recent
> > introduction in SPC-4.  The correct way to tell if we have an overflow
> > or not is to look at the additional sense length and compare it to the
> > allocation length; this will work for everything.
> >
> > I'm not even convinced that overflow is important: for a lot of the
> > sense probes, we deliberately induce overflows by giving the request
> > sense command a short buffer.  Printing a warning in scsi_check_sense
> > will get very noisy very fast.
> >
> And indeed I would rather prefer to have it the other way round;
> we're using a fixed sense_buffer within the SCSI stack, which might
> not be large enough to hold all sense data.
> So I would prefer to have an indicator on whether _the internal_ 
> sense buffer overflowed; this would even give us some valid use-case 
> now.

So, if I understand what you're saying, we could check for overflow if
(sense_data[7] + 8) > SCSI_SENSE_BUFFERSIZE

We could do that, certainly.  I think, though, that overflow of sense
data is more likely to occur if a sense buffer smaller than
SCSI_SENSE_BUFFERSIZE bytes is used, e.g. by a call to
scsi_eh_prep_cmnd(), which is an exported symbol.

The existing 96-byte SCSI_SENSE_BUFFERSIZE may well be big enough.
(I did increase it to the SPC-4 defined value of 252 bytes in a later
patch in the series if the appropriate kernel config option is enabled.)

> Plus we can add the sense buffer overflow bit to that if required.
> 
> Cheers,
> 
> Hannes

Thanks for your comments.

Re: [PATCH RFC 8/9] [SCSI] Add sense and Unit Attention generation to scsi_debug

[Ewan Milne]

On Sat, 2013-01-19 at 13:43 -0500, Douglas Gilbert wrote:
> On 13-01-18 11:27 AM, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> >
> > Added capability to scsi_debug to generate sense and Unit Attention
> > conditions to exercise the enhanced sense and Unit Attention handling.
> >
> > Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> > ---
> >   drivers/scsi/scsi_debug.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 133 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index 88f6d16..8fe9bcd 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -3051,10 +3051,27 @@ static ssize_t sdebug_max_luns_store(struct device_driver * ddp,
> >   				     const char * buf, size_t count)
> >   {
> >           int n;
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +	struct sdebug_host_info *sdbg_host;
> > +	struct sdebug_dev_info *devip;
> > +#endif
> >
> >   	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> >   		scsi_debug_max_luns = n;
> >   		sdebug_max_tgts_luns();
> > +
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +		spin_lock(&sdebug_host_list_lock);
> > +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> > +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> > +					    dev_list) {
> > +				mk_sense_buffer(devip, UNIT_ATTENTION,
> > +						0x3f, 0x0e);
> > +				devip->sense_pending = 1;
> > +			}
> > +		}
> > +		spin_unlock(&sdebug_host_list_lock);
> > +#endif
> >   		return count;
> >   	}
> >   	return -EINVAL;
> > @@ -3101,12 +3118,28 @@ static ssize_t sdebug_virtual_gb_store(struct device_driver * ddp,
> >   				       const char * buf, size_t count)
> >   {
> >           int n;
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +	struct sdebug_host_info *sdbg_host;
> > +	struct sdebug_dev_info *devip;
> > +#endif
> >
> >   	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> >   		scsi_debug_virtual_gb = n;
> >
> >   		sdebug_capacity = get_sdebug_capacity();
> >
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +		spin_lock(&sdebug_host_list_lock);
> > +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> > +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> > +					    dev_list) {
> > +				mk_sense_buffer(devip, UNIT_ATTENTION,
> > +						0x2a, 0x09);
> > +				devip->sense_pending = 1;
> > +			}
> > +		}
> > +		spin_unlock(&sdebug_host_list_lock);
> > +#endif
> >   		return count;
> >   	}
> >   	return -EINVAL;
> > @@ -3233,6 +3266,90 @@ static ssize_t sdebug_sense_overflow_store(struct device_driver *ddp,
> >   }
> >   DRIVER_ATTR(sense_overflow, S_IWUSR, NULL, sdebug_sense_overflow_store);
> >
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +static ssize_t sdebug_ua_overflow_store(struct device_driver *ddp,
> > +					const char *buf, size_t count)
> > +{
> > +	int n;
> > +	struct sdebug_host_info *sdbg_host;
> > +	struct sdebug_dev_info *devip;
> > +
> > +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> > +		spin_lock(&sdebug_host_list_lock);
> > +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> > +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> > +					    dev_list) {
> > +				if (scsi_debug_dsense) {
> > +					mk_sense_buffer(devip, UNIT_ATTENTION,
> > +							0, 0);
> > +					devip->sense_buff[7] = 8;
> > +					devip->sense_buff[8] = 0x02;
> > +					devip->sense_buff[9] = 0x06;
> > +					devip->sense_buff[12] = 0x01;
> > +				}
> > +				devip->sense_pending = 1;
> > +			}
> > +		}
> > +		spin_unlock(&sdebug_host_list_lock);
> > +		return count;
> > +	}
> > +	return -EINVAL;
> > +}
> > +DRIVER_ATTR(ua_overflow, S_IWUSR, NULL, sdebug_ua_overflow_store);
> > +
> > +static ssize_t sdebug_soft_threshold_reached_store(struct device_driver *ddp,
> > +						   const char *buf,
> > +						   size_t count)
> > +{
> > +	int n;
> > +	struct sdebug_host_info *sdbg_host;
> > +	struct sdebug_dev_info *devip;
> > +
> > +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> > +		spin_lock(&sdebug_host_list_lock);
> > +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> > +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> > +					    dev_list) {
> > +				mk_sense_buffer(devip, UNIT_ATTENTION,
> > +						0x38, 0x07);
> > +				devip->sense_pending = 1;
> > +			}
> > +		}
> > +		spin_unlock(&sdebug_host_list_lock);
> > +		return count;
> > +	}
> > +	return -EINVAL;
> > +}
> > +DRIVER_ATTR(soft_threshold_reached, S_IWUSR, NULL,
> > +	    sdebug_soft_threshold_reached_store);
> > +
> > +static ssize_t sdebug_mode_parameters_changed_store(struct device_driver *ddp,
> > +						    const char *buf,
> > +						    size_t count)
> > +{
> > +	int n;
> > +	struct sdebug_host_info *sdbg_host;
> > +	struct sdebug_dev_info *devip;
> > +
> > +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> > +		spin_lock(&sdebug_host_list_lock);
> > +		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> > +			list_for_each_entry(devip, &sdbg_host->dev_info_list,
> > +					    dev_list) {
> > +				mk_sense_buffer(devip, UNIT_ATTENTION,
> > +						0x2a, 0x01);
> > +				devip->sense_pending = 1;
> > +			}
> > +		}
> > +		spin_unlock(&sdebug_host_list_lock);
> > +		return count;
> > +	}
> > +	return -EINVAL;
> > +}
> > +DRIVER_ATTR(mode_parameters_changed, S_IWUSR, NULL,
> > +	    sdebug_mode_parameters_changed_store);
> > +#endif
> > +
> >   /* Note: The following function creates attribute files in the
> >      /sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
> >      files (over those found in the /sys/module/scsi_debug/parameters
> > @@ -3268,11 +3385,27 @@ static int do_create_driverfs_files(void)
> >   	ret |= driver_create_file(&sdebug_driverfs_driver, &driver_attr_map);
> >   	ret |= driver_create_file(&sdebug_driverfs_driver,
> >   				  &driver_attr_sense_overflow);
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +	ret |= driver_create_file(&sdebug_driverfs_driver,
> > +				  &driver_attr_ua_overflow);
> > +	ret |= driver_create_file(&sdebug_driverfs_driver,
> > +				  &driver_attr_soft_threshold_reached);
> > +	ret |= driver_create_file(&sdebug_driverfs_driver,
> > +				  &driver_attr_mode_parameters_changed);
> > +#endif
> >   	return ret;
> >   }
> >
> >   static void do_remove_driverfs_files(void)
> >   {
> > +#ifdef CONFIG_SCSI_ENHANCED_UA
> > +	driver_remove_file(&sdebug_driverfs_driver,
> > +			   &driver_attr_mode_parameters_changed);
> > +	driver_remove_file(&sdebug_driverfs_driver,
> > +			   &driver_attr_soft_threshold_reached);
> > +	driver_remove_file(&sdebug_driverfs_driver,
> > +			   &driver_attr_ua_overflow);
> > +#endif
> >   	driver_remove_file(&sdebug_driverfs_driver,
> >   			   &driver_attr_sense_overflow);
> >   	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_map);
> 
> Ewan,
> Looks good. Could you add MODULE_PARM_DESC macros (in
> alphabetical order) for mode_parameters_changed,
> soft_threshold_reached and ua_overflow?

It wasn't my intention to add these as module parameters, I only wanted
them to be writable via sysfs.  This is because they are not attributes,
they are like the "rescan" sysfs mechanism in [scsi_sysfs.c] in that
writing to them causes an action to be performed.

> 
> Doug Gilbert
> 

Thanks for your comments.

Re: [PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Bart Van Assche]

On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
>
> @@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>   *     Dispatch queued events to their associated scsi_device kobjects
>   *     as uevents.
>   */
> -void scsi_evt_thread(struct work_struct *work)
> +void sdev_evt_thread(struct work_struct *work)
>  {
>         struct scsi_device *sdev;
>         LIST_HEAD(event_list);

This code does not run as a long-living thread so please consider
using the name "sdev_evt_work" instead.

Bart.

Re: [PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Bart Van Assche]

On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> @@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>   *     Dispatch queued events to their associated scsi_device kobjects
>   *     as uevents.
>   */
> -void scsi_evt_thread(struct work_struct *work)
> +void sdev_evt_thread(struct work_struct *work)
>  {
>         struct scsi_device *sdev;
>         LIST_HEAD(event_list);
> @@ -2214,7 +2214,7 @@ void scsi_evt_thread(struct work_struct *work)
>         sdev = container_of(work, struct scsi_device, event_work);
>
>         while (1) {
> -               struct scsi_event *evt;
> +               struct sdev_event *evt;
>                 struct list_head *this, *tmp;
>                 unsigned long flags;
>
> @@ -2226,9 +2226,9 @@ void scsi_evt_thread(struct work_struct *work)
>                         break;
>
>                 list_for_each_safe(this, tmp, &event_list) {
> -                       evt = list_entry(this, struct scsi_event, node);
> +                       evt = list_entry(this, struct sdev_event, node);
>                         list_del(&evt->node);
> -                       scsi_evt_emit(sdev, evt);
> +                       sdev_evt_emit(sdev, evt);
>                         kfree(evt);
>                 }
>         }

If schedule_work() gets invoked if work is already on a workqueue then
it will return immediately. Does that mean that the above approach for
processing the event list is racy and that new events will not get
processed if schedule_work() is invoked after the while loop finished
but before the above function returns ?

Bart.

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Hannes Reinecke]

On 01/22/2013 04:10 PM, Ewan Milne wrote:
> On Mon, 2013-01-21 at 08:26 +0100, Hannes Reinecke wrote:
>> On 01/18/2013 05:46 PM, James Bottomley wrote:
>>> On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
>>>> --- a/drivers/scsi/scsi_error.c
>>>> +++ b/drivers/scsi/scsi_error.c
>>>> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>>>    	if (! scsi_command_normalize_sense(scmd, &sshdr))
>>>>    		return FAILED;	/* no valid sense data */
>>>>
>>>> +	if (sshdr.overflow)
>>>> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
>>>> +
>>>>    	if (scsi_sense_is_deferred(&sshdr))
>>>>    		return NEEDS_RETRY;
>>>>
>>>> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>>>>    			sshdr->asc = sense_buffer[2];
>>>>    		if (sb_len > 3)
>>>>    			sshdr->ascq = sense_buffer[3];
>>>> +		if (sb_len > 4)
>>>> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
>>>>    		if (sb_len > 7)
>>>>    			sshdr->additional_length = sense_buffer[7];
>>>>    	} else {
>>>>    		/*
>>>>    		 * fixed format
>>>>    		 */
>>>> -		if (sb_len > 2)
>>>> +		if (sb_len > 2) {
>>>> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
>>>>    			sshdr->sense_key = (sense_buffer[2] & 0xf);
>>>> +		}
>>>>    		if (sb_len > 7) {
>>>>    			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
>>>>    					 sb_len : (sense_buffer[7] + 8);
>>>
>>> This isn't the right way to do it:  The overflow bit is a recent
>>> introduction in SPC-4.  The correct way to tell if we have an overflow
>>> or not is to look at the additional sense length and compare it to the
>>> allocation length; this will work for everything.
>>>
>>> I'm not even convinced that overflow is important: for a lot of the
>>> sense probes, we deliberately induce overflows by giving the request
>>> sense command a short buffer.  Printing a warning in scsi_check_sense
>>> will get very noisy very fast.
>>>
>> And indeed I would rather prefer to have it the other way round;
>> we're using a fixed sense_buffer within the SCSI stack, which might
>> not be large enough to hold all sense data.
>> So I would prefer to have an indicator on whether _the internal_
>> sense buffer overflowed; this would even give us some valid use-case
>> now.
>
> So, if I understand what you're saying, we could check for overflow if
> (sense_data[7] + 8) > SCSI_SENSE_BUFFERSIZE
>
Precisely.

> We could do that, certainly.  I think, though, that overflow of sense
> data is more likely to occur if a sense buffer smaller than
> SCSI_SENSE_BUFFERSIZE bytes is used, e.g. by a call to
> scsi_eh_prep_cmnd(), which is an exported symbol.
>
> The existing 96-byte SCSI_SENSE_BUFFERSIZE may well be big enough.
> (I did increase it to the SPC-4 defined value of 252 bytes in a later
> patch in the series if the appropriate kernel config option is enabled.)
>
Indeed, it _may_.

I just would like to have an indicator on whether it actually _is_.
Currently we have no way of telling.
Seeing that we want to implement _real_ sense code handling we
really want to know whether we've missed some information.

Relying on the SPC-defined values doesn't really cut it, as it's 
only in the very latest draft and it'll be ages until we're seeing
these devices in the field. So we have to find a way of handling
older devices, which may yet send us large sense data.
(Think of referrals ...)

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

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Hannes Reinecke]

On 01/22/2013 04:08 PM, Ewan Milne wrote:
> On Fri, 2013-01-18 at 16:46 +0000, James Bottomley wrote:
>> On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
>>> --- a/drivers/scsi/scsi_error.c
>>> +++ b/drivers/scsi/scsi_error.c
>>> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>>   	if (! scsi_command_normalize_sense(scmd, &sshdr))
>>>   		return FAILED;	/* no valid sense data */
>>>
>>> +	if (sshdr.overflow)
>>> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
>>> +
>>>   	if (scsi_sense_is_deferred(&sshdr))
>>>   		return NEEDS_RETRY;
>>>
>>> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>>>   			sshdr->asc = sense_buffer[2];
>>>   		if (sb_len > 3)
>>>   			sshdr->ascq = sense_buffer[3];
>>> +		if (sb_len > 4)
>>> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
>>>   		if (sb_len > 7)
>>>   			sshdr->additional_length = sense_buffer[7];
>>>   	} else {
>>>   		/*
>>>   		 * fixed format
>>>   		 */
>>> -		if (sb_len > 2)
>>> +		if (sb_len > 2) {
>>> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
>>>   			sshdr->sense_key = (sense_buffer[2] & 0xf);
>>> +		}
>>>   		if (sb_len > 7) {
>>>   			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
>>>   					 sb_len : (sense_buffer[7] + 8);
>>
>> This isn't the right way to do it:  The overflow bit is a recent
>> introduction in SPC-4.  The correct way to tell if we have an overflow
>> or not is to look at the additional sense length and compare it to the
>> allocation length; this will work for everything.
>
> Unfortunately, I am not sure that the allocation length that was sent
> to the device is always available.  I will look into this more closely
> but it appeared to me that e.g. FC drivers like qla2xxx get the sense
> data automatically from the HBA firmware.  In the case of that driver
> the host sense buffer size looks like it is hard-coded to 32 bytes,
> for all I know the firmware might only asking for 18 bytes.
>
Hmm.

Maybe we should be adding a 'max_sense_len' field to the SCSI host;
that way we could check against this.
Nevertheless, that information would be lost to us in either case.

I wonder how these vendors propose to handle long sense code data;
silently ignoring it doesn't seem to be the correct way.

> Of course, for a normal REQUEST SENSE command where the allocation
> length is in the CDB, it would indeed be easy to add a check against
> the additional sense length.
>
>>
>> I'm not even convinced that overflow is important: for a lot of the
>> sense probes, we deliberately induce overflows by giving the request
>> sense command a short buffer.  Printing a warning in scsi_check_sense
>> will get very noisy very fast.
>
> That would indeed be a problem.  I didn't see this behavior when testing
> the changes but I'll need to investigate this further.
>
> The purpose of detecting the sense data overflow was to provide some
> visibility that a device is returning a large amount of sense data that
> is currently being silently ignored.  In the case of descriptor format
> sense data, it is possible that a descriptor we want to examine is
> located after one or more other descriptors, and we might not get it
> at all if the buffer isn't large enough.
>
Agreed. but the newest standard won't be adopted to existing 
devices, so we should figure out a way of dealing with those, too.

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

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[James Bottomley]

On Tue, 2013-01-22 at 10:08 -0500, Ewan Milne wrote:
> On Fri, 2013-01-18 at 16:46 +0000, James Bottomley wrote:
> > On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> > > --- a/drivers/scsi/scsi_error.c
> > > +++ b/drivers/scsi/scsi_error.c
> > > @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> > >  	if (! scsi_command_normalize_sense(scmd, &sshdr))
> > >  		return FAILED;	/* no valid sense data */
> > >  
> > > +	if (sshdr.overflow)
> > > +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> > > +
> > >  	if (scsi_sense_is_deferred(&sshdr))
> > >  		return NEEDS_RETRY;
> > >  
> > > @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> > >  			sshdr->asc = sense_buffer[2];
> > >  		if (sb_len > 3)
> > >  			sshdr->ascq = sense_buffer[3];
> > > +		if (sb_len > 4)
> > > +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
> > >  		if (sb_len > 7)
> > >  			sshdr->additional_length = sense_buffer[7];
> > >  	} else {
> > >  		/*
> > >  		 * fixed format
> > >  		 */
> > > -		if (sb_len > 2)
> > > +		if (sb_len > 2) {
> > > +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
> > >  			sshdr->sense_key = (sense_buffer[2] & 0xf);
> > > +		}
> > >  		if (sb_len > 7) {
> > >  			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> > >  					 sb_len : (sense_buffer[7] + 8);
> > 
> > This isn't the right way to do it:  The overflow bit is a recent
> > introduction in SPC-4.  The correct way to tell if we have an overflow
> > or not is to look at the additional sense length and compare it to the
> > allocation length; this will work for everything.
> 
> Unfortunately, I am not sure that the allocation length that was sent
> to the device is always available.

Well, yes it is, we just don't store it.  scsi_normalize_sense() takes
as input the length of the sense buffer we gave it.  If we want an
overflow indication, we can certainly compare that against the
additional length (assuming we have enough bytes to get the additional
length).

>   I will look into this more closely
> but it appeared to me that e.g. FC drivers like qla2xxx get the sense
> data automatically from the HBA firmware.  In the case of that driver
> the host sense buffer size looks like it is hard-coded to 32 bytes,
> for all I know the firmware might only asking for 18 bytes.

You mean for their ACA emulation in the transport?  They have to be
picking up at least the standard minimum in order not to be in
violation, surely?

> Of course, for a normal REQUEST SENSE command where the allocation
> length is in the CDB, it would indeed be easy to add a check against
> the additional sense length.
> 
> > 
> > I'm not even convinced that overflow is important: for a lot of the
> > sense probes, we deliberately induce overflows by giving the request
> > sense command a short buffer.  Printing a warning in scsi_check_sense
> > will get very noisy very fast.
> 
> That would indeed be a problem.  I didn't see this behavior when testing
> the changes but I'll need to investigate this further.
> 
> The purpose of detecting the sense data overflow was to provide some
> visibility that a device is returning a large amount of sense data that
> is currently being silently ignored.  In the case of descriptor format
> sense data, it is possible that a descriptor we want to examine is
> located after one or more other descriptors, and we might not get it
> at all if the buffer isn't large enough.

But why should we care?  A lot of the time it will be spewing
descriptors with irrelevant FRU information.  I really think printing
there's been an overflow isn't a good idea.  I'm not sure there's much
use in the sshdr indicating there's been one.  It *may* be useful to
indicate how big the allocation length should have been, but I'm not
even convinced of that, since the data is now lost.

James

Re: [PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Ewan Milne]

On Tue, 2013-01-22 at 10:38 -0700, Bart Van Assche wrote:
> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> > @@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
> >   *     Dispatch queued events to their associated scsi_device kobjects
> >   *     as uevents.
> >   */
> > -void scsi_evt_thread(struct work_struct *work)
> > +void sdev_evt_thread(struct work_struct *work)
> >  {
> >         struct scsi_device *sdev;
> >         LIST_HEAD(event_list);
> > @@ -2214,7 +2214,7 @@ void scsi_evt_thread(struct work_struct *work)
> >         sdev = container_of(work, struct scsi_device, event_work);
> >
> >         while (1) {
> > -               struct scsi_event *evt;
> > +               struct sdev_event *evt;
> >                 struct list_head *this, *tmp;
> >                 unsigned long flags;
> >
> > @@ -2226,9 +2226,9 @@ void scsi_evt_thread(struct work_struct *work)
> >                         break;
> >
> >                 list_for_each_safe(this, tmp, &event_list) {
> > -                       evt = list_entry(this, struct scsi_event, node);
> > +                       evt = list_entry(this, struct sdev_event, node);
> >                         list_del(&evt->node);
> > -                       scsi_evt_emit(sdev, evt);
> > +                       sdev_evt_emit(sdev, evt);
> >                         kfree(evt);
> >                 }
> >         }
> 
> If schedule_work() gets invoked if work is already on a workqueue then
> it will return immediately. Does that mean that the above approach for
> processing the event list is racy and that new events will not get
> processed if schedule_work() is invoked after the while loop finished
> but before the above function returns ?

I thought that the way that the workqueue mechanism did this was that
the work struct was removed from the list and WORK_STRUCT_PENDING_BIT
was cleared in the work_struct before the work function was invoked.

In that case I the code should be OK since the work function will have
to take the lock which is held around the code to add the event to the
list and the call to schedule_work().

But I see your point, and I since I just added more SDEV_EVT_xxx codes
(and an STARGET_EVT_xxx code, which uses the same mechanism), I didn't
inspect this code carefully.  I'll give it some more thought.

It seems to me that the workqueue mechanism would be hard to use if
there weren't some guarantee against the race condition you mention.

> Bart.

Thank you for your comments.

Re: [PATCH RFC 4/9] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event

[Ewan Milne]

On Tue, 2013-01-22 at 10:33 -0700, Bart Van Assche wrote:
> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> >
> > @@ -2206,7 +2206,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
> >   *     Dispatch queued events to their associated scsi_device kobjects
> >   *     as uevents.
> >   */
> > -void scsi_evt_thread(struct work_struct *work)
> > +void sdev_evt_thread(struct work_struct *work)
> >  {
> >         struct scsi_device *sdev;
> >         LIST_HEAD(event_list);
> 
> This code does not run as a long-living thread so please consider
> using the name "sdev_evt_work" instead.

Good idea.  I'll do that.

> 
> Bart.

Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Ewan Milne]

On Wed, 2013-01-23 at 13:06 +0000, James Bottomley wrote:
> On Tue, 2013-01-22 at 10:08 -0500, Ewan Milne wrote:
> > On Fri, 2013-01-18 at 16:46 +0000, James Bottomley wrote:
> > > On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
> > > > --- a/drivers/scsi/scsi_error.c
> > > > +++ b/drivers/scsi/scsi_error.c
> > > > @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> > > >  	if (! scsi_command_normalize_sense(scmd, &sshdr))
> > > >  		return FAILED;	/* no valid sense data */
> > > >  
> > > > +	if (sshdr.overflow)
> > > > +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
> > > > +
> > > >  	if (scsi_sense_is_deferred(&sshdr))
> > > >  		return NEEDS_RETRY;
> > > >  
> > > > @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> > > >  			sshdr->asc = sense_buffer[2];
> > > >  		if (sb_len > 3)
> > > >  			sshdr->ascq = sense_buffer[3];
> > > > +		if (sb_len > 4)
> > > > +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
> > > >  		if (sb_len > 7)
> > > >  			sshdr->additional_length = sense_buffer[7];
> > > >  	} else {
> > > >  		/*
> > > >  		 * fixed format
> > > >  		 */
> > > > -		if (sb_len > 2)
> > > > +		if (sb_len > 2) {
> > > > +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
> > > >  			sshdr->sense_key = (sense_buffer[2] & 0xf);
> > > > +		}
> > > >  		if (sb_len > 7) {
> > > >  			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> > > >  					 sb_len : (sense_buffer[7] + 8);
> > > 
> > > This isn't the right way to do it:  The overflow bit is a recent
> > > introduction in SPC-4.  The correct way to tell if we have an overflow
> > > or not is to look at the additional sense length and compare it to the
> > > allocation length; this will work for everything.
> > 
> > Unfortunately, I am not sure that the allocation length that was sent
> > to the device is always available.
> 
> Well, yes it is, we just don't store it.  scsi_normalize_sense() takes
> as input the length of the sense buffer we gave it.  If we want an
> overflow indication, we can certainly compare that against the
> additional length (assuming we have enough bytes to get the additional
> length).
> 
> >   I will look into this more closely
> > but it appeared to me that e.g. FC drivers like qla2xxx get the sense
> > data automatically from the HBA firmware.  In the case of that driver
> > the host sense buffer size looks like it is hard-coded to 32 bytes,
> > for all I know the firmware might only asking for 18 bytes.
> 
> You mean for their ACA emulation in the transport?  They have to be
> picking up at least the standard minimum in order not to be in
> violation, surely?

I'm sure that's the case, I just don't know if the minimum is big
enough.  See below.

> 
> > Of course, for a normal REQUEST SENSE command where the allocation
> > length is in the CDB, it would indeed be easy to add a check against
> > the additional sense length.
> > 
> > > 
> > > I'm not even convinced that overflow is important: for a lot of the
> > > sense probes, we deliberately induce overflows by giving the request
> > > sense command a short buffer.  Printing a warning in scsi_check_sense
> > > will get very noisy very fast.
> > 
> > That would indeed be a problem.  I didn't see this behavior when testing
> > the changes but I'll need to investigate this further.
> > 
> > The purpose of detecting the sense data overflow was to provide some
> > visibility that a device is returning a large amount of sense data that
> > is currently being silently ignored.  In the case of descriptor format
> > sense data, it is possible that a descriptor we want to examine is
> > located after one or more other descriptors, and we might not get it
> > at all if the buffer isn't large enough.
> 
> But why should we care?  A lot of the time it will be spewing
> descriptors with irrelevant FRU information.  I really think printing
> there's been an overflow isn't a good idea.  I'm not sure there's much
> use in the sshdr indicating there's been one.  It *may* be useful to
> indicate how big the allocation length should have been, but I'm not
> even convinced of that, since the data is now lost.

My thinking was that it was important to log the fact that the device
had reported a Unit Attention queue overflow, which is reported in a
sense key specific descriptor, so I was concerned that if the sense
buffer wasn't big enough, the host would never see that there had
been a UA queue overflow.  That was why I had added the logging of
a sense buffer overflow.  I wasn't so much interested in reporting
sense buffer overflow for its own sake.

So, as it stands right now, I think I'll remove the 1/9 component
of the patch set, unless there is consensus that it is really needed.

> 
> James
> 
> 

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Bart Van Assche]

On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
> to provide enhanced support for Unit Attention conditions, as well as
> detection of reported sense data overflow conditions and some changes
> to sense data processing.  It also adds a uevent when the reported
> capacity changes on an sd device.
>
> There was some discussion about this a couple of years ago on the linux-scsi
> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
> Although one approach is to send all SCSI sense data to a userspace daemon
> for processing, this patch set does not take that approach due to the
> difficulty in reliably delivering all of the data.  An interesting UA
> condition might not be delivered due to a flood of media errors, for example.
>
> The mechanism used is to flag when certain UA ASC/ASCQ codes are received
> that report asynchronous changes to the storage device configuration.
> An appropriate uevent is then generated for the scsi_device or scsi_target
> object.  An aggregation mechanism is used to avoid generating uevents at
> too high a rate, and to coalesce multiple UAs reported by LUNs on the
> same target for a REPORTED LUNS DATA HAS CHANGED sense code.

Does this patch series add a function that allows SCSI LLDs to report
AEN data to the SCSI core ? What if a SCSI target reports a LUN
inventory change via AER to e.g. the iSCSI initiator and that
initiator ignores the AEN data ? Will that result in AEN data being
ignored and no automatic LUN rescanning ?

Bart.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Hannes Reinecke]

On 01/24/2013 01:19 AM, Bart Van Assche wrote:
> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
>> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
>> to provide enhanced support for Unit Attention conditions, as well as
>> detection of reported sense data overflow conditions and some changes
>> to sense data processing.  It also adds a uevent when the reported
>> capacity changes on an sd device.
>>
>> There was some discussion about this a couple of years ago on the linux-scsi
>> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
>> Although one approach is to send all SCSI sense data to a userspace daemon
>> for processing, this patch set does not take that approach due to the
>> difficulty in reliably delivering all of the data.  An interesting UA
>> condition might not be delivered due to a flood of media errors, for example.
>>
>> The mechanism used is to flag when certain UA ASC/ASCQ codes are received
>> that report asynchronous changes to the storage device configuration.
>> An appropriate uevent is then generated for the scsi_device or scsi_target
>> object.  An aggregation mechanism is used to avoid generating uevents at
>> too high a rate, and to coalesce multiple UAs reported by LUNs on the
>> same target for a REPORTED LUNS DATA HAS CHANGED sense code.
>
> Does this patch series add a function that allows SCSI LLDs to report
> AEN data to the SCSI core ? What if a SCSI target reports a LUN
> inventory change via AER to e.g. the iSCSI initiator and that
> initiator ignores the AEN data ? Will that result in AEN data being
> ignored and no automatic LUN rescanning ?
>
Well, first and foremost we _don't_ have automatic LUN rescanning.
This patchset just puts in the infrastructure that userspace can 
know _when_ a LUN rescan might be in order.

As for AEN, does iSCSI _do_ AEN? I thought it got removed ...

If it does, though, it should schedule an event on its own whenever 
an AER is received. The same goes for LLDDs with vendor-specific 
AENs; thinking of megaraid_sas here ...

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

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Wed, 2013-01-23 at 17:19 -0700, Bart Van Assche wrote:
> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> > This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
> > to provide enhanced support for Unit Attention conditions, as well as
> > detection of reported sense data overflow conditions and some changes
> > to sense data processing.  It also adds a uevent when the reported
> > capacity changes on an sd device.
> >
> > There was some discussion about this a couple of years ago on the linux-scsi
> > mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
> > Although one approach is to send all SCSI sense data to a userspace daemon
> > for processing, this patch set does not take that approach due to the
> > difficulty in reliably delivering all of the data.  An interesting UA
> > condition might not be delivered due to a flood of media errors, for example.
> >
> > The mechanism used is to flag when certain UA ASC/ASCQ codes are received
> > that report asynchronous changes to the storage device configuration.
> > An appropriate uevent is then generated for the scsi_device or scsi_target
> > object.  An aggregation mechanism is used to avoid generating uevents at
> > too high a rate, and to coalesce multiple UAs reported by LUNs on the
> > same target for a REPORTED LUNS DATA HAS CHANGED sense code.
> 
> Does this patch series add a function that allows SCSI LLDs to report
> AEN data to the SCSI core ? What if a SCSI target reports a LUN
> inventory change via AER to e.g. the iSCSI initiator and that
> initiator ignores the AEN data ? Will that result in AEN data being
> ignored and no automatic LUN rescanning ?

Well, what the patch series does is add handling in scsi_check_sense(),
so any sense processing that went through that path would be handled.
I don't think this would apply to AEN, however.  The only AEN handling
that I am aware of was in iscsi-initiator-utils, in userspace, which
did a rescan on the iscsi session when a 3F 0E code was received.

> 
> Bart.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Thu, 2013-01-24 at 12:38 +0100, Hannes Reinecke wrote:
> On 01/24/2013 01:19 AM, Bart Van Assche wrote:
> > On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> >> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
> >> to provide enhanced support for Unit Attention conditions, as well as
> >> detection of reported sense data overflow conditions and some changes
> >> to sense data processing.  It also adds a uevent when the reported
> >> capacity changes on an sd device.
> >>
> >> There was some discussion about this a couple of years ago on the linux-scsi
> >> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
> >> Although one approach is to send all SCSI sense data to a userspace daemon
> >> for processing, this patch set does not take that approach due to the
> >> difficulty in reliably delivering all of the data.  An interesting UA
> >> condition might not be delivered due to a flood of media errors, for example.
> >>
> >> The mechanism used is to flag when certain UA ASC/ASCQ codes are received
> >> that report asynchronous changes to the storage device configuration.
> >> An appropriate uevent is then generated for the scsi_device or scsi_target
> >> object.  An aggregation mechanism is used to avoid generating uevents at
> >> too high a rate, and to coalesce multiple UAs reported by LUNs on the
> >> same target for a REPORTED LUNS DATA HAS CHANGED sense code.
> >
> > Does this patch series add a function that allows SCSI LLDs to report
> > AEN data to the SCSI core ? What if a SCSI target reports a LUN
> > inventory change via AER to e.g. the iSCSI initiator and that
> > initiator ignores the AEN data ? Will that result in AEN data being
> > ignored and no automatic LUN rescanning ?
> >
> Well, first and foremost we _don't_ have automatic LUN rescanning.
> This patchset just puts in the infrastructure that userspace can 
> know _when_ a LUN rescan might be in order.
> 
> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...

I think you are right, I think AEN was removed in SAM-3 (sam3r14, if my
sources are correct).

Bart might have a point, though.  I wonder if an iscsi device conforming
to SAM-2 would report a LUN inventory change via AEN, but not as a UA
on a normal SCSI command, in which case the iscsi initiator would have
to handle it.  I'll have to do some research and probably ask some
vendors about that, it could maybe be addressed with a follow-on patch.

> 
> If it does, though, it should schedule an event on its own whenever 
> an AER is received. The same goes for LLDDs with vendor-specific 
> AENs; thinking of megaraid_sas here ...
> 
> Cheers,
> 
> Hannes

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Mike Christie]

On 01/24/2013 04:38 AM, Hannes Reinecke wrote:
> On 01/24/2013 01:19 AM, Bart Van Assche wrote:
>> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
>>> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
>>> to provide enhanced support for Unit Attention conditions, as well as
>>> detection of reported sense data overflow conditions and some changes
>>> to sense data processing.  It also adds a uevent when the reported
>>> capacity changes on an sd device.
>>>
>>> There was some discussion about this a couple of years ago on the
>>> linux-scsi
>>> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
>>> Although one approach is to send all SCSI sense data to a userspace
>>> daemon
>>> for processing, this patch set does not take that approach due to the
>>> difficulty in reliably delivering all of the data.  An interesting UA
>>> condition might not be delivered due to a flood of media errors, for
>>> example.
>>>
>>> The mechanism used is to flag when certain UA ASC/ASCQ codes are
>>> received
>>> that report asynchronous changes to the storage device configuration.
>>> An appropriate uevent is then generated for the scsi_device or
>>> scsi_target
>>> object.  An aggregation mechanism is used to avoid generating uevents at
>>> too high a rate, and to coalesce multiple UAs reported by LUNs on the
>>> same target for a REPORTED LUNS DATA HAS CHANGED sense code.
>>
>> Does this patch series add a function that allows SCSI LLDs to report
>> AEN data to the SCSI core ? What if a SCSI target reports a LUN
>> inventory change via AER to e.g. the iSCSI initiator and that
>> initiator ignores the AEN data ? Will that result in AEN data being
>> ignored and no automatic LUN rescanning ?
>>
> Well, first and foremost we _don't_ have automatic LUN rescanning.
> This patchset just puts in the infrastructure that userspace can know
> _when_ a LUN rescan might be in order.
> 
> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...

The AEN is sent to userspace right now.

> 
> If it does, though, it should schedule an event on its own whenever an
> AER is received. The same goes for LLDDs with vendor-specific AENs;
> thinking of megaraid_sas here ...
> 

Yeah. It would be nicer if there was a wrapper around this code:

+#ifdef CONFIG_SCSI_ENHANCED_UA
+			struct scsi_target *starget = scsi_target(sdev);
+			if (atomic_xchg(&starget->lun_change_reported, 1) == 0)
+				schedule_delayed_work(&starget->ua_dwork, 2*HZ);
+			scmd_printk(KERN_WARNING, scmd,
+				    "Reported LUNs data has changed");
+#else

so drivers do not have to have to duplicate and know that low level of
details.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Bart Van Assche]

On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>
> If it does, though, it should schedule an event on its own whenever an AER
> is received. The same goes for LLDDs with vendor-specific AENs; thinking of
> megaraid_sas here ...

Let me ask this another way. SAN users expect that the LUN list at the
initiator side gets updated automatically after a SAN configuration
change. How should a SAN system communicate to a SCSI initiator that
the LUN list has been changed ? Some FC SAN systems send a LIP after a
configuration change to force the initiator to rescan LUNs. But how to
inform the initiator about a LUN change for other SCSI protocols ? I'm
not sure that it is even possible to report such a change via sense
data in case a SAN user first removes all LUNs and after that change
adds one or more LUNs.

Bart.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Hannes Reinecke]

On 01/24/2013 03:38 PM, Bart Van Assche wrote:
> On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>>
>> If it does, though, it should schedule an event on its own whenever an AER
>> is received. The same goes for LLDDs with vendor-specific AENs; thinking of
>> megaraid_sas here ...
>
> Let me ask this another way. SAN users expect that the LUN list at the
> initiator side gets updated automatically after a SAN configuration
> change. How should a SAN system communicate to a SCSI initiator that
> the LUN list has been changed ? Some FC SAN systems send a LIP after a
> configuration change to force the initiator to rescan LUNs.

And thereby disrupting traffic on _ALL_ LUNs on the loop.
Really cool idea.
I know; the one vendor which does _not_ talk to us.

> But how to inform the initiator about a LUN change for other SCSI protocols ?
 > I'm not sure that it is even possible to report such a change via 
sense
> data in case a SAN user first removes all LUNs and after that change
> adds one or more LUNs.
>
The official way is indeed via UAs; most storage arrays (Hello, 
NetApp!) provide a default LUN0 which is always visible.
Up to the point that some even refuse to add 'normal' disk LUNs to 
LUN0. Or have the ominous 'Well-known Address' LUN to handle these 
kind of issues.

Obviously, one needs to send commands to it to even _get_ an UA back.

But yeah, normally it's possible.

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

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Mike Christie]

On 01/24/2013 07:51 AM, Hannes Reinecke wrote:
> On 01/24/2013 03:38 PM, Bart Van Assche wrote:
>> On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>>>
>>> If it does, though, it should schedule an event on its own whenever
>>> an AER
>>> is received. The same goes for LLDDs with vendor-specific AENs;
>>> thinking of
>>> megaraid_sas here ...
>>
>> Let me ask this another way. SAN users expect that the LUN list at the
>> initiator side gets updated automatically after a SAN configuration
>> change. How should a SAN system communicate to a SCSI initiator that
>> the LUN list has been changed ? Some FC SAN systems send a LIP after a
>> configuration change to force the initiator to rescan LUNs.
> 
> And thereby disrupting traffic on _ALL_ LUNs on the loop.
> Really cool idea.
> I know; the one vendor which does _not_ talk to us.
> 
>> But how to inform the initiator about a LUN change for other SCSI
>> protocols ?
>> I'm not sure that it is even possible to report such a change via sense
>> data in case a SAN user first removes all LUNs and after that change
>> adds one or more LUNs.
>>
> The official way is indeed via UAs; most storage arrays (Hello, NetApp!)
> provide a default LUN0 which is always visible.
> Up to the point that some even refuse to add 'normal' disk LUNs to LUN0.
> Or have the ominous 'Well-known Address' LUN to handle these kind of
> issues.
> 
> Obviously, one needs to send commands to it to even _get_ an UA back.
> 

In SAM5 there is that QUERY ASYNCHRONOUS EVENT TMF. Could we send that
periodically to lun0/well-knwon-lun if the transport supports it (iscsi
will in
http://tools.ietf.org/html/draft-ietf-storm-iscsi-sam-06#section-6).
Whatever daemon in userspace handles these other events, could send it
(we just need to add a interface) or we could add kernel code.

This should not hold up Ewan's patches though.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Hannes Reinecke]

On 01/24/2013 04:00 PM, Mike Christie wrote:
> On 01/24/2013 07:51 AM, Hannes Reinecke wrote:
>> On 01/24/2013 03:38 PM, Bart Van Assche wrote:
>>> On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>>>>
>>>> If it does, though, it should schedule an event on its own whenever
>>>> an AER
>>>> is received. The same goes for LLDDs with vendor-specific AENs;
>>>> thinking of
>>>> megaraid_sas here ...
>>>
>>> Let me ask this another way. SAN users expect that the LUN list at the
>>> initiator side gets updated automatically after a SAN configuration
>>> change. How should a SAN system communicate to a SCSI initiator that
>>> the LUN list has been changed ? Some FC SAN systems send a LIP after a
>>> configuration change to force the initiator to rescan LUNs.
>>
>> And thereby disrupting traffic on _ALL_ LUNs on the loop.
>> Really cool idea.
>> I know; the one vendor which does _not_ talk to us.
>>
>>> But how to inform the initiator about a LUN change for other SCSI
>>> protocols ?
>>> I'm not sure that it is even possible to report such a change via sense
>>> data in case a SAN user first removes all LUNs and after that change
>>> adds one or more LUNs.
>>>
>> The official way is indeed via UAs; most storage arrays (Hello, NetApp!)
>> provide a default LUN0 which is always visible.
>> Up to the point that some even refuse to add 'normal' disk LUNs to LUN0.
>> Or have the ominous 'Well-known Address' LUN to handle these kind of
>> issues.
>>
>> Obviously, one needs to send commands to it to even _get_ an UA back.
>>
>
> In SAM5 there is that QUERY ASYNCHRONOUS EVENT TMF. Could we send that
> periodically to lun0/well-knwon-lun if the transport supports it (iscsi
> will in
> http://tools.ietf.org/html/draft-ietf-storm-iscsi-sam-06#section-6).
> Whatever daemon in userspace handles these other events, could send it
> (we just need to add a interface) or we could add kernel code.
>
Oh, cool.
Polling a device to figure out if we should poll it :-)

We'd be better off sending TEST UNIT READY to it; then we should
be getting UAs regardless on the SAM version in use on the target.

(Especially as some target lie about the supported version, so
they might be supporting SAM-5 without telling us ...)

> This should not hold up Ewan's patches though.
>
Correct.

AEN handling discussion is a different story and should be build
on top of Ewans patches.

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

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Thu, 2013-01-24 at 16:15 +0100, Hannes Reinecke wrote:
> On 01/24/2013 04:00 PM, Mike Christie wrote:
> > On 01/24/2013 07:51 AM, Hannes Reinecke wrote:
> >> On 01/24/2013 03:38 PM, Bart Van Assche wrote:
> >>> On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
> >>>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
> >>>>
> >>>> If it does, though, it should schedule an event on its own whenever
> >>>> an AER
> >>>> is received. The same goes for LLDDs with vendor-specific AENs;
> >>>> thinking of
> >>>> megaraid_sas here ...
> >>>
> >>> Let me ask this another way. SAN users expect that the LUN list at the
> >>> initiator side gets updated automatically after a SAN configuration
> >>> change. How should a SAN system communicate to a SCSI initiator that
> >>> the LUN list has been changed ? Some FC SAN systems send a LIP after a
> >>> configuration change to force the initiator to rescan LUNs.
> >>
> >> And thereby disrupting traffic on _ALL_ LUNs on the loop.
> >> Really cool idea.
> >> I know; the one vendor which does _not_ talk to us.
> >>
> >>> But how to inform the initiator about a LUN change for other SCSI
> >>> protocols ?
> >>> I'm not sure that it is even possible to report such a change via sense
> >>> data in case a SAN user first removes all LUNs and after that change
> >>> adds one or more LUNs.
> >>>
> >> The official way is indeed via UAs; most storage arrays (Hello, NetApp!)
> >> provide a default LUN0 which is always visible.
> >> Up to the point that some even refuse to add 'normal' disk LUNs to LUN0.
> >> Or have the ominous 'Well-known Address' LUN to handle these kind of
> >> issues.
> >>
> >> Obviously, one needs to send commands to it to even _get_ an UA back.
> >>
> >
> > In SAM5 there is that QUERY ASYNCHRONOUS EVENT TMF. Could we send that
> > periodically to lun0/well-knwon-lun if the transport supports it (iscsi
> > will in
> > http://tools.ietf.org/html/draft-ietf-storm-iscsi-sam-06#section-6).
> > Whatever daemon in userspace handles these other events, could send it
> > (we just need to add a interface) or we could add kernel code.
> >
> Oh, cool.
> Polling a device to figure out if we should poll it :-)
> 
> We'd be better off sending TEST UNIT READY to it; then we should
> be getting UAs regardless on the SAM version in use on the target.

I believe that multipath is already sending periodic commands to devices
through the different paths, to determine up-to-date path status.
So, in at least some cases, we wouldn't have to do anything else.

> 
> (Especially as some target lie about the supported version, so
> they might be supporting SAM-5 without telling us ...)
> 
> > This should not hold up Ewan's patches though.
> >
> Correct.
> 
> AEN handling discussion is a different story and should be build
> on top of Ewans patches.
> 
> Cheers,
> 
> Hannes

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Thu, 2013-01-24 at 07:01 -0700, Mike Christie wrote:
> On 01/24/2013 04:38 AM, Hannes Reinecke wrote:
> > On 01/24/2013 01:19 AM, Bart Van Assche wrote:
> >> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
> >>> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
> >>> to provide enhanced support for Unit Attention conditions, as well as
> >>> detection of reported sense data overflow conditions and some changes
> >>> to sense data processing.  It also adds a uevent when the reported
> >>> capacity changes on an sd device.
> >>>
> >>> There was some discussion about this a couple of years ago on the
> >>> linux-scsi
> >>> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
> >>> Although one approach is to send all SCSI sense data to a userspace
> >>> daemon
> >>> for processing, this patch set does not take that approach due to the
> >>> difficulty in reliably delivering all of the data.  An interesting UA
> >>> condition might not be delivered due to a flood of media errors, for
> >>> example.
> >>>
> >>> The mechanism used is to flag when certain UA ASC/ASCQ codes are
> >>> received
> >>> that report asynchronous changes to the storage device configuration.
> >>> An appropriate uevent is then generated for the scsi_device or
> >>> scsi_target
> >>> object.  An aggregation mechanism is used to avoid generating uevents at
> >>> too high a rate, and to coalesce multiple UAs reported by LUNs on the
> >>> same target for a REPORTED LUNS DATA HAS CHANGED sense code.
> >>
> >> Does this patch series add a function that allows SCSI LLDs to report
> >> AEN data to the SCSI core ? What if a SCSI target reports a LUN
> >> inventory change via AER to e.g. the iSCSI initiator and that
> >> initiator ignores the AEN data ? Will that result in AEN data being
> >> ignored and no automatic LUN rescanning ?
> >>
> > Well, first and foremost we _don't_ have automatic LUN rescanning.
> > This patchset just puts in the infrastructure that userspace can know
> > _when_ a LUN rescan might be in order.
> > 
> > As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
> 
> The AEN is sent to userspace right now.
> 
> > 
> > If it does, though, it should schedule an event on its own whenever an
> > AER is received. The same goes for LLDDs with vendor-specific AENs;
> > thinking of megaraid_sas here ...
> > 
> 
> Yeah. It would be nicer if there was a wrapper around this code:
> 
> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +			struct scsi_target *starget = scsi_target(sdev);
> +			if (atomic_xchg(&starget->lun_change_reported, 1) == 0)
> +				schedule_delayed_work(&starget->ua_dwork, 2*HZ);
> +			scmd_printk(KERN_WARNING, scmd,
> +				    "Reported LUNs data has changed");
> +#else
> 
> so drivers do not have to have to duplicate and know that low level of
> details.

I could move that to an exported function.  I guess the questions would
be (a) is this the only ASC/ASCQ combination that should be available
in that way, and (b) should it be conditional on the config option?

Thanks for your comments.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Mike Christie]

On 01/24/2013 03:02 PM, Ewan Milne wrote:
> On Thu, 2013-01-24 at 07:01 -0700, Mike Christie wrote:
>> On 01/24/2013 04:38 AM, Hannes Reinecke wrote:
>>> On 01/24/2013 01:19 AM, Bart Van Assche wrote:
>>>> On Fri, Jan 18, 2013 at 9:27 AM, Ewan D. Milne <emilne@redhat.com> wrote:
>>>>> This patch set adds changes to the SCSI mid-layer, sysfs and scsi_debug
>>>>> to provide enhanced support for Unit Attention conditions, as well as
>>>>> detection of reported sense data overflow conditions and some changes
>>>>> to sense data processing.  It also adds a uevent when the reported
>>>>> capacity changes on an sd device.
>>>>>
>>>>> There was some discussion about this a couple of years ago on the
>>>>> linux-scsi
>>>>> mailing list:  http://marc.info/?l=linux-scsi&m=129702506514742&w=2
>>>>> Although one approach is to send all SCSI sense data to a userspace
>>>>> daemon
>>>>> for processing, this patch set does not take that approach due to the
>>>>> difficulty in reliably delivering all of the data.  An interesting UA
>>>>> condition might not be delivered due to a flood of media errors, for
>>>>> example.
>>>>>
>>>>> The mechanism used is to flag when certain UA ASC/ASCQ codes are
>>>>> received
>>>>> that report asynchronous changes to the storage device configuration.
>>>>> An appropriate uevent is then generated for the scsi_device or
>>>>> scsi_target
>>>>> object.  An aggregation mechanism is used to avoid generating uevents at
>>>>> too high a rate, and to coalesce multiple UAs reported by LUNs on the
>>>>> same target for a REPORTED LUNS DATA HAS CHANGED sense code.
>>>>
>>>> Does this patch series add a function that allows SCSI LLDs to report
>>>> AEN data to the SCSI core ? What if a SCSI target reports a LUN
>>>> inventory change via AER to e.g. the iSCSI initiator and that
>>>> initiator ignores the AEN data ? Will that result in AEN data being
>>>> ignored and no automatic LUN rescanning ?
>>>>
>>> Well, first and foremost we _don't_ have automatic LUN rescanning.
>>> This patchset just puts in the infrastructure that userspace can know
>>> _when_ a LUN rescan might be in order.
>>>
>>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>>
>> The AEN is sent to userspace right now.
>>
>>>
>>> If it does, though, it should schedule an event on its own whenever an
>>> AER is received. The same goes for LLDDs with vendor-specific AENs;
>>> thinking of megaraid_sas here ...
>>>
>>
>> Yeah. It would be nicer if there was a wrapper around this code:
>>
>> +#ifdef CONFIG_SCSI_ENHANCED_UA
>> +			struct scsi_target *starget = scsi_target(sdev);
>> +			if (atomic_xchg(&starget->lun_change_reported, 1) == 0)
>> +				schedule_delayed_work(&starget->ua_dwork, 2*HZ);
>> +			scmd_printk(KERN_WARNING, scmd,
>> +				    "Reported LUNs data has changed");
>> +#else
>>
>> so drivers do not have to have to duplicate and know that low level of
>> details.
> 
> I could move that to an exported function.  I guess the questions would
> be (a) is this the only ASC/ASCQ combination that should be available
> in that way, and (b) should it be conditional on the config option?
> 

A. There are others. Basically anything that is handled in patch 6.

B. I think you want to make a scsi_check_sense function that
scsi_error.c calls and drivers can call. It would take a sshdr. That
core function would then do all this. So it would basically be like:


static int scsi_check_cmd_sense(struct scsi_cmnd *scmd)
{
	struct scsi_sense_hdr sshdr;

        if (!scsi_command_normalize_sense(scmd, &sshdr))
		return FAILED;  /* no valid sense data */

        if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done !=
scsi_eh_done)
                /*
                 * nasty: for mid-layer issued TURs, we need to return the
                 * actual sense data without any recovery attempt.  For eh
                 * issued ones, we need to try to recover and interpret
                 */
                return SUCCESS;


	return scsi_check_sense(&sshdr);
}


// other drivers that get sense not in a scsi command can then call this.
int scsi_check_sense(struct scsi_device *sdev, struct scsi_sense_hdr
sshdr, sense_buffer)
{
	This is basically the scsi_check_sense we have today but below the cmd
related checks above.

}

Also, scsi_dh_alua is checking for report luns data has changed and
inquiry data has changed. That code should be removed or if alua is used
then we will not hit your new schedule work code paths.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Mike Christie]

On 01/24/2013 09:15 AM, Hannes Reinecke wrote:
> On 01/24/2013 04:00 PM, Mike Christie wrote:
>> On 01/24/2013 07:51 AM, Hannes Reinecke wrote:
>>> On 01/24/2013 03:38 PM, Bart Van Assche wrote:
>>>> On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>>>>>
>>>>> If it does, though, it should schedule an event on its own whenever
>>>>> an AER
>>>>> is received. The same goes for LLDDs with vendor-specific AENs;
>>>>> thinking of
>>>>> megaraid_sas here ...
>>>>
>>>> Let me ask this another way. SAN users expect that the LUN list at the
>>>> initiator side gets updated automatically after a SAN configuration
>>>> change. How should a SAN system communicate to a SCSI initiator that
>>>> the LUN list has been changed ? Some FC SAN systems send a LIP after a
>>>> configuration change to force the initiator to rescan LUNs.
>>>
>>> And thereby disrupting traffic on _ALL_ LUNs on the loop.
>>> Really cool idea.
>>> I know; the one vendor which does _not_ talk to us.
>>>
>>>> But how to inform the initiator about a LUN change for other SCSI
>>>> protocols ?
>>>> I'm not sure that it is even possible to report such a change via sense
>>>> data in case a SAN user first removes all LUNs and after that change
>>>> adds one or more LUNs.
>>>>
>>> The official way is indeed via UAs; most storage arrays (Hello, NetApp!)
>>> provide a default LUN0 which is always visible.
>>> Up to the point that some even refuse to add 'normal' disk LUNs to LUN0.
>>> Or have the ominous 'Well-known Address' LUN to handle these kind of
>>> issues.
>>>
>>> Obviously, one needs to send commands to it to even _get_ an UA back.
>>>
>>
>> In SAM5 there is that QUERY ASYNCHRONOUS EVENT TMF. Could we send that
>> periodically to lun0/well-knwon-lun if the transport supports it (iscsi
>> will in
>> http://tools.ietf.org/html/draft-ietf-storm-iscsi-sam-06#section-6).
>> Whatever daemon in userspace handles these other events, could send it
>> (we just need to add a interface) or we could add kernel code.
>>
> Oh, cool.
> Polling a device to figure out if we should poll it :-)
> 
> We'd be better off sending TEST UNIT READY to it; then we should
> be getting UAs regardless on the SAM version in use on the target.
> 

To handle the case where all devices are removed then new ones are
added, we will not have kernel structs or /dev/sdXs to send TURs to to
find new ones. Are you thinking we would do something like have the
kernel create temp structs to send TURs to LUN0/well-known-lun? Or, do
you think we would have something doing scsi scans (call
scsi_scan_target or scsi_scan_host) every once in a while?

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Hannes Reinecke]

On 01/26/2013 07:20 PM, Mike Christie wrote:
> On 01/24/2013 09:15 AM, Hannes Reinecke wrote:
>> On 01/24/2013 04:00 PM, Mike Christie wrote:
>>> On 01/24/2013 07:51 AM, Hannes Reinecke wrote:
>>>> On 01/24/2013 03:38 PM, Bart Van Assche wrote:
>>>>> On Thu, Jan 24, 2013 at 4:38 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>>> As for AEN, does iSCSI _do_ AEN? I thought it got removed ...
>>>>>>
>>>>>> If it does, though, it should schedule an event on its own whenever
>>>>>> an AER
>>>>>> is received. The same goes for LLDDs with vendor-specific AENs;
>>>>>> thinking of
>>>>>> megaraid_sas here ...
>>>>>
>>>>> Let me ask this another way. SAN users expect that the LUN list at the
>>>>> initiator side gets updated automatically after a SAN configuration
>>>>> change. How should a SAN system communicate to a SCSI initiator that
>>>>> the LUN list has been changed ? Some FC SAN systems send a LIP after a
>>>>> configuration change to force the initiator to rescan LUNs.
>>>>
>>>> And thereby disrupting traffic on _ALL_ LUNs on the loop.
>>>> Really cool idea.
>>>> I know; the one vendor which does _not_ talk to us.
>>>>
>>>>> But how to inform the initiator about a LUN change for other SCSI
>>>>> protocols ?
>>>>> I'm not sure that it is even possible to report such a change via sense
>>>>> data in case a SAN user first removes all LUNs and after that change
>>>>> adds one or more LUNs.
>>>>>
>>>> The official way is indeed via UAs; most storage arrays (Hello, NetApp!)
>>>> provide a default LUN0 which is always visible.
>>>> Up to the point that some even refuse to add 'normal' disk LUNs to LUN0.
>>>> Or have the ominous 'Well-known Address' LUN to handle these kind of
>>>> issues.
>>>>
>>>> Obviously, one needs to send commands to it to even _get_ an UA back.
>>>>
>>>
>>> In SAM5 there is that QUERY ASYNCHRONOUS EVENT TMF. Could we send that
>>> periodically to lun0/well-knwon-lun if the transport supports it (iscsi
>>> will in
>>> http://tools.ietf.org/html/draft-ietf-storm-iscsi-sam-06#section-6).
>>> Whatever daemon in userspace handles these other events, could send it
>>> (we just need to add a interface) or we could add kernel code.
>>>
>> Oh, cool.
>> Polling a device to figure out if we should poll it :-)
>>
>> We'd be better off sending TEST UNIT READY to it; then we should
>> be getting UAs regardless on the SAM version in use on the target.
>>
>
> To handle the case where all devices are removed then new ones are
> added, we will not have kernel structs or /dev/sdXs to send TURs to to
> find new ones. Are you thinking we would do something like have the
> kernel create temp structs to send TURs to LUN0/well-known-lun? Or, do
> you think we would have something doing scsi scans (call
> scsi_scan_target or scsi_scan_host) every once in a while?
>
Well, not as such.

Most arrays (eg RDAC, EVA, or EMC) will present you with a separate 
device (like the infamous 'Universal Xport' :-) which will be 
present always, so it can be used for polling in case no targets are 
present.
NetApp and the like are more tricky as they do _not_ present any 
targets per default. So we'd need something here to talk to.
Some kind of 'virtual' LUN0 or somesuch.

Wouldn't this be a grand topic for LSF?

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

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Jeremy Linton]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/24/2013 8:38 AM, Bart Van Assche wrote:

> Let me ask this another way. SAN users expect that the LUN list at the 
> initiator side gets updated automatically after a SAN configuration change.
> How should a SAN system communicate to a SCSI initiator that the LUN list
> has been changed ? Some FC SAN systems send a LIP after a configuration
> change to force the initiator to rescan LUNs. But how to

	What I think your looking for is RSCN (Registered State Change notification)
. Hook that, and then check the name server. This will tell you when ports get
added/removed. You can then report luns against lun 0 of all the known target
ports. This allows you to transparently detect changes.

	Otherwise, you run the risk of trapping UA's in the lower level portions of
the stack that _REALLY_ need to be propagated to the controlling driver or
application.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJRBpOeAAoJEL5i86xrzcy765gH/AnG1TPJ5Y3RGx00TiLP5shb
+yS384FoIis4XuWijUdofsAcnZzUFaMgH7lPBr5TkT1yYDgyXtzvpjV/2rvWlvzA
PfHPU4vPFmpF1XO7IX2PJCpHAYheHXhucnMkXVLI9GA5nR9+BPQjjvav24ixGKPc
b2889zju7Z7KUb0R4SXWtSCbRZZtYuBj0Rckh8a/ra9wJXHuMpsg7+7OzrLqbSqH
OcAmcb5Q8T/5D6Rj4rJVF3d1Fzr5+P2qrMhS+eb98I6phZ5UvHs66nY/pHjCGpbA
SShQlGOg7+nIjxsf9jjl2/sgx0jJH40koyW8Xv9WERE75eQ9bVBpBX3BeosvlJs=
=isBF
-----END PGP SIGNATURE-----

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Bart Van Assche]

On 01/28/13 16:05, Jeremy Linton wrote:
> What I think your looking for is RSCN (Registered State Change notification).
> Hook that, and then check the name server. This will tell you when ports get
> added/removed. You can then report luns against lun 0 of all the known target
> ports. This allows you to transparently detect changes.
>
> Otherwise, you run the risk of trapping UA's in the lower level portions of
> the stack that _REALLY_ need to be propagated to the controlling driver or
> application.

Thanks for the feedback. Unfortunately sending an RSCN is only possible 
when using Fibre Channel as transport layer. I'm looking for a solution 
that also works with other SCSI transports, e.g. iSCSI and SRP.

Bart.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Hannes Reinecke]

On 01/28/2013 04:44 PM, Bart Van Assche wrote:
> On 01/28/13 16:05, Jeremy Linton wrote:
>> What I think your looking for is RSCN (Registered State Change
>> notification).
>> Hook that, and then check the name server. This will tell you when
>> ports get
>> added/removed. You can then report luns against lun 0 of all the
>> known target
>> ports. This allows you to transparently detect changes.
>>
>> Otherwise, you run the risk of trapping UA's in the lower level
>> portions of
>> the stack that _REALLY_ need to be propagated to the controlling
>> driver or
>> application.
>
> Thanks for the feedback. Unfortunately sending an RSCN is only
> possible when using Fibre Channel as transport layer. I'm looking
> for a solution that also works with other SCSI transports, e.g.
> iSCSI and SRP.
>
And we're already doing that. Rather, the FC HBAs do exactly this.

So the proposal would be to export a 'virtual' LUN0 if none is 
present, so that we always have a device to talk to, right?

Shouldn't be too hard; we already create a 'virtual' LUN0 during 
scanning, only we delete it if no LUNs are found.
So we're already doing this, only we don't tell :-)

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

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Jeremy Linton]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/28/2013 9:44 AM, Bart Van Assche wrote:
> when using Fibre Channel as transport layer. I'm looking for a solution 
> that also works with other SCSI transports, e.g. iSCSI and SRP.

	Doesn't iSCSI have a SNS server you can subscribe to that provides similar
functionality (name services and port add/remove)? SAS has the BROADCAST
functionality too which can act as an AEN.


	




-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJRBp7TAAoJEL5i86xrzcy7jHAIAJlrqG553O0dqiGUFnb+W+oe
+KB6SfST0DiqATBJee61kJWw0hzC8EYwDVXgGJnw4c4XqmaAt0JNLPSq8F77DMkC
UQzAdz7U8QxrJp9cwdz4HpWiVXolFJ7I6Gg9Og+KBxAMDIWp/mQNa06Y+b0XQgmW
/KH4/AJMC+cn5GCYq55cSn+ZmKGO3JVB4tbj1VRgyxF/+dBlfiw94YoHxI6ODvF/
440Mo1rtI9P5+hdqGlkqxZmjZBzawCWZDHkos75dKIYl332FqrwubBNngL/St8GO
1DGlK6+STDLIE8XyFPhqKLBiObhnKhnZHjtgmGXEw20ZYx7QLd4IPOLLVGdYFik=
=PWeG
-----END PGP SIGNATURE-----

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

On Mon, 2013-01-28 at 09:52 -0600, Jeremy Linton wrote:
> On 1/28/2013 9:44 AM, Bart Van Assche wrote:
> > when using Fibre Channel as transport layer. I'm looking for a solution 
> > that also works with other SCSI transports, e.g. iSCSI and SRP.
> 
> 	Doesn't iSCSI have a SNS server you can subscribe to that provides similar
> functionality (name services and port add/remove)? SAS has the BROADCAST
> functionality too which can act as an AEN.
> 

I don't think you have to have an iSNS server to use iSCSI, though.
It is possible to use Send Targets for discovery.  At least that used
to be the case.

-Ewan

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Mike Christie]

On 01/28/2013 09:52 AM, Jeremy Linton wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/28/2013 9:44 AM, Bart Van Assche wrote:
>> when using Fibre Channel as transport layer. I'm looking for a solution 
>> that also works with other SCSI transports, e.g. iSCSI and SRP.
> 
> 	Doesn't iSCSI have a SNS server you can subscribe to that provides similar
> functionality (name services and port add/remove)? SAS has the BROADCAST
> functionality too which can act as an AEN.
> 

iSCSI does have a SNS server. In iSCSI it is called iSNS. The problem is
that it is not widely used.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[James Bottomley]

On Mon, 2013-01-28 at 16:48 +0100, Hannes Reinecke wrote:
> On 01/28/2013 04:44 PM, Bart Van Assche wrote:
> > On 01/28/13 16:05, Jeremy Linton wrote:
> >> What I think your looking for is RSCN (Registered State Change
> >> notification).
> >> Hook that, and then check the name server. This will tell you when
> >> ports get
> >> added/removed. You can then report luns against lun 0 of all the
> >> known target
> >> ports. This allows you to transparently detect changes.
> >>
> >> Otherwise, you run the risk of trapping UA's in the lower level
> >> portions of
> >> the stack that _REALLY_ need to be propagated to the controlling
> >> driver or
> >> application.
> >
> > Thanks for the feedback. Unfortunately sending an RSCN is only
> > possible when using Fibre Channel as transport layer. I'm looking
> > for a solution that also works with other SCSI transports, e.g.
> > iSCSI and SRP.
> >
> And we're already doing that. Rather, the FC HBAs do exactly this.
> 
> So the proposal would be to export a 'virtual' LUN0 if none is 
> present, so that we always have a device to talk to, right?
> 
> Shouldn't be too hard; we already create a 'virtual' LUN0 during 
> scanning, only we delete it if no LUNs are found.
> So we're already doing this, only we don't tell :-)

I'm not sure I'd like to leave virtual LUN-0 around if we find nothing
because it's going to cause confusion and be nasty in cases where a
physical lun 0 is created later.  However, don't the standards define
this ignored lun: W-LUN that's supposed to be created precisely for this
purpose?  I wouldn't object to creating and exporting one of those if we
have to and the array doesn't supply it.

James

RE: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Shyam_Iyer]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Jeremy Linton
> Sent: Monday, January 28, 2013 10:05 AM
> To: Bart Van Assche
> Cc: Hannes Reinecke; Ewan D. Milne; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention
> handling
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/24/2013 8:38 AM, Bart Van Assche wrote:
> 
> > Let me ask this another way. SAN users expect that the LUN list at the
> > initiator side gets updated automatically after a SAN configuration change.
> > How should a SAN system communicate to a SCSI initiator that the LUN
> > list has been changed ? Some FC SAN systems send a LIP after a
> > configuration change to force the initiator to rescan LUNs. But how to
> 
> 	What I think your looking for is RSCN (Registered State Change
> notification) . Hook that, and then check the name server. This will tell you
> when ports get added/removed. You can then report luns against lun 0 of all
> the known target ports. This allows you to transparently detect changes.

iSNS based SCN registrations - Open iSCSI facilitates a subset of such SCNs.

http://linux.dell.com/files/presentations/Red_Hat_Summit_2009/Simplifying_Linux_iSCSI_management_with_iSNS-v0.7.pdf

Now if you want SCSI to do SCN registrations that will be a topic to discuss.. though I would say we don't go down that path..

Since both FC/FCoE and iSCSI can register with an iSNS/SNS server this should solve it atleast for the login/logout scenario.

Re: [PATCH RFC 0/9] [SCSI] Enhanced sense and Unit Attention handling

[Ewan Milne]

Thanks to everyone for the comments on this patch series.

Based upon the feedback, I'll be making the following changes
to the patch series before submitting it:

   - Remove patch 1/9 "Detect overflow of sense data buffer"
   - Change name of "sdev_evt_thread" to "sdev_evt_work"
   - Change name of "starget_evt_thread" to "starget_evt_work"
   - Pull code out of scsi_check_sense() that handles UAs into
     an exported function so that drivers can report conditions
     received asynchronously

The discussions about further changes (e.g. AEN, W-LUN) to
ensure conditions are reported to the host in the case of no
active I/O in progress or no LUNs currently presented have been
good.  I think this could be addressed with subsequent patches.

-Ewan