[PATCH v4 00/10] Enhanced Unit Attention handling

[PATCH v4 00/10] Enhanced 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.

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.  The expecting_cc_ua flag is used for REPORTED LUNS DATA HAS CHANGED
unit attention conditions to avoid generating duplicate events on multiple
LUNs.

Changes made since earlier v3 version:
   - Put fixes to existing code in separate individual patches
   - Removed UA queue overflow condition reporting
   - Removed delayed_work aggregation mechanism for events
   - Eliminated separate scsi_device and scsi_target mechanisms
   - Changed to use a single environment variable for uevents
   - Clear expecting_cc_ua on successful commands
   - Added use of expecting_cc_ua for REPORTED LUNS DATA HAS CHANGED

Changes made since earlier v2 version:

   - Remove patch 1/8 "Generate uevent on sd capacity change"
   - Remove patch 8/8 "Streamline detection of FM/EOM/ILI status"
   - Changed scsi_debug to not generate UA on INQUIRY or REPORT_LUNS
   - Changed scsi_debug to only report UA queue overflow condition
     if dsense=1, as descriptor format sense data is needed

Changes made since earlier RFC version:

   - Remove patch 1/9 "Detect overflow of sense data buffer"
     Some scsi_debug changes in this patch were moved to patch 7/8
   - Corrected Kconfig help text
   - 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

Thanks to everyone for the comments on this patch series.

Ewan D. Milne (10):
  scsi: Fix incorrect function name in comment
  scsi: Correct size of envp[]
  scsi: Add missing newline to scsi_sysfs.c
  scsi: Change to use list_for_each_entry_safe
  scsi: Rename scsi_evt_thread() to scsi_evt_work()
  scsi: Move schedule_work() call to be outside lock
  scsi: Clear expecting_cc_ua on successful commands
  scsi: Generate uevents on certain unit attention codes
  scsi_debug: Add optional unit attention reporting
  scsi: Added scsi_target rescan capability to sysfs

 drivers/scsi/scsi_debug.c  | 131 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_error.c  | 125 ++++++++++++++++++++++++++++++++++--------
 drivers/scsi/scsi_lib.c    |  52 +++++++++++++++---
 drivers/scsi/scsi_priv.h   |   5 +-
 drivers/scsi/scsi_scan.c   |  33 ++----------
 drivers/scsi/scsi_sysfs.c  |  81 +++++++++++++++++++++++++---
 include/scsi/scsi_device.h |  11 +++-
 7 files changed, 372 insertions(+), 66 deletions(-)

-- 
1.7.11.7

[PATCH v4 01/10] scsi: Fix incorrect function name in comment

[Ewan D. Milne]

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

The function name is "scsi_evt_emit", not "sdev_evt_emit".

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6dfb978..f6499db 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2171,7 +2171,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 EXPORT_SYMBOL(scsi_device_set_state);
 
 /**
- * 	sdev_evt_emit - emit a single SCSI device uevent
+ *	scsi_evt_emit - emit a single SCSI device uevent
  *	@sdev: associated SCSI device
  *	@evt: event to emit
  *
-- 
1.7.11.7

[PATCH v4 02/10] scsi: Correct size of envp[]

[Ewan D. Milne]

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

The envp[] array in scsi_evt_emit() only needs to have 2 entries.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6499db..6585049 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2180,7 +2180,7 @@ EXPORT_SYMBOL(scsi_device_set_state);
 static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 {
 	int idx = 0;
-	char *envp[3];
+	char *envp[2];
 
 	switch (evt->evt_type) {
 	case SDEV_EVT_MEDIA_CHANGE:
-- 
1.7.11.7

[PATCH v4 03/10] scsi: Add missing newline to scsi_sysfs.c

[Ewan D. Milne]

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

show_iostat_counterbits() is obviously missing a newline in
the function declaration.

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

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 04c2a27..7394a77 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -647,7 +647,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);
 }
-- 
1.7.11.7

[PATCH v4 04/10] scsi: Change to use list_for_each_entry_safe

[Ewan D. Milne]

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

scsi_device_dev_release_usercontext() should be using
"list_for_each_entry_safe" instead of "list_for_each_safe".

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

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 7394a77..34f7580 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -335,7 +335,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	struct scsi_device *sdev;
 	struct device *parent;
 	struct scsi_target *starget;
-	struct list_head *this, *tmp;
+	struct scsi_event *evt, *next;
 	unsigned long flags;
 
 	sdev = container_of(work, struct scsi_device, ew.work);
@@ -352,10 +352,7 @@ 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;
-
-		evt = list_entry(this, struct scsi_event, node);
+	list_for_each_entry_safe(evt, next, &sdev->event_list, node) {
 		list_del(&evt->node);
 		kfree(evt);
 	}
-- 
1.7.11.7

[PATCH v4 05/10] scsi: Rename scsi_evt_thread() to scsi_evt_work()

[Ewan D. Milne]

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

The scsi_evt_thread() function is not actually a thread, it is
a work function.  So it should be named scsi_evt_work() instead.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6585049..97699a5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2198,13 +2198,13 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 }
 
 /**
- * 	sdev_evt_thread - send a uevent for each scsi event
+ *	scsi_evt_work - send a uevent for each scsi event
  *	@work: work struct for scsi_device
  *
  *	Dispatch queued events to their associated scsi_device kobjects
  *	as uevents.
  */
-void scsi_evt_thread(struct work_struct *work)
+void scsi_evt_work(struct work_struct *work)
 {
 	struct scsi_device *sdev;
 	LIST_HEAD(event_list);
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 07ce3f5..ed80f21 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 scsi_evt_work(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..0adfecb 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, scsi_evt_work);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
-- 
1.7.11.7

[PATCH v4 06/10] scsi: Move schedule_work() call to be outside lock

[Ewan D. Milne]

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

The call to schedule_work() in sdev_evt_send() should
not be made while the sdev->list_lock is held.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 97699a5..f871ecf 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2255,8 +2255,8 @@ void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt)
 
 	spin_lock_irqsave(&sdev->list_lock, flags);
 	list_add_tail(&evt->node, &sdev->event_list);
-	schedule_work(&sdev->event_work);
 	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	schedule_work(&sdev->event_work);
 }
 EXPORT_SYMBOL_GPL(sdev_evt_send);
 
-- 
1.7.11.7

[PATCH v4 07/10] scsi: Clear expecting_cc_ua on successful commands

[Ewan D. Milne]

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

If a device does not report a unit attention as expected,
clear the expecting_cc_ua flag so that we will not suppress
a future unit attention condition that is *not* expected.
INQUIRY and REPORT LUNS commands should not do this, however,
because they do not report pending unit attention conditions.

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d0f71e5..96707a6 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1540,6 +1540,13 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 		 */
 		return ADD_TO_MLQUEUE;
 	case GOOD:
+		/*
+		 * Reset expecting_cc_ua for all commands except INQUIRY and
+		 * REPORT LUNS, because if we didn't get a UA on this command
+		 * as expected, then we don't want to suppress a subsequent one.
+		 */
+		if (scmd->cmnd[0] != INQUIRY && scmd->cmnd[0] != REPORT_LUNS)
+			scmd->device->expecting_cc_ua = 0;
 		scsi_handle_queue_ramp_up(scmd->device);
 	case COMMAND_TERMINATED:
 		return SUCCESS;
-- 
1.7.11.7

[PATCH v4 08/10] scsi: Generate uevents on certain unit attention codes

[Ewan D. Milne]

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

Generate a uevent 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
    3F/03  INQUIRY DATA HAS CHANGED
    3F/0E  REPORTED LUNS DATA HAS CHANGED

Log kernel messages when the following Unit Attention ASC/ASCQ
codes are received that are not as specific as those above:

    2A/xx  PARAMETERS CHANGED
    3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED

Added logic to set expecting_cc_ua for other LUNs on SPC-3 devices
after REPORTED LUNS DATA HAS CHANGED is received, so that duplicate
uevents are not generated, and clear expecting_cc_ua when a
REPORT LUNS command completes, in accordance with the SPC-3
specification regarding reporting of the 3F 0E ASC/ASCQ UA.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_error.c  | 118 +++++++++++++++++++++++++++++++++++++--------
 drivers/scsi/scsi_lib.c    |  42 ++++++++++++++--
 drivers/scsi/scsi_sysfs.c  |  10 ++++
 include/scsi/scsi_device.h |  11 ++++-
 4 files changed, 156 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 96707a6..227041a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -222,6 +222,86 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
 }
 #endif
 
+ /**
+ * scsi_report_lun_change - Set flag on all *other* devices on the same target
+ *                          to indicate that a UNIT ATTENTION is expected.
+ *                          Only do this for SPC-3 devices, however.
+ * @sdev:	Device reporting the UNIT ATTENTION
+ */
+static void scsi_report_lun_change(struct scsi_device *sdev)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct scsi_device *tmp_sdev;
+
+	if (sdev->scsi_level == SCSI_SPC_3)
+		shost_for_each_device(tmp_sdev, shost) {
+			if (tmp_sdev->channel == sdev->channel &&
+			    tmp_sdev->id == sdev->id &&
+			    tmp_sdev != sdev)
+				tmp_sdev->expecting_cc_ua = 1;
+		}
+}
+
+/**
+ * scsi_report_sense - Examine scsi sense information and log messages for
+ *		       certain conditions, also issue uevents for some of them.
+ * @sshdr:	sshdr to be examined
+ */
+static void scsi_report_sense(struct scsi_device *sdev,
+			      struct scsi_sense_hdr *sshdr)
+{
+	enum scsi_device_event evt_type = SDEV_EVT_MAXBITS;	/* i.e. none */
+	unsigned long flags;
+
+	if (sshdr->sense_key == UNIT_ATTENTION) {
+		if (sshdr->asc == 0x3f && sshdr->ascq == 0x03) {
+			evt_type = SDEV_EVT_INQUIRY_CHANGE_REPORTED;
+			sdev_printk(KERN_WARNING, sdev,
+				    "Inquiry data has changed");
+		} else if (sshdr->asc == 0x3f && sshdr->ascq == 0x0e) {
+			evt_type = SDEV_EVT_LUN_CHANGE_REPORTED;
+			scsi_report_lun_change(sdev);
+			sdev_printk(KERN_WARNING, sdev,
+				    "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)
+			sdev_printk(KERN_WARNING, sdev,
+				    "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) {
+			evt_type = SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED;
+			sdev_printk(KERN_WARNING, sdev,
+				    "Warning! Received an indication that the "
+				    "LUN reached a thin provisioning soft "
+				    "threshold.\n");
+		}
+
+		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
+			evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
+			sdev_printk(KERN_WARNING, sdev,
+				    "Mode parameters changed");
+		} else if (sshdr->asc == 0x2a && sshdr->ascq == 0x09) {
+			evt_type = SDEV_EVT_CAPACITY_CHANGE_REPORTED;
+			sdev_printk(KERN_WARNING, sdev,
+				    "Capacity data has changed");
+		} else if (sshdr->asc == 0x2a)
+			sdev_printk(KERN_WARNING, sdev,
+				    "Parameters changed");
+	}
+
+	if (evt_type != SDEV_EVT_MAXBITS) {
+		spin_lock_irqsave(&sdev->list_lock, flags);
+		set_bit(evt_type, sdev->pending_events);
+		spin_unlock_irqrestore(&sdev->list_lock, flags);
+		schedule_work(&sdev->event_work);
+	}
+}
+
 /**
  * scsi_check_sense - Examine scsi cmd sense
  * @scmd:	Cmd to have sense checked.
@@ -241,6 +321,8 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	if (! scsi_command_normalize_sense(scmd, &sshdr))
 		return FAILED;	/* no valid sense data */
 
+	scsi_report_sense(sdev, &sshdr);
+
 	if (scsi_sense_is_deferred(&sshdr))
 		return NEEDS_RETRY;
 
@@ -291,7 +373,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 * if we are expecting a cc/ua because of a bus reset that we
 		 * performed, treat this just as a retry.  otherwise this is
 		 * information that we should pass up to the upper-level driver
-		 * so that we can deal with it there.
+		 * so that we can deal with it there.  we might also expect a
+		 * cc/ua if another LUN on the target reported a UA with
+		 * ASC/ASCQ of 3F 0E - REPORTED LUNS DATA HAS CHANGED.
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			/*
@@ -318,26 +402,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.
@@ -1414,6 +1478,7 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
  */
 int scsi_decide_disposition(struct scsi_cmnd *scmd)
 {
+	struct scsi_device *sdev;
 	int rtn;
 
 	/*
@@ -1544,9 +1609,20 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 		 * Reset expecting_cc_ua for all commands except INQUIRY and
 		 * REPORT LUNS, because if we didn't get a UA on this command
 		 * as expected, then we don't want to suppress a subsequent one.
+		 *
+		 * A successful REPORT LUNS should reset expecting_cc_ua for
+		 * REPORTED LUNS DATA HAS CHANGED on all LUNs on the target.
+		 * (In general, we do not get a REPORT LUNS on the same LUN that
+		 * originally reported the REPORTED LUNS DATA HAS CHANGED UA.)
 		 */
 		if (scmd->cmnd[0] != INQUIRY && scmd->cmnd[0] != REPORT_LUNS)
 			scmd->device->expecting_cc_ua = 0;
+		else if (scmd->cmnd[0] == REPORT_LUNS)
+			shost_for_each_device(sdev, scmd->device->host) {
+				if (sdev->channel == scmd->device->channel &&
+				    sdev->id == scmd->device->id)
+					sdev->expecting_cc_ua = 0;
+			}
 		scsi_handle_queue_ramp_up(scmd->device);
 	case COMMAND_TERMINATED:
 		return SUCCESS;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f871ecf..1dd107f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2181,12 +2181,28 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 {
 	int idx = 0;
 	char *envp[2];
+	int target = 0;
 
 	switch (evt->evt_type) {
 	case SDEV_EVT_MEDIA_CHANGE:
 		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
 		break;
-
+	case SDEV_EVT_INQUIRY_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
+		break;
+	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_UA=CAPACITY_DATA_HAS_CHANGED";
+		break;
+	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
+	       envp[idx++] = "SDEV_UA=THIN_PROVISIONING_SOFT_THRESHOLD_REACHED";
+		break;
+	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_UA=MODE_PARAMETERS_CHANGED";
+		break;
+	case SDEV_EVT_LUN_CHANGE_REPORTED:
+		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
+		target = 1;
+		break;
 	default:
 		/* do nothing */
 		break;
@@ -2194,7 +2210,11 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 
 	envp[idx++] = NULL;
 
-	kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
+	if (target)
+		kobject_uevent_env(&sdev->sdev_target->dev.kobj, KOBJ_CHANGE,
+				   envp);
+	else
+		kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
 }
 
 /**
@@ -2207,14 +2227,25 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 void scsi_evt_work(struct work_struct *work)
 {
 	struct scsi_device *sdev;
+	unsigned long flags;
+	DECLARE_BITMAP(tmp_events, SDEV_EVT_MAXBITS);
+	enum scsi_device_event evt_type;
 	LIST_HEAD(event_list);
 
 	sdev = container_of(work, struct scsi_device, event_work);
 
+	spin_lock_irqsave(&sdev->list_lock, flags);
+	memcpy(tmp_events, sdev->pending_events, sizeof(sdev->pending_events));
+	memset(sdev->pending_events, 0, sizeof(sdev->pending_events));
+	spin_unlock_irqrestore(&sdev->list_lock, flags);
+
+	for (evt_type = SDEV_EVT_FIRST; evt_type <= SDEV_EVT_LAST; evt_type++)
+		if (test_bit(evt_type, tmp_events))
+			sdev_evt_send_simple(sdev, evt_type, GFP_KERNEL);
+
 	while (1) {
 		struct scsi_event *evt;
 		struct list_head *this, *tmp;
-		unsigned long flags;
 
 		spin_lock_irqsave(&sdev->list_lock, flags);
 		list_splice_init(&sdev->event_list, &event_list);
@@ -2280,6 +2311,11 @@ 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_INQUIRY_CHANGE_REPORTED:
+	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
+	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
+	case SDEV_EVT_LUN_CHANGE_REPORTED:
 	default:
 		/* do nothing */
 		break;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 34f7580..d5d86b2 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -710,6 +710,11 @@ 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(inquiry_change_reported, INQUIRY_CHANGE_REPORTED)
+DECLARE_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
+DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
+DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
+DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
 
 /* Default template for device attributes.  May NOT be modified */
 static struct attribute *scsi_sdev_attrs[] = {
@@ -729,6 +734,11 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_ioerr_cnt.attr,
 	&dev_attr_modalias.attr,
 	REF_EVT(media_change),
+	REF_EVT(inquiry_change_reported),
+	REF_EVT(capacity_change_reported),
+	REF_EVT(soft_threshold_reached),
+	REF_EVT(mode_parameter_change_reported),
+	REF_EVT(lun_change_reported),
 	NULL
 };
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6efb2e1..0a79f0c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -51,8 +51,16 @@ enum scsi_device_state {
 
 enum scsi_device_event {
 	SDEV_EVT_MEDIA_CHANGE	= 1,	/* media has changed */
+	SDEV_EVT_INQUIRY_CHANGE_REPORTED	= 2,	/* UA reported */
+	SDEV_EVT_CAPACITY_CHANGE_REPORTED	= 3,	/* UA reported */
+	SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED	= 4,  /* UA reported */
+	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED	= 5,	/* UA reported */
+
+	SDEV_EVT_LUN_CHANGE_REPORTED	= 6,	/* UA reported */
+
+	SDEV_EVT_FIRST		= SDEV_EVT_MEDIA_CHANGE,
+	SDEV_EVT_LAST		= SDEV_EVT_LUN_CHANGE_REPORTED,
 
-	SDEV_EVT_LAST		= SDEV_EVT_MEDIA_CHANGE,
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
 
@@ -154,6 +162,7 @@ struct scsi_device {
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
+	DECLARE_BITMAP(pending_events, SDEV_EVT_MAXBITS); /* pending events */
 	struct list_head event_list;	/* asserted events */
 	struct work_struct event_work;
 
-- 
1.7.11.7

[PATCH v4 09/10] scsi_debug: Add optional unit attention reporting

[Ewan D. Milne]

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

Added "report_ua" module parameter to control reporting
of unit attention conditions when the number of LUNs is
changed, or the virtual_gb size of the device is changed.

Also added capability to generate unit attention conditions:

   38 07 - THIN PROVISIONING SOFT THRESHOLD REACHED
   2A 01 - MODE PARAMETERS CHANGED

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

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 182d5a5..738b197 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -111,6 +111,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_PTYPE   0
 #define DEF_SCSI_LEVEL   5    /* INQUIRY, byte2 [5->SPC-3] */
 #define DEF_SECTOR_SIZE 512
+#define DEF_REPORT_UA 0
 #define DEF_UNMAP_ALIGNMENT 0
 #define DEF_UNMAP_GRANULARITY 1
 #define DEF_UNMAP_MAX_BLOCKS 0xFFFFFFFF
@@ -182,6 +183,7 @@ static int scsi_debug_physblk_exp = DEF_PHYSBLK_EXP;
 static int scsi_debug_ptype = DEF_PTYPE; /* SCSI peripheral type (0==disk) */
 static int scsi_debug_scsi_level = DEF_SCSI_LEVEL;
 static int scsi_debug_sector_size = DEF_SECTOR_SIZE;
+static int scsi_debug_report_ua = DEF_REPORT_UA;
 static int scsi_debug_virtual_gb = DEF_VIRTUAL_GB;
 static int scsi_debug_vpd_use_hostno = DEF_VPD_USE_HOSTNO;
 static unsigned int scsi_debug_lbpu = DEF_LBPU;
@@ -230,6 +232,8 @@ struct sdebug_dev_info {
 	char reset;
 	char stopped;
 	char used;
+	char luns_changed;
+	char sense_pending;
 };
 
 struct sdebug_host_info {
@@ -268,6 +272,7 @@ static int num_host_resets = 0;
 static int dix_writes;
 static int dix_reads;
 static int dif_errors;
+static int luns_changed;
 
 static DEFINE_SPINLOCK(queued_arr_lock);
 static DEFINE_RWLOCK(atomic_rw);
@@ -2756,6 +2761,7 @@ module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
 module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
 module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
 module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
+module_param_named(report_ua, scsi_debug_report_ua, int, S_IRUGO | S_IWUSR);
 module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
 module_param_named(unmap_granularity, scsi_debug_unmap_granularity, int, S_IRUGO);
 module_param_named(unmap_max_blocks, scsi_debug_unmap_max_blocks, int, S_IRUGO);
@@ -2798,6 +2804,7 @@ MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
 MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
 MODULE_PARM_DESC(scsi_level, "SCSI level to simulate(def=5[SPC-3])");
 MODULE_PARM_DESC(sector_size, "logical block size in bytes (def=512)");
+MODULE_PARM_DESC(report_ua, "report unit attentions when reconfigured (def=0)");
 MODULE_PARM_DESC(unmap_alignment, "lowest aligned thin provisioning lba (def=0)");
 MODULE_PARM_DESC(unmap_granularity, "thin provisioning granularity in blocks (def=1)");
 MODULE_PARM_DESC(unmap_max_blocks, "max # of blocks can be unmapped in one cmd (def=0xffffffff)");
@@ -3050,10 +3057,37 @@ static ssize_t sdebug_max_luns_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)) {
 		scsi_debug_max_luns = n;
 		sdebug_max_tgts_luns();
+
+		if (!scsi_debug_report_ua)
+			return count;
+
+		/*
+		 * SPC-3 behavior is to report a UNIT ATTENTION with ASC/ASCQ
+		 * REPORTED LUNS DATA HAS CHANGED on every LUN on the target,
+		 * until a REPORT LUNS command is received.  SPC-4 behavior is
+		 * to report it only once.  NOTE:  scsi_debug_scsi_level does
+		 * not use the same values as struct scsi_device->scsi_level.
+		 */
+		if (scsi_debug_scsi_level == 5) {	/* SPC-3 */
+			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) {
+					devip->luns_changed = 1;
+				}
+			}
+			spin_unlock(&sdebug_host_list_lock);
+		} else if (scsi_debug_scsi_level > 5)
+			luns_changed = 1;
+
 		return count;
 	}
 	return -EINVAL;
@@ -3100,12 +3134,27 @@ static ssize_t sdebug_virtual_gb_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)) {
 		scsi_debug_virtual_gb = n;
 
 		sdebug_capacity = get_sdebug_capacity();
 
+		if (!scsi_debug_report_ua)
+			return count;
+
+		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);
 		return count;
 	}
 	return -EINVAL;
@@ -3205,6 +3254,57 @@ 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_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);
 
 /* Note: The following function creates attribute files in the
    /sys/bus/pseudo/drivers/scsi_debug directory. The advantage of these
@@ -3239,11 +3339,19 @@ 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_soft_threshold_reached);
+	ret |= driver_create_file(&sdebug_driverfs_driver,
+				  &driver_attr_mode_parameters_changed);
 	return ret;
 }
 
 static void do_remove_driverfs_files(void)
 {
+	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_map);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_ato);
 	driver_remove_file(&sdebug_driverfs_driver, &driver_attr_guard);
@@ -3597,6 +3705,8 @@ int scsi_debug_queuecommand_lck(struct scsi_cmnd *SCpnt, done_funct_t done)
 	int inj_dix = 0;
 	int delay_override = 0;
 	int unmap = 0;
+	struct sdebug_host_info *sdbg_host;
+	struct sdebug_dev_info *tmpdevip;
 
 	scsi_set_resid(SCpnt, 0);
 	if ((SCSI_DEBUG_OPT_NOISE & scsi_debug_opts) && cmd) {
@@ -3774,6 +3884,15 @@ read:
 		}
 		break;
 	case REPORT_LUNS:	/* mandatory, ignore unit attention */
+		luns_changed = 0;
+		spin_lock(&sdebug_host_list_lock);
+		list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+			list_for_each_entry(tmpdevip, &sdbg_host->dev_info_list,
+					    dev_list) {
+				tmpdevip->luns_changed = 0;
+			}
+		}
+		spin_unlock(&sdebug_host_list_lock);
 		delay_override = 1;
 		errsts = resp_report_luns(SCpnt, devip);
 		break;
@@ -3926,6 +4045,18 @@ write:
 		errsts = check_condition_result;
 		break;
 	}
+	if (!errsts && (*cmd != INQUIRY) && (*cmd != REPORT_LUNS)) {
+		if (!devip->sense_pending &&
+		    (luns_changed || devip->luns_changed)) {
+			luns_changed = 0;
+			devip->luns_changed = 0;
+			mk_sense_buffer(devip, UNIT_ATTENTION, 0x3f, 0x0e);
+			errsts = check_condition_result;
+		} else if (devip->sense_pending) {
+			devip->sense_pending = 0;
+			errsts = check_condition_result;
+		}
+	}
 	return schedule_resp(SCpnt, devip, done, errsts,
 			     (delay_override ? 0 : scsi_debug_delay));
 }
-- 
1.7.11.7

[PATCH v4 10/10] scsi: Added scsi_target rescan capability to sysfs

[Ewan D. Milne]

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

Add a "rescan" attribute to sysfs for scsi_target objects,
to permit them to be scanned for LUN changes (e.g. from udev).

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

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index ed80f21..7c33799 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -131,7 +131,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 0adfecb..243c8b4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -343,26 +343,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)
 {
@@ -413,15 +393,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 d5d86b2..212b43a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1129,6 +1129,67 @@ 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);
+
+static struct attribute *scsi_target_attrs[] = {
+	&dev_attr_trescan.attr,
+	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

Re: [PATCH v4 00/10] Enhanced Unit Attention handling

[James Bottomley]

On Thu, 2013-08-01 at 16:57 -0400, Ewan D. Milne wrote:
> 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.

Well it doesn't really does it?  As I see it, patches 1-6 are trivial
changes unrelated to this so just roll them up and resend as a single
patch later when we've got the actual real stuff sorted out (except
patch 6 which looks bogus), patch 7 should be part of patch 8 which is
the actual unit attention events, patch 9 is an extension of scsi_debug
to generate UAs which needs to go separately via Doug and patch 10 is an
unrelated addition of a scan target parameter.

James

Re: [PATCH v4 08/10] scsi: Generate uevents on certain unit attention codes

[James Bottomley]

On Thu, 2013-08-01 at 16:57 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> Generate a uevent 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
>     3F/03  INQUIRY DATA HAS CHANGED
>     3F/0E  REPORTED LUNS DATA HAS CHANGED
> 
> Log kernel messages when the following Unit Attention ASC/ASCQ
> codes are received that are not as specific as those above:
> 
>     2A/xx  PARAMETERS CHANGED
>     3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED
> 
> Added logic to set expecting_cc_ua for other LUNs on SPC-3 devices
> after REPORTED LUNS DATA HAS CHANGED is received, so that duplicate
> uevents are not generated, and clear expecting_cc_ua when a
> REPORT LUNS command completes, in accordance with the SPC-3
> specification regarding reporting of the 3F 0E ASC/ASCQ UA.
> 
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>  drivers/scsi/scsi_error.c  | 118 +++++++++++++++++++++++++++++++++++++--------
>  drivers/scsi/scsi_lib.c    |  42 ++++++++++++++--
>  drivers/scsi/scsi_sysfs.c  |  10 ++++
>  include/scsi/scsi_device.h |  11 ++++-
>  4 files changed, 156 insertions(+), 25 deletions(-)

In general, this looks good.  Just a few points below, and I think it
can go in.

> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 96707a6..227041a 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -222,6 +222,86 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
>  }
>  #endif
>  
> + /**
> + * scsi_report_lun_change - Set flag on all *other* devices on the same target
> + *                          to indicate that a UNIT ATTENTION is expected.
> + *                          Only do this for SPC-3 devices, however.
> + * @sdev:	Device reporting the UNIT ATTENTION
> + */
> +static void scsi_report_lun_change(struct scsi_device *sdev)
> +{
> +	struct Scsi_Host *shost = sdev->host;
> +	struct scsi_device *tmp_sdev;
> +
> +	if (sdev->scsi_level == SCSI_SPC_3)

Why the SPC3 check?  We have SPC2 targets that use report luns and
presumably work as well.

> +		shost_for_each_device(tmp_sdev, shost) {
> +			if (tmp_sdev->channel == sdev->channel &&
> +			    tmp_sdev->id == sdev->id &&
> +			    tmp_sdev != sdev)

This should be starget_for_each_device calling a function rather than
hand rolling.

> +				tmp_sdev->expecting_cc_ua = 1;

Even with a restricted target loop, this is a bit messy (I think it's my
fault: I did ask you to reuse the existing mechanism, but now I see it,
a separate mechanism that functions the same way on the target looks a
lot cleaner) .  It looks like a struct scsi_target
expecting_lun_change:1 flag would work better in this case?  You'd set
it in the target at first sight, check it in scsi_report_sense() and
clear it on successful REPORT_LUNS command.  There's no need to lock it
because operations on a u32 are atomic and we're going to get slop
around this and the possibility of extra events anyway.

> +		}
> +}
> +
> +/**
> + * scsi_report_sense - Examine scsi sense information and log messages for
> + *		       certain conditions, also issue uevents for some of them.
> + * @sshdr:	sshdr to be examined
> + */
> +static void scsi_report_sense(struct scsi_device *sdev,
> +			      struct scsi_sense_hdr *sshdr)
> +{
> +	enum scsi_device_event evt_type = SDEV_EVT_MAXBITS;	/* i.e. none */
> +	unsigned long flags;
> +
> +	if (sshdr->sense_key == UNIT_ATTENTION) {
> +		if (sshdr->asc == 0x3f && sshdr->ascq == 0x03) {
> +			evt_type = SDEV_EVT_INQUIRY_CHANGE_REPORTED;
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Inquiry data has changed");
> +		} else if (sshdr->asc == 0x3f && sshdr->ascq == 0x0e) {
> +			evt_type = SDEV_EVT_LUN_CHANGE_REPORTED;
> +			scsi_report_lun_change(sdev);
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "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)
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "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) {
> +			evt_type = SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED;
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Warning! Received an indication that the "
> +				    "LUN reached a thin provisioning soft "
> +				    "threshold.\n");
> +		}
> +
> +		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
> +			evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Mode parameters changed");
> +		} else if (sshdr->asc == 0x2a && sshdr->ascq == 0x09) {
> +			evt_type = SDEV_EVT_CAPACITY_CHANGE_REPORTED;
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Capacity data has changed");
> +		} else if (sshdr->asc == 0x2a)
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Parameters changed");
> +	}
> +
> +	if (evt_type != SDEV_EVT_MAXBITS) {
> +		spin_lock_irqsave(&sdev->list_lock, flags);
> +		set_bit(evt_type, sdev->pending_events);
> +		spin_unlock_irqrestore(&sdev->list_lock, flags);

This is a bitmap: bitmaps can be processed locklessly, just use
test_and_clear_bit in the consumer.

> +		schedule_work(&sdev->event_work);
> +	}
> +}
> +
>  /**
>   * scsi_check_sense - Examine scsi cmd sense
>   * @scmd:	Cmd to have sense checked.
> @@ -241,6 +321,8 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>  	if (! scsi_command_normalize_sense(scmd, &sshdr))
>  		return FAILED;	/* no valid sense data */
>  
> +	scsi_report_sense(sdev, &sshdr);
> +
>  	if (scsi_sense_is_deferred(&sshdr))
>  		return NEEDS_RETRY;
>  
> @@ -291,7 +373,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>  		 * if we are expecting a cc/ua because of a bus reset that we
>  		 * performed, treat this just as a retry.  otherwise this is
>  		 * information that we should pass up to the upper-level driver
> -		 * so that we can deal with it there.
> +		 * so that we can deal with it there.  we might also expect a
> +		 * cc/ua if another LUN on the target reported a UA with
> +		 * ASC/ASCQ of 3F 0E - REPORTED LUNS DATA HAS CHANGED.
>  		 */
>  		if (scmd->device->expecting_cc_ua) {
>  			/*
> @@ -318,26 +402,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.
> @@ -1414,6 +1478,7 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
>   */
>  int scsi_decide_disposition(struct scsi_cmnd *scmd)
>  {
> +	struct scsi_device *sdev;
>  	int rtn;
>  
>  	/*
> @@ -1544,9 +1609,20 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
>  		 * Reset expecting_cc_ua for all commands except INQUIRY and
>  		 * REPORT LUNS, because if we didn't get a UA on this command
>  		 * as expected, then we don't want to suppress a subsequent one.
> +		 *
> +		 * A successful REPORT LUNS should reset expecting_cc_ua for
> +		 * REPORTED LUNS DATA HAS CHANGED on all LUNs on the target.
> +		 * (In general, we do not get a REPORT LUNS on the same LUN that
> +		 * originally reported the REPORTED LUNS DATA HAS CHANGED UA.)
>  		 */
>  		if (scmd->cmnd[0] != INQUIRY && scmd->cmnd[0] != REPORT_LUNS)
>  			scmd->device->expecting_cc_ua = 0;
> +		else if (scmd->cmnd[0] == REPORT_LUNS)
> +			shost_for_each_device(sdev, scmd->device->host) {
> +				if (sdev->channel == scmd->device->channel &&
> +				    sdev->id == scmd->device->id)

starget_for_each_device(), but switching to a target flag makes this
entire loop go away.

> +					sdev->expecting_cc_ua = 0;
> +			}
>  		scsi_handle_queue_ramp_up(scmd->device);
>  	case COMMAND_TERMINATED:
>  		return SUCCESS;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f871ecf..1dd107f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2181,12 +2181,28 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>  {
>  	int idx = 0;
>  	char *envp[2];
> +	int target = 0;
>  
>  	switch (evt->evt_type) {
>  	case SDEV_EVT_MEDIA_CHANGE:
>  		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
>  		break;
> -
> +	case SDEV_EVT_INQUIRY_CHANGE_REPORTED:
> +		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
> +		break;
> +	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
> +		envp[idx++] = "SDEV_UA=CAPACITY_DATA_HAS_CHANGED";
> +		break;
> +	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
> +	       envp[idx++] = "SDEV_UA=THIN_PROVISIONING_SOFT_THRESHOLD_REACHED";
> +		break;
> +	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
> +		envp[idx++] = "SDEV_UA=MODE_PARAMETERS_CHANGED";
> +		break;
> +	case SDEV_EVT_LUN_CHANGE_REPORTED:
> +		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
> +		target = 1;
> +		break;
>  	default:
>  		/* do nothing */
>  		break;
> @@ -2194,7 +2210,11 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>  
>  	envp[idx++] = NULL;
>  
> -	kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
> +	if (target)
> +		kobject_uevent_env(&sdev->sdev_target->dev.kobj, KOBJ_CHANGE,
> +				   envp);
> +	else
> +		kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
>  }
>  
>  /**
> @@ -2207,14 +2227,25 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>  void scsi_evt_work(struct work_struct *work)
>  {
>  	struct scsi_device *sdev;
> +	unsigned long flags;
> +	DECLARE_BITMAP(tmp_events, SDEV_EVT_MAXBITS);
> +	enum scsi_device_event evt_type;
>  	LIST_HEAD(event_list);
>  
>  	sdev = container_of(work, struct scsi_device, event_work);
>  
> +	spin_lock_irqsave(&sdev->list_lock, flags);
> +	memcpy(tmp_events, sdev->pending_events, sizeof(sdev->pending_events));
> +	memset(sdev->pending_events, 0, sizeof(sdev->pending_events));
> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +
> +	for (evt_type = SDEV_EVT_FIRST; evt_type <= SDEV_EVT_LAST; evt_type++)
> +		if (test_bit(evt_type, tmp_events))
> +			sdev_evt_send_simple(sdev, evt_type, GFP_KERNEL);
> +
>  	while (1) {
>  		struct scsi_event *evt;
>  		struct list_head *this, *tmp;
> -		unsigned long flags;
>  
>  		spin_lock_irqsave(&sdev->list_lock, flags);
>  		list_splice_init(&sdev->event_list, &event_list);
> @@ -2280,6 +2311,11 @@ 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_INQUIRY_CHANGE_REPORTED:
> +	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
> +	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
> +	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
> +	case SDEV_EVT_LUN_CHANGE_REPORTED:
>  	default:
>  		/* do nothing */
>  		break;
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 34f7580..d5d86b2 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -710,6 +710,11 @@ 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(inquiry_change_reported, INQUIRY_CHANGE_REPORTED)
> +DECLARE_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
> +DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
> +DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
> +DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
>  
>  /* Default template for device attributes.  May NOT be modified */
>  static struct attribute *scsi_sdev_attrs[] = {
> @@ -729,6 +734,11 @@ static struct attribute *scsi_sdev_attrs[] = {
>  	&dev_attr_ioerr_cnt.attr,
>  	&dev_attr_modalias.attr,
>  	REF_EVT(media_change),
> +	REF_EVT(inquiry_change_reported),
> +	REF_EVT(capacity_change_reported),
> +	REF_EVT(soft_threshold_reached),
> +	REF_EVT(mode_parameter_change_reported),
> +	REF_EVT(lun_change_reported),

This last one should be a property of the target attributes, shouldn't
it?  Otherwise the listener receives and event on the target and has to
go fishing around trying to find the one LUN we set the flag on.

Either we keep the flag on the sdev and send the event from the sdev or
we move the flag to the target and send the event from the target.

>  	NULL
>  };
>  
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 6efb2e1..0a79f0c 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -51,8 +51,16 @@ enum scsi_device_state {
>  
>  enum scsi_device_event {
>  	SDEV_EVT_MEDIA_CHANGE	= 1,	/* media has changed */
> +	SDEV_EVT_INQUIRY_CHANGE_REPORTED	= 2,	/* UA reported */
> +	SDEV_EVT_CAPACITY_CHANGE_REPORTED	= 3,	/* UA reported */
> +	SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED	= 4,  /* UA reported */
> +	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED	= 5,	/* UA reported */
> +
> +	SDEV_EVT_LUN_CHANGE_REPORTED	= 6,	/* UA reported */

You don't need to explicitly number an enumeration.  The point is the
compiler does it for you so we can't screw up and add something with the
same number twice.

James

> +
> +	SDEV_EVT_FIRST		= SDEV_EVT_MEDIA_CHANGE,
> +	SDEV_EVT_LAST		= SDEV_EVT_LUN_CHANGE_REPORTED,
>  
> -	SDEV_EVT_LAST		= SDEV_EVT_MEDIA_CHANGE,
>  	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
>  };
>  
> @@ -154,6 +162,7 @@ struct scsi_device {
>  	unsigned is_visible:1;	/* is the device visible in sysfs */
>  
>  	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
> +	DECLARE_BITMAP(pending_events, SDEV_EVT_MAXBITS); /* pending events */
>  	struct list_head event_list;	/* asserted events */
>  	struct work_struct event_work;
>  

Re: [PATCH v4 08/10] scsi: Generate uevents on certain unit attention codes

[Ewan Milne]

On Fri, 2013-08-02 at 10:06 -0700, James Bottomley wrote:
> On Thu, 2013-08-01 at 16:57 -0400, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <emilne@redhat.com>
> > 
> > Generate a uevent 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
> >     3F/03  INQUIRY DATA HAS CHANGED
> >     3F/0E  REPORTED LUNS DATA HAS CHANGED
> > 
> > Log kernel messages when the following Unit Attention ASC/ASCQ
> > codes are received that are not as specific as those above:
> > 
> >     2A/xx  PARAMETERS CHANGED
> >     3F/xx  TARGET OPERATING CONDITIONS HAVE CHANGED
> > 
> > Added logic to set expecting_cc_ua for other LUNs on SPC-3 devices
> > after REPORTED LUNS DATA HAS CHANGED is received, so that duplicate
> > uevents are not generated, and clear expecting_cc_ua when a
> > REPORT LUNS command completes, in accordance with the SPC-3
> > specification regarding reporting of the 3F 0E ASC/ASCQ UA.
> > 
> > Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> > ---
> >  drivers/scsi/scsi_error.c  | 118 +++++++++++++++++++++++++++++++++++++--------
> >  drivers/scsi/scsi_lib.c    |  42 ++++++++++++++--
> >  drivers/scsi/scsi_sysfs.c  |  10 ++++
> >  include/scsi/scsi_device.h |  11 ++++-
> >  4 files changed, 156 insertions(+), 25 deletions(-)
> 
> In general, this looks good.  Just a few points below, and I think it
> can go in.
> 
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 96707a6..227041a 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -222,6 +222,86 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
> >  }
> >  #endif
> >  
> > + /**
> > + * scsi_report_lun_change - Set flag on all *other* devices on the same target
> > + *                          to indicate that a UNIT ATTENTION is expected.
> > + *                          Only do this for SPC-3 devices, however.
> > + * @sdev:	Device reporting the UNIT ATTENTION
> > + */
> > +static void scsi_report_lun_change(struct scsi_device *sdev)
> > +{
> > +	struct Scsi_Host *shost = sdev->host;
> > +	struct scsi_device *tmp_sdev;
> > +
> > +	if (sdev->scsi_level == SCSI_SPC_3)
> 
> Why the SPC3 check?  We have SPC2 targets that use report luns and
> presumably work as well.

Really the check was for SPC-4 and above, which I believe only generates
a single REPORT LUNS DATA HAS CHANGED unit attention on the first LUN
accessed.  This was described in T10 06-411r2.  I think it is still
needed but should be changed to <= SCSI_SPC_3.

> 
> > +		shost_for_each_device(tmp_sdev, shost) {
> > +			if (tmp_sdev->channel == sdev->channel &&
> > +			    tmp_sdev->id == sdev->id &&
> > +			    tmp_sdev != sdev)
> 
> This should be starget_for_each_device calling a function rather than
> hand rolling.
> 
> > +				tmp_sdev->expecting_cc_ua = 1;
> 
> Even with a restricted target loop, this is a bit messy (I think it's my
> fault: I did ask you to reuse the existing mechanism, but now I see it,
> a separate mechanism that functions the same way on the target looks a
> lot cleaner) .  It looks like a struct scsi_target
> expecting_lun_change:1 flag would work better in this case?  You'd set
> it in the target at first sight, check it in scsi_report_sense() and
> clear it on successful REPORT_LUNS command.  There's no need to lock it
> because operations on a u32 are atomic and we're going to get slop
> around this and the possibility of extra events anyway.

I've tried this out, and it seems to work OK, but I'm a little concerned
about the possibility of losing a subsequent notification.  If a flag is
used on the target, then multiple 3F 0E UAs from the same device could
be suppressed, which isn't exactly the correct behavior.  Each LUN is
only supposed to report 3F 0E once in SPC-3, per change.  If we receive
another one, then there presumably has been *another* change.

Perhaps I'm being too picky about this, and I'm not at all sure that
every storage array has this implemented properly either, so maybe it
doesn't matter enough.

> 
> > +		}
> > +}
> > +
> > +/**
> > + * scsi_report_sense - Examine scsi sense information and log messages for
> > + *		       certain conditions, also issue uevents for some of them.
> > + * @sshdr:	sshdr to be examined
> > + */
> > +static void scsi_report_sense(struct scsi_device *sdev,
> > +			      struct scsi_sense_hdr *sshdr)
> > +{
> > +	enum scsi_device_event evt_type = SDEV_EVT_MAXBITS;	/* i.e. none */
> > +	unsigned long flags;
> > +
> > +	if (sshdr->sense_key == UNIT_ATTENTION) {
> > +		if (sshdr->asc == 0x3f && sshdr->ascq == 0x03) {
> > +			evt_type = SDEV_EVT_INQUIRY_CHANGE_REPORTED;
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "Inquiry data has changed");
> > +		} else if (sshdr->asc == 0x3f && sshdr->ascq == 0x0e) {
> > +			evt_type = SDEV_EVT_LUN_CHANGE_REPORTED;
> > +			scsi_report_lun_change(sdev);
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "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)
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "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) {
> > +			evt_type = SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED;
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "Warning! Received an indication that the "
> > +				    "LUN reached a thin provisioning soft "
> > +				    "threshold.\n");
> > +		}
> > +
> > +		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
> > +			evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "Mode parameters changed");
> > +		} else if (sshdr->asc == 0x2a && sshdr->ascq == 0x09) {
> > +			evt_type = SDEV_EVT_CAPACITY_CHANGE_REPORTED;
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "Capacity data has changed");
> > +		} else if (sshdr->asc == 0x2a)
> > +			sdev_printk(KERN_WARNING, sdev,
> > +				    "Parameters changed");
> > +	}
> > +
> > +	if (evt_type != SDEV_EVT_MAXBITS) {
> > +		spin_lock_irqsave(&sdev->list_lock, flags);
> > +		set_bit(evt_type, sdev->pending_events);
> > +		spin_unlock_irqrestore(&sdev->list_lock, flags);
> 
> This is a bitmap: bitmaps can be processed locklessly, just use
> test_and_clear_bit in the consumer.
> 
> > +		schedule_work(&sdev->event_work);
> > +	}
> > +}
> > +
> >  /**
> >   * scsi_check_sense - Examine scsi cmd sense
> >   * @scmd:	Cmd to have sense checked.
> > @@ -241,6 +321,8 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> >  	if (! scsi_command_normalize_sense(scmd, &sshdr))
> >  		return FAILED;	/* no valid sense data */
> >  
> > +	scsi_report_sense(sdev, &sshdr);
> > +
> >  	if (scsi_sense_is_deferred(&sshdr))
> >  		return NEEDS_RETRY;
> >  
> > @@ -291,7 +373,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
> >  		 * if we are expecting a cc/ua because of a bus reset that we
> >  		 * performed, treat this just as a retry.  otherwise this is
> >  		 * information that we should pass up to the upper-level driver
> > -		 * so that we can deal with it there.
> > +		 * so that we can deal with it there.  we might also expect a
> > +		 * cc/ua if another LUN on the target reported a UA with
> > +		 * ASC/ASCQ of 3F 0E - REPORTED LUNS DATA HAS CHANGED.
> >  		 */
> >  		if (scmd->device->expecting_cc_ua) {
> >  			/*
> > @@ -318,26 +402,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.
> > @@ -1414,6 +1478,7 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
> >   */
> >  int scsi_decide_disposition(struct scsi_cmnd *scmd)
> >  {
> > +	struct scsi_device *sdev;
> >  	int rtn;
> >  
> >  	/*
> > @@ -1544,9 +1609,20 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
> >  		 * Reset expecting_cc_ua for all commands except INQUIRY and
> >  		 * REPORT LUNS, because if we didn't get a UA on this command
> >  		 * as expected, then we don't want to suppress a subsequent one.
> > +		 *
> > +		 * A successful REPORT LUNS should reset expecting_cc_ua for
> > +		 * REPORTED LUNS DATA HAS CHANGED on all LUNs on the target.
> > +		 * (In general, we do not get a REPORT LUNS on the same LUN that
> > +		 * originally reported the REPORTED LUNS DATA HAS CHANGED UA.)
> >  		 */
> >  		if (scmd->cmnd[0] != INQUIRY && scmd->cmnd[0] != REPORT_LUNS)
> >  			scmd->device->expecting_cc_ua = 0;
> > +		else if (scmd->cmnd[0] == REPORT_LUNS)
> > +			shost_for_each_device(sdev, scmd->device->host) {
> > +				if (sdev->channel == scmd->device->channel &&
> > +				    sdev->id == scmd->device->id)
> 
> starget_for_each_device(), but switching to a target flag makes this
> entire loop go away.
> 
> > +					sdev->expecting_cc_ua = 0;
> > +			}
> >  		scsi_handle_queue_ramp_up(scmd->device);
> >  	case COMMAND_TERMINATED:
> >  		return SUCCESS;
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index f871ecf..1dd107f 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -2181,12 +2181,28 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
> >  {
> >  	int idx = 0;
> >  	char *envp[2];
> > +	int target = 0;
> >  
> >  	switch (evt->evt_type) {
> >  	case SDEV_EVT_MEDIA_CHANGE:
> >  		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
> >  		break;
> > -
> > +	case SDEV_EVT_INQUIRY_CHANGE_REPORTED:
> > +		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
> > +		break;
> > +	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
> > +		envp[idx++] = "SDEV_UA=CAPACITY_DATA_HAS_CHANGED";
> > +		break;
> > +	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
> > +	       envp[idx++] = "SDEV_UA=THIN_PROVISIONING_SOFT_THRESHOLD_REACHED";
> > +		break;
> > +	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
> > +		envp[idx++] = "SDEV_UA=MODE_PARAMETERS_CHANGED";
> > +		break;
> > +	case SDEV_EVT_LUN_CHANGE_REPORTED:
> > +		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
> > +		target = 1;
> > +		break;
> >  	default:
> >  		/* do nothing */
> >  		break;
> > @@ -2194,7 +2210,11 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
> >  
> >  	envp[idx++] = NULL;
> >  
> > -	kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
> > +	if (target)
> > +		kobject_uevent_env(&sdev->sdev_target->dev.kobj, KOBJ_CHANGE,
> > +				   envp);
> > +	else
> > +		kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
> >  }
> >  
> >  /**
> > @@ -2207,14 +2227,25 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
> >  void scsi_evt_work(struct work_struct *work)
> >  {
> >  	struct scsi_device *sdev;
> > +	unsigned long flags;
> > +	DECLARE_BITMAP(tmp_events, SDEV_EVT_MAXBITS);
> > +	enum scsi_device_event evt_type;
> >  	LIST_HEAD(event_list);
> >  
> >  	sdev = container_of(work, struct scsi_device, event_work);
> >  
> > +	spin_lock_irqsave(&sdev->list_lock, flags);
> > +	memcpy(tmp_events, sdev->pending_events, sizeof(sdev->pending_events));
> > +	memset(sdev->pending_events, 0, sizeof(sdev->pending_events));
> > +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> > +
> > +	for (evt_type = SDEV_EVT_FIRST; evt_type <= SDEV_EVT_LAST; evt_type++)
> > +		if (test_bit(evt_type, tmp_events))
> > +			sdev_evt_send_simple(sdev, evt_type, GFP_KERNEL);
> > +
> >  	while (1) {
> >  		struct scsi_event *evt;
> >  		struct list_head *this, *tmp;
> > -		unsigned long flags;
> >  
> >  		spin_lock_irqsave(&sdev->list_lock, flags);
> >  		list_splice_init(&sdev->event_list, &event_list);
> > @@ -2280,6 +2311,11 @@ 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_INQUIRY_CHANGE_REPORTED:
> > +	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
> > +	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
> > +	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
> > +	case SDEV_EVT_LUN_CHANGE_REPORTED:
> >  	default:
> >  		/* do nothing */
> >  		break;
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 34f7580..d5d86b2 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -710,6 +710,11 @@ 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(inquiry_change_reported, INQUIRY_CHANGE_REPORTED)
> > +DECLARE_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
> > +DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
> > +DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
> > +DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
> >  
> >  /* Default template for device attributes.  May NOT be modified */
> >  static struct attribute *scsi_sdev_attrs[] = {
> > @@ -729,6 +734,11 @@ static struct attribute *scsi_sdev_attrs[] = {
> >  	&dev_attr_ioerr_cnt.attr,
> >  	&dev_attr_modalias.attr,
> >  	REF_EVT(media_change),
> > +	REF_EVT(inquiry_change_reported),
> > +	REF_EVT(capacity_change_reported),
> > +	REF_EVT(soft_threshold_reached),
> > +	REF_EVT(mode_parameter_change_reported),
> > +	REF_EVT(lun_change_reported),
> 
> This last one should be a property of the target attributes, shouldn't
> it?  Otherwise the listener receives and event on the target and has to
> go fishing around trying to find the one LUN we set the flag on.
> 
> Either we keep the flag on the sdev and send the event from the sdev or
> we move the flag to the target and send the event from the target.

I could go either way on this -- conceptually it seems like the event
should be reported on the scsi_target object, since the accessible LUNs
on the target is what has actually changed on the storage, the LUN is
just the messenger.  However, implementing this required adding a bunch
of infrastructure for the uevent on the scsi_target object, which made
the patch bigger, so I took that out in the v4 version.

Sending the event to the sdev would require a more complicated udev rule
to handle it, since the logical action to take would be to perform a
rescan of the target (that's what the last component of the v4 patch was
designed to handle).

I'll change it to report the event on the sdev and post a v5 with what
I have now, including the other things you mentioned.  Comments welcome.

-Ewan

> 
> >  	NULL
> >  };
> >  
> > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > index 6efb2e1..0a79f0c 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -51,8 +51,16 @@ enum scsi_device_state {
> >  
> >  enum scsi_device_event {
> >  	SDEV_EVT_MEDIA_CHANGE	= 1,	/* media has changed */
> > +	SDEV_EVT_INQUIRY_CHANGE_REPORTED	= 2,	/* UA reported */
> > +	SDEV_EVT_CAPACITY_CHANGE_REPORTED	= 3,	/* UA reported */
> > +	SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED	= 4,  /* UA reported */
> > +	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED	= 5,	/* UA reported */
> > +
> > +	SDEV_EVT_LUN_CHANGE_REPORTED	= 6,	/* UA reported */
> 
> You don't need to explicitly number an enumeration.  The point is the
> compiler does it for you so we can't screw up and add something with the
> same number twice.
> 
> James
> 
> > +
> > +	SDEV_EVT_FIRST		= SDEV_EVT_MEDIA_CHANGE,
> > +	SDEV_EVT_LAST		= SDEV_EVT_LUN_CHANGE_REPORTED,
> >  
> > -	SDEV_EVT_LAST		= SDEV_EVT_MEDIA_CHANGE,
> >  	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
> >  };
> >  
> > @@ -154,6 +162,7 @@ struct scsi_device {
> >  	unsigned is_visible:1;	/* is the device visible in sysfs */
> >  
> >  	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
> > +	DECLARE_BITMAP(pending_events, SDEV_EVT_MAXBITS); /* pending events */
> >  	struct list_head event_list;	/* asserted events */
> >  	struct work_struct event_work;
> >  
> 
> 
> 

Re: [PATCH v4 08/10] scsi: Generate uevents on certain unit attention codes

[James Bottomley]

On Thu, 2013-08-08 at 12:08 -0400, Ewan Milne wrote:
> On Fri, 2013-08-02 at 10:06 -0700, James Bottomley wrote:
> > On Thu, 2013-08-01 at 16:57 -0400, Ewan D. Milne wrote:
> > > From: "Ewan D. Milne" <emilne@redhat.com>
> > > + * scsi_report_lun_change - Set flag on all *other* devices on the same target
> > > + *                          to indicate that a UNIT ATTENTION is expected.
> > > + *                          Only do this for SPC-3 devices, however.
> > > + * @sdev:	Device reporting the UNIT ATTENTION
> > > + */
> > > +static void scsi_report_lun_change(struct scsi_device *sdev)
> > > +{
> > > +	struct Scsi_Host *shost = sdev->host;
> > > +	struct scsi_device *tmp_sdev;
> > > +
> > > +	if (sdev->scsi_level == SCSI_SPC_3)
> > 
> > Why the SPC3 check?  We have SPC2 targets that use report luns and
> > presumably work as well.
> 
> Really the check was for SPC-4 and above, which I believe only generates
> a single REPORT LUNS DATA HAS CHANGED unit attention on the first LUN
> accessed.  This was described in T10 06-411r2.  I think it is still
> needed but should be changed to <= SCSI_SPC_3.

Perhaps the check needs to go then.  Just because SPC-4 says one event
per target doesn't mean devices will obey it.  There's no harm if we
guard against repeats even for SPC-4 and above (if they don't do it,
it's just a few extra setting and unsetting of flags) and we'll just
have to remove it again when the first broken device shows up.

> > 
> > > +		shost_for_each_device(tmp_sdev, shost) {
> > > +			if (tmp_sdev->channel == sdev->channel &&
> > > +			    tmp_sdev->id == sdev->id &&
> > > +			    tmp_sdev != sdev)
> > 
> > This should be starget_for_each_device calling a function rather than
> > hand rolling.
> > 
> > > +				tmp_sdev->expecting_cc_ua = 1;
> > 
> > Even with a restricted target loop, this is a bit messy (I think it's my
> > fault: I did ask you to reuse the existing mechanism, but now I see it,
> > a separate mechanism that functions the same way on the target looks a
> > lot cleaner) .  It looks like a struct scsi_target
> > expecting_lun_change:1 flag would work better in this case?  You'd set
> > it in the target at first sight, check it in scsi_report_sense() and
> > clear it on successful REPORT_LUNS command.  There's no need to lock it
> > because operations on a u32 are atomic and we're going to get slop
> > around this and the possibility of extra events anyway.
> 
> I've tried this out, and it seems to work OK, but I'm a little concerned
> about the possibility of losing a subsequent notification.  If a flag is
> used on the target, then multiple 3F 0E UAs from the same device could
> be suppressed, which isn't exactly the correct behavior.  Each LUN is
> only supposed to report 3F 0E once in SPC-3, per change.  If we receive
> another one, then there presumably has been *another* change.
> 
> Perhaps I'm being too picky about this, and I'm not at all sure that
> every storage array has this implemented properly either, so maybe it
> doesn't matter enough.

Well, you're not thinking about it the right way. Even on the per sdev
flag, we don't clear until a successful GOOD status return, so it will
suppress multiple genuine UAs anyway, assuming the lun data changes are
done fast enough.

>From a systems point of view, we receive a UA, trigger and event and
then some user programme tries to work out what changed by issuing a
REPORT LUNS and we suppress all UAs for LUN data changes until the
REPORT LUNS comes down.  Even if we get multiple genuine LUN data
changes in that interval, the user programme is going to read the latest
up to date information anyway, so it doesn't matter in the slightest
that we can lose genuine events.

The actual problem with the new scheme is the reverse: that it doesn't
suppress all the spurious events: given a target with three luns, two of
which are in constant use and one of which in sporadic, we'll suppress
the UAs from the LUNs in constant use, but if the sporadic LUN receives
no I/O until after the REPORT LUNS comes down, then it will trigger
another UA for an event we've already processed.  I think that's a price
worth paying for simplicity, though.

> > > --- a/drivers/scsi/scsi_sysfs.c
> > > +++ b/drivers/scsi/scsi_sysfs.c
> > > @@ -710,6 +710,11 @@ 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(inquiry_change_reported, INQUIRY_CHANGE_REPORTED)
> > > +DECLARE_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
> > > +DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
> > > +DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
> > > +DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
> > >  
> > >  /* Default template for device attributes.  May NOT be modified */
> > >  static struct attribute *scsi_sdev_attrs[] = {
> > > @@ -729,6 +734,11 @@ static struct attribute *scsi_sdev_attrs[] = {
> > >  	&dev_attr_ioerr_cnt.attr,
> > >  	&dev_attr_modalias.attr,
> > >  	REF_EVT(media_change),
> > > +	REF_EVT(inquiry_change_reported),
> > > +	REF_EVT(capacity_change_reported),
> > > +	REF_EVT(soft_threshold_reached),
> > > +	REF_EVT(mode_parameter_change_reported),
> > > +	REF_EVT(lun_change_reported),
> > 
> > This last one should be a property of the target attributes, shouldn't
> > it?  Otherwise the listener receives and event on the target and has to
> > go fishing around trying to find the one LUN we set the flag on.
> > 
> > Either we keep the flag on the sdev and send the event from the sdev or
> > we move the flag to the target and send the event from the target.
> 
> I could go either way on this -- conceptually it seems like the event
> should be reported on the scsi_target object, since the accessible LUNs
> on the target is what has actually changed on the storage, the LUN is
> just the messenger.  However, implementing this required adding a bunch
> of infrastructure for the uevent on the scsi_target object, which made
> the patch bigger, so I took that out in the v4 version.
> 
> Sending the event to the sdev would require a more complicated udev rule
> to handle it, since the logical action to take would be to perform a
> rescan of the target (that's what the last component of the v4 patch was
> designed to handle).
> 
> I'll change it to report the event on the sdev and post a v5 with what
> I have now, including the other things you mentioned.  Comments welcome.

OK, I'm fine either way, so I think we're good to go.

James

Re: [PATCH v4 08/10] scsi: Generate uevents on certain unit attention codes

[Ewan Milne]

On Thu, 2013-08-08 at 18:15 -0700, James Bottomley wrote:
> On Thu, 2013-08-08 at 12:08 -0400, Ewan Milne wrote:
> > On Fri, 2013-08-02 at 10:06 -0700, James Bottomley wrote:
> > > On Thu, 2013-08-01 at 16:57 -0400, Ewan D. Milne wrote:
> > > > From: "Ewan D. Milne" <emilne@redhat.com>
> > > > + * scsi_report_lun_change - Set flag on all *other* devices on the same target
> > > > + *                          to indicate that a UNIT ATTENTION is expected.
> > > > + *                          Only do this for SPC-3 devices, however.
> > > > + * @sdev:	Device reporting the UNIT ATTENTION
> > > > + */
> > > > +static void scsi_report_lun_change(struct scsi_device *sdev)
> > > > +{
> > > > +	struct Scsi_Host *shost = sdev->host;
> > > > +	struct scsi_device *tmp_sdev;
> > > > +
> > > > +	if (sdev->scsi_level == SCSI_SPC_3)
> > > 
> > > Why the SPC3 check?  We have SPC2 targets that use report luns and
> > > presumably work as well.
> > 
> > Really the check was for SPC-4 and above, which I believe only generates
> > a single REPORT LUNS DATA HAS CHANGED unit attention on the first LUN
> > accessed.  This was described in T10 06-411r2.  I think it is still
> > needed but should be changed to <= SCSI_SPC_3.
> 
> Perhaps the check needs to go then.  Just because SPC-4 says one event
> per target doesn't mean devices will obey it.  There's no harm if we
> guard against repeats even for SPC-4 and above (if they don't do it,
> it's just a few extra setting and unsetting of flags) and we'll just
> have to remove it again when the first broken device shows up.
> > > 
> > > > +		shost_for_each_device(tmp_sdev, shost) {
> > > > +			if (tmp_sdev->channel == sdev->channel &&
> > > > +			    tmp_sdev->id == sdev->id &&
> > > > +			    tmp_sdev != sdev)
> > > 
> > > This should be starget_for_each_device calling a function rather than
> > > hand rolling.
> > > 
> > > > +				tmp_sdev->expecting_cc_ua = 1;
> > > 
> > > Even with a restricted target loop, this is a bit messy (I think it's my
> > > fault: I did ask you to reuse the existing mechanism, but now I see it,
> > > a separate mechanism that functions the same way on the target looks a
> > > lot cleaner) .  It looks like a struct scsi_target
> > > expecting_lun_change:1 flag would work better in this case?  You'd set
> > > it in the target at first sight, check it in scsi_report_sense() and
> > > clear it on successful REPORT_LUNS command.  There's no need to lock it
> > > because operations on a u32 are atomic and we're going to get slop
> > > around this and the possibility of extra events anyway.
> > 
> > I've tried this out, and it seems to work OK, but I'm a little concerned
> > about the possibility of losing a subsequent notification.  If a flag is
> > used on the target, then multiple 3F 0E UAs from the same device could
> > be suppressed, which isn't exactly the correct behavior.  Each LUN is
> > only supposed to report 3F 0E once in SPC-3, per change.  If we receive
> > another one, then there presumably has been *another* change.
> > 
> > Perhaps I'm being too picky about this, and I'm not at all sure that
> > every storage array has this implemented properly either, so maybe it
> > doesn't matter enough.
> 
> Well, you're not thinking about it the right way. Even on the per sdev
> flag, we don't clear until a successful GOOD status return, so it will
> suppress multiple genuine UAs anyway, assuming the lun data changes are
> done fast enough.
> 
> From a systems point of view, we receive a UA, trigger and event and
> then some user programme tries to work out what changed by issuing a
> REPORT LUNS and we suppress all UAs for LUN data changes until the
> REPORT LUNS comes down.  Even if we get multiple genuine LUN data
> changes in that interval, the user programme is going to read the latest
> up to date information anyway, so it doesn't matter in the slightest
> that we can lose genuine events.
> 
> The actual problem with the new scheme is the reverse: that it doesn't
> suppress all the spurious events: given a target with three luns, two of
> which are in constant use and one of which in sporadic, we'll suppress
> the UAs from the LUNs in constant use, but if the sporadic LUN receives
> no I/O until after the REPORT LUNS comes down, then it will trigger
> another UA for an event we've already processed.  I think that's a price
> worth paying for simplicity, though.

I think that is OK, actually.  The REPORT LUNS is supposed to clear the
pending condition on all the LUNs that haven't yet reported the UA.

> 
> > > > --- a/drivers/scsi/scsi_sysfs.c
> > > > +++ b/drivers/scsi/scsi_sysfs.c
> > > > @@ -710,6 +710,11 @@ 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(inquiry_change_reported, INQUIRY_CHANGE_REPORTED)
> > > > +DECLARE_EVT(capacity_change_reported, CAPACITY_CHANGE_REPORTED)
> > > > +DECLARE_EVT(soft_threshold_reached, SOFT_THRESHOLD_REACHED_REPORTED)
> > > > +DECLARE_EVT(mode_parameter_change_reported, MODE_PARAMETER_CHANGE_REPORTED)
> > > > +DECLARE_EVT(lun_change_reported, LUN_CHANGE_REPORTED)
> > > >  
> > > >  /* Default template for device attributes.  May NOT be modified */
> > > >  static struct attribute *scsi_sdev_attrs[] = {
> > > > @@ -729,6 +734,11 @@ static struct attribute *scsi_sdev_attrs[] = {
> > > >  	&dev_attr_ioerr_cnt.attr,
> > > >  	&dev_attr_modalias.attr,
> > > >  	REF_EVT(media_change),
> > > > +	REF_EVT(inquiry_change_reported),
> > > > +	REF_EVT(capacity_change_reported),
> > > > +	REF_EVT(soft_threshold_reached),
> > > > +	REF_EVT(mode_parameter_change_reported),
> > > > +	REF_EVT(lun_change_reported),
> > > 
> > > This last one should be a property of the target attributes, shouldn't
> > > it?  Otherwise the listener receives and event on the target and has to
> > > go fishing around trying to find the one LUN we set the flag on.
> > > 
> > > Either we keep the flag on the sdev and send the event from the sdev or
> > > we move the flag to the target and send the event from the target.
> > 
> > I could go either way on this -- conceptually it seems like the event
> > should be reported on the scsi_target object, since the accessible LUNs
> > on the target is what has actually changed on the storage, the LUN is
> > just the messenger.  However, implementing this required adding a bunch
> > of infrastructure for the uevent on the scsi_target object, which made
> > the patch bigger, so I took that out in the v4 version.
> > 
> > Sending the event to the sdev would require a more complicated udev rule
> > to handle it, since the logical action to take would be to perform a
> > rescan of the target (that's what the last component of the v4 patch was
> > designed to handle).
> > 
> > I'll change it to report the event on the sdev and post a v5 with what
> > I have now, including the other things you mentioned.  Comments welcome.
> 
> OK, I'm fine either way, so I think we're good to go.

I sent v5 yesterday.  Let me know if you want a v6 with that SPC_3 test
removed. (I'll be out next week, if you want to just edit it, go ahead.)

> 
> 
> James
> 
> 

Re: [PATCH v4 08/10] scsi: Generate uevents on certain unit attention codes

[James Bottomley]

On Fri, 2013-08-09 at 10:50 -0400, Ewan Milne wrote:
> On Thu, 2013-08-08 at 18:15 -0700, James Bottomley wrote: 
> > OK, I'm fine either way, so I think we're good to go.
> 
> I sent v5 yesterday.  Let me know if you want a v6 with that SPC_3 test
> removed. (I'll be out next week, if you want to just edit it, go ahead.)

I'll just edit it.

James