Re: [PATCH 0/2] generate uevent for SCSI sense code

[Song Liu]

Dear Hannes and James, 

Could you please kindly review this patch and let me know what do we 
need to move forward with this?

Thanks and Regards,
Song


> On Aug 4, 2017, at 10:18 AM, Song Liu <songliubrav...@fb.com> wrote:
> 
> Hi all, 
> 
> Could you please share your feedback on this version of the change? 
> 
> Thanks in advance,
> Song
> 
>>> On 7/21/17, 3:58 PM, "Song Liu" <songliubrav...@fb.com> wrote:
> 
>This change is to follow up our discussion on event log for media
>management during LSF/MM 2017.
> 
>Changes from RFC v3:
>  Incorporate feedback by Johannes Thumshirn
> 
>Thanks,
>Song
> 
>Song Liu (2):
>  scsi: generate uevent for SCSI sense code
>  scsi: add rate limit to scsi sense code uevent
> 
> drivers/scsi/Kconfig   | 14 +++
> drivers/scsi/hosts.c   |  4 
> drivers/scsi/scsi_error.c  | 58 
> ++
> drivers/scsi/scsi_lib.c| 58 
> +-
> drivers/scsi/scsi_sysfs.c  | 51 
> include/scsi/scsi_common.h |  6 +
> include/scsi/scsi_device.h | 27 -
> include/scsi/scsi_host.h   | 13 +++
> 8 files changed, 229 insertions(+), 2 deletions(-)
> 
>--
>2.9.3
> 
> 

Re: [PATCH 0/2] generate uevent for SCSI sense code

[Song Liu]

Hi all, 

Could you please share your feedback on this version of the change? 
 
Thanks in advance,
Song

>> On 7/21/17, 3:58 PM, "Song Liu" <songliubrav...@fb.com> wrote:

This change is to follow up our discussion on event log for media
management during LSF/MM 2017.

Changes from RFC v3:
  Incorporate feedback by Johannes Thumshirn

Thanks,
    Song

Song Liu (2):
  scsi: generate uevent for SCSI sense code
  scsi: add rate limit to scsi sense code uevent

 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/hosts.c   |  4 
 drivers/scsi/scsi_error.c  | 58 
++
 drivers/scsi/scsi_lib.c| 58 
+-
 drivers/scsi/scsi_sysfs.c  | 51 
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 27 -
 include/scsi/scsi_host.h   | 13 +++
 8 files changed, 229 insertions(+), 2 deletions(-)

--
2.9.3

[PATCH 1/2] scsi: generate uevent for SCSI sense code

[Song Liu]

This patch adds capability for SCSI layer to generate uevent for SCSI
sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.

We can configure which sense keys generate uevent for each device
through sysfs entry sense_event_filter, which is a bitmap of
"sense keys to generate uevent" For example, the following enables
uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04) on scsi
drive sdc:

echo 0x000c > /sys/block/sdc/device/sense_event_filter

Here is an example output captured by udevadm:

KERNEL[587.353177] change   /devices/pci:00/XX
ACTION=change
CDB=\x88\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00
DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XX
DEVTYPE=scsi_device
DRIVER=sd
MODALIAS=scsi:t-0x00
SDEV_SENSE=1
SENSE_BUFFER=\x72\x03\x11\x14\x00\x00\x00\x34\x00\x0a\x80 
SENSE_CODE=3/11/14
SEQNUM=4796
SUBSYSTEM=scsi

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/scsi_error.c  | 42 +
 drivers/scsi/scsi_lib.c| 58 +-
 drivers/scsi/scsi_sysfs.c  | 51 
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 27 -
 6 files changed, 196 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index d384f4f..0fb672b 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -226,6 +226,20 @@ config SCSI_LOGGING
  there should be no noticeable performance impact as long as you have
  logging turned off.
 
+config SCSI_SENSE_UEVENT
+   bool "SCSI sense code logging"
+   depends on SCSI
+   default n
+   ---help---
+ This turns on uevent for SCSI sense code.
+
+ You can configure which sense keys generate uevent for each device
+ through sysfs entry sense_event_filter. For example, the following
+ enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
+ on scsi drive sdc:
+
+ echo 0x000c > /sys/block/sdc/device/sense_event_filter
+
 config SCSI_SCAN_ASYNC
bool "Asynchronous SCSI scanning"
depends on SCSI
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ea9f40e..f6107f7 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -426,6 +426,47 @@ static void scsi_report_sense(struct scsi_device *sdev,
}
 }
 
+/*
+ * generate uevent when receiving sense code from device
+ */
+static void scsi_send_sense_uevent(struct scsi_device *sdev,
+  struct scsi_cmnd *scmd,
+  struct scsi_sense_hdr *sshdr)
+{
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   struct scsi_event *evt;
+   unsigned char sb_len;
+
+   if (!test_bit(sshdr->sense_key, >sense_event_filter))
+   return;
+   evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
+   if (!evt)
+   return;
+
+   evt->sense_evt_data.cmnd = kzalloc(scmd->cmd_len, GFP_ATOMIC);
+   if (!evt->sense_evt_data.cmnd)
+   goto alloc_cmd_fail;
+
+   sb_len = scsi_sense_data_length(scmd->sense_buffer);
+
+   evt->sense_evt_data.sense_buffer = kzalloc(sb_len, GFP_ATOMIC);
+   if (!evt->sense_evt_data.sense_buffer)
+   goto alloc_sense_fail;
+
+   evt->sense_evt_data.cmd_len = scmd->cmd_len;
+   evt->sense_evt_data.sb_len = sb_len;
+   memcpy(evt->sense_evt_data.cmnd, scmd->cmnd, scmd->cmd_len);
+   memcpy(evt->sense_evt_data.sense_buffer, scmd->sense_buffer, sb_len);
+
+   sdev_evt_send(sdev, evt);
+   return;
+alloc_sense_fail:
+   kfree(evt->sense_evt_data.cmnd);
+alloc_cmd_fail:
+   kfree(evt);
+#endif
+}
+
 /**
  * scsi_check_sense - Examine scsi cmd sense
  * @scmd:  Cmd to have sense checked.
@@ -446,6 +487,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
return FAILED;  /* no valid sense data */
 
scsi_report_sense(sdev, );
+   scsi_send_sense_uevent(sdev, scmd, );
 
if (scsi_sense_is_deferred())
return NEEDS_RETRY;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6097b8..7c6ab28 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2677,7 +2677,15 @@ 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[5];  /* SDEV_EVT_SCSI_SENSE needs most entries (4) */
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   char *buf = NULL;
+   int i;
+   int buf_size;
+   int offset;
+   struct scsi_sense_hdr sshdr;
+#endif
+
 
switch (evt->evt_type) {
case SDEV_EVT_MEDIA_CHANGE:
@@ -2702,6 +2710,49 @@ static void scsi_evt_emit(str

[PATCH 0/2] generate uevent for SCSI sense code

[Song Liu]

This change is to follow up our discussion on event log for media
management during LSF/MM 2017.

Changes from RFC v3:
  Incorporate feedback by Johannes Thumshirn

Thanks,
Song

Song Liu (2):
  scsi: generate uevent for SCSI sense code
  scsi: add rate limit to scsi sense code uevent

 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/hosts.c   |  4 
 drivers/scsi/scsi_error.c  | 58 ++
 drivers/scsi/scsi_lib.c| 58 +-
 drivers/scsi/scsi_sysfs.c  | 51 
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 27 -
 include/scsi/scsi_host.h   | 13 +++
 8 files changed, 229 insertions(+), 2 deletions(-)

--
2.9.3

[PATCH 2/2] scsi: add rate limit to scsi sense code uevent

[Song Liu]

This patch adds rate limits to SCSI sense code uevets. Currently,
the rate limit is hard-coded to 64 events per second per Scsi_Host.

The code tracks nano second time of latest 64 events in a circular
buffer. When a new event arrives, the time is compared against the
latest time in the buffer. If the difference is smaller than one
second, the new event is dropped.

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/hosts.c  |  4 
 drivers/scsi/scsi_error.c | 16 
 include/scsi/scsi_host.h  | 13 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 831a1c8..219481b 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -408,6 +408,10 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(>host_wait);
mutex_init(>scan_mutex);
 
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   spin_lock_init(>latest_event_lock);
+#endif
+
index = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
if (index < 0)
goto fail_kfree;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f6107f7..58047f9 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -436,9 +436,25 @@ static void scsi_send_sense_uevent(struct scsi_device 
*sdev,
 #ifdef CONFIG_SCSI_SENSE_UEVENT
struct scsi_event *evt;
unsigned char sb_len;
+   struct Scsi_Host *shost = sdev->host;
+   unsigned long flags;
+   u64 time_ns;
 
if (!test_bit(sshdr->sense_key, >sense_event_filter))
return;
+
+   time_ns = ktime_to_ns(ktime_get());
+   spin_lock_irqsave(>latest_event_lock, flags);
+   if (time_ns - shost->latest_event_times[shost->latest_event_idx] <
+   NSEC_PER_SEC) {
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+   return;
+   }
+   shost->latest_event_times[shost->latest_event_idx] = time_ns;
+   shost->latest_event_idx = (shost->latest_event_idx + 1) %
+   MAX_SENSE_EVENT_PER_SECOND;
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+
evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
if (!evt)
return;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index afb0481..8aacb15 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -731,6 +731,19 @@ struct Scsi_Host {
 */
struct device *dma_dev;
 
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+#define MAX_SENSE_EVENT_PER_SECOND 64
+   /*
+* To rate limit uevents to MAX_SENSE_EVENT_PER_SECOND, we track
+* nano second time of MAX_SENSE_EVENT_PER_SECOND most recent
+* events. If there are already MAX_SENSE_EVENT_PER_SECOND in the
+* past seconds, new event is dropped.
+*/
+   u64 latest_event_times[MAX_SENSE_EVENT_PER_SECOND];
+   int latest_event_idx;
+   spinlock_t  latest_event_lock;
+#endif
+
/*
 * We should ensure that this is aligned, both for better performance
 * and also because some compilers (m68k) don't automatically force
-- 
2.9.3

[RFC v3 1/2] scsi: generate uevent for SCSI sense code

[Song Liu]

This patch adds capability for SCSI layer to generate uevent for SCSI
sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.

We can configure which sense keys generate uevent for each device
through sysfs entry sense_event_filter, which is a bitmap of
"sense keys to generate uevent" For example, the following enables
uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04) on scsi
drive sdc:

echo 0x000c > /sys/block/sdc/device/sense_event_filter

Here is an example output captured by udevadm:

KERNEL[587.353177] change   /devices/pci:00/XX
ACTION=change
CDB=\x88\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00
DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XX
DEVTYPE=scsi_device
DRIVER=sd
MODALIAS=scsi:t-0x00
SDEV_SENSE=1
SENSE_BUFFER=\x72\x03\x11\x14\x00\x00\x00\x34\x00\x0a\x80 
SENSE_CODE=3/11/14
SEQNUM=4796
SUBSYSTEM=scsi

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/scsi_error.c  | 43 ++
 drivers/scsi/scsi_lib.c| 58 +-
 drivers/scsi/scsi_sysfs.c  | 51 
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 27 -
 6 files changed, 197 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index d384f4f..0fb672b 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -226,6 +226,20 @@ config SCSI_LOGGING
  there should be no noticeable performance impact as long as you have
  logging turned off.
 
+config SCSI_SENSE_UEVENT
+   bool "SCSI sense code logging"
+   depends on SCSI
+   default n
+   ---help---
+ This turns on uevent for SCSI sense code.
+
+ You can configure which sense keys generate uevent for each device
+ through sysfs entry sense_event_filter. For example, the following
+ enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
+ on scsi drive sdc:
+
+ echo 0x000c > /sys/block/sdc/device/sense_event_filter
+
 config SCSI_SCAN_ASYNC
bool "Asynchronous SCSI scanning"
depends on SCSI
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ac31964..b8ef869 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -426,6 +426,48 @@ static void scsi_report_sense(struct scsi_device *sdev,
}
 }
 
+/*
+ * generate uevent when receiving sense code from device
+ */
+static void scsi_send_sense_uevent(struct scsi_device *sdev,
+  struct scsi_cmnd *scmd,
+  struct scsi_sense_hdr *sshdr)
+{
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   struct scsi_event *evt;
+   unsigned char sb_len;
+
+   if (!test_bit(sshdr->sense_key & 0xf,
+ >sense_event_filter))
+   return;
+   evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
+   if (!evt)
+   return;
+
+   evt->sense_evt_data.cmnd = kzalloc(scmd->cmd_len, GFP_ATOMIC);
+   if (!evt->sense_evt_data.cmnd)
+   goto alloc_cmd_fail;
+
+   sb_len = scsi_sense_data_length(scmd->sense_buffer);
+
+   evt->sense_evt_data.sense_buffer = kzalloc(sb_len, GFP_ATOMIC);
+   if (!evt->sense_evt_data.sense_buffer)
+   goto alloc_sense_fail;
+
+   evt->sense_evt_data.cmd_len = scmd->cmd_len;
+   evt->sense_evt_data.sb_len = sb_len;
+   memcpy(evt->sense_evt_data.cmnd, scmd->cmnd, scmd->cmd_len);
+   memcpy(evt->sense_evt_data.sense_buffer, scmd->sense_buffer, sb_len);
+
+   sdev_evt_send(sdev, evt);
+   return;
+alloc_sense_fail:
+   kfree(evt->sense_evt_data.cmnd);
+alloc_cmd_fail:
+   kfree(evt);
+#endif
+}
+
 /**
  * scsi_check_sense - Examine scsi cmd sense
  * @scmd:  Cmd to have sense checked.
@@ -446,6 +488,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
return FAILED;  /* no valid sense data */
 
scsi_report_sense(sdev, );
+   scsi_send_sense_uevent(sdev, scmd, );
 
if (scsi_sense_is_deferred())
return NEEDS_RETRY;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 41c19c7..67cc0fb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2677,7 +2677,15 @@ 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[5];  /* SDEV_EVT_SCSI_SENSE needs most entries (4) */
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   char *buf = NULL;
+   int i;
+   int buf_size;
+   int offset;
+   struct scsi_sense_hdr sshdr;
+#endif
+
 
switch (evt->evt_type) {
case SDEV_EVT_MEDIA_CHANGE:
@@ -2702,6 +

[RFC v3 0/2] generate uevent for SCSI sense code

[Song Liu]

This change is to follow up our discussion on event log for media
management during LSF/MM 2017.

Changes from RFC v2:
1. change rate limit of events from per device to per SCSI host
2. fix bugs from feedbacks

Please kindly let me know your thoughts on this.

Thanks,
Song

Song Liu (2):
  scsi: generate uevent for SCSI sense code
  scsi: add rate limit to scsi sense code uevent

 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/hosts.c   |  4 
 drivers/scsi/scsi_error.c  | 59 ++
 drivers/scsi/scsi_lib.c| 58 -
 drivers/scsi/scsi_sysfs.c  | 51 +++
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 27 -
 include/scsi/scsi_host.h   | 13 ++
 8 files changed, 230 insertions(+), 2 deletions(-)

--
2.9.3

[RFC v3 2/2] scsi: add rate limit to scsi sense code uevent

[Song Liu]

This patch adds rate limits to SCSI sense code uevets. Currently,
the rate limit is hard-coded to 64 events per second per Scsi_Host.

The code tracks nano second time of latest 64 events in a circular
buffer. When a new event arrives, the time is compared against the
latest time in the buffer. If the difference is smaller than one
second, the new event is dropped.

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/hosts.c  |  4 
 drivers/scsi/scsi_error.c | 16 
 include/scsi/scsi_host.h  | 13 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 831a1c8..219481b 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -408,6 +408,10 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(>host_wait);
mutex_init(>scan_mutex);
 
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   spin_lock_init(>latest_event_lock);
+#endif
+
index = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
if (index < 0)
goto fail_kfree;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index b8ef869..a0f5aab 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -436,10 +436,26 @@ static void scsi_send_sense_uevent(struct scsi_device 
*sdev,
 #ifdef CONFIG_SCSI_SENSE_UEVENT
struct scsi_event *evt;
unsigned char sb_len;
+   struct Scsi_Host *shost = sdev->host;
+   unsigned long flags;
+   u64 time_ns;
 
if (!test_bit(sshdr->sense_key & 0xf,
  >sense_event_filter))
return;
+
+   time_ns = ktime_to_ns(ktime_get());
+   spin_lock_irqsave(>latest_event_lock, flags);
+   if (time_ns - shost->latest_event_times[shost->latest_event_idx] <
+   NSEC_PER_SEC) {
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+   return;
+   }
+   shost->latest_event_times[shost->latest_event_idx] = time_ns;
+   shost->latest_event_idx = (shost->latest_event_idx + 1) %
+   MAX_SENSE_EVENT_PER_SECOND;
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+
evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
if (!evt)
return;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index afb0481..8aacb15 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -731,6 +731,19 @@ struct Scsi_Host {
 */
struct device *dma_dev;
 
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+#define MAX_SENSE_EVENT_PER_SECOND 64
+   /*
+* To rate limit uevents to MAX_SENSE_EVENT_PER_SECOND, we track
+* nano second time of MAX_SENSE_EVENT_PER_SECOND most recent
+* events. If there are already MAX_SENSE_EVENT_PER_SECOND in the
+* past seconds, new event is dropped.
+*/
+   u64 latest_event_times[MAX_SENSE_EVENT_PER_SECOND];
+   int latest_event_idx;
+   spinlock_t  latest_event_lock;
+#endif
+
/*
 * We should ensure that this is aligned, both for better performance
 * and also because some compilers (m68k) don't automatically force
-- 
2.9.3

Re: [RFC v2 2/2] scsi: add rate limit to scsi sense code uevent

[Song Liu]

> On Jun 6, 2017, at 6:22 AM, Ewan D. Milne <emi...@redhat.com> wrote:
> 
> On Thu, 2017-05-18 at 11:14 -0700, Song Liu wrote:
>> This patch adds rate limits to SCSI sense code uevets. Currently,
>> the rate limit is hard-coded to 16 events per second.
>> 
>> The code tracks nano second time of latest 16 events in a circular
>> buffer. When a new event arrives, the time is compared against the
>> latest time in the buffer. If the difference is smaller than one
>> second, the new event is dropped.
>> 
>> Signed-off-by: Song Liu <songliubrav...@fb.com>
>> ---
>> drivers/scsi/scsi_error.c  | 15 +++
>> drivers/scsi/scsi_scan.c   |  4 
>> include/scsi/scsi_device.h | 12 +++-
>> 3 files changed, 30 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index a49c63a..91e6b0a 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -436,11 +436,26 @@ static void scsi_send_sense_uevent(struct scsi_device 
>> *sdev,
>> #ifdef CONFIG_SCSI_SENSE_UEVENT
>>  struct scsi_event *evt;
>>  unsigned char sb_len;
>> +unsigned long flags;
>> +u64 time_ns;
>> 
>>  if (!test_bit(sshdr->sense_key & 0xf,
>>>sense_event_filter))
>>  return;
>>  evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
>> +
>> +time_ns = ktime_to_ns(ktime_get());
>> +spin_lock_irqsave(>latest_event_lock, flags);
>> +if (time_ns - sdev->latest_event_times[sdev->latest_event_idx] <
>> +NSEC_PER_SEC) {
>> +spin_unlock_irqrestore(>latest_event_lock, flags);
>> +return;
> 
> You have an allocated evt here, so if you return here you will leak
> kernel memory.

I will fix this in next version. 

> 
> This code appears to be rate limiting per-sdev.  You have to remember
> that on a large system, there could be thousands of these devices.
> You could easily end up generating 10s or 100s of thousands of events
> per second, in a very short amount of time under certain failure
> conditions.
> 
> The udev event mechanism can realistically only process a few
> hundred events per second before it starts getting behind, due to
> the rule processing engine (the rules in userspace).
> 
> You also have to consider that if you clog up udev with a lot of
> your events, you affect event processing for other events.
> 

Yeah, this is great point. Would Scsi_Host level rate limiting make
sense for this type of cases? Or shall I add a global rate limit?

Thanks,
Song

Re: [RFC v2 0/2] generate uevent for SCSI sense code

[Song Liu]

> On May 18, 2017, at 11:14 AM, Song Liu <songliubrav...@fb.com> wrote:
> 
> This change is to follow up our discussion on event log for media
> management during LSF/MM 2017.
> 
> I have included feedbacks from Hannes Reinecke, Ewan D. Milne, and
> Benjamin Block.
> 
> Please kindly let me know your thoughts on this.
> 
> Thanks,
> Song
> 
> Song Liu (2):
>  scsi: generate uevent for SCSI sense code
>  scsi: add rate limit to scsi sense code uevent
> 
> drivers/scsi/Kconfig   | 14 +++
> drivers/scsi/scsi_error.c  | 58 ++
> drivers/scsi/scsi_lib.c| 58 +-
> drivers/scsi/scsi_scan.c   |  4 
> drivers/scsi/scsi_sysfs.c  | 51 
> include/scsi/scsi_common.h |  6 +
> include/scsi/scsi_device.h | 37 -
> 7 files changed, 226 insertions(+), 2 deletions(-)
> 
> --
> 2.9.3

Dear Hannes, James, Benjamin, and Ewan, 

Could you please share your feedback on this version of the RFC? Does it 
need major changes?

Thanks,
Song

[RFC v2 2/2] scsi: add rate limit to scsi sense code uevent

[Song Liu]

This patch adds rate limits to SCSI sense code uevets. Currently,
the rate limit is hard-coded to 16 events per second.

The code tracks nano second time of latest 16 events in a circular
buffer. When a new event arrives, the time is compared against the
latest time in the buffer. If the difference is smaller than one
second, the new event is dropped.

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/scsi_error.c  | 15 +++
 drivers/scsi/scsi_scan.c   |  4 
 include/scsi/scsi_device.h | 12 +++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a49c63a..91e6b0a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -436,11 +436,26 @@ static void scsi_send_sense_uevent(struct scsi_device 
*sdev,
 #ifdef CONFIG_SCSI_SENSE_UEVENT
struct scsi_event *evt;
unsigned char sb_len;
+   unsigned long flags;
+   u64 time_ns;
 
if (!test_bit(sshdr->sense_key & 0xf,
  >sense_event_filter))
return;
evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
+
+   time_ns = ktime_to_ns(ktime_get());
+   spin_lock_irqsave(>latest_event_lock, flags);
+   if (time_ns - sdev->latest_event_times[sdev->latest_event_idx] <
+   NSEC_PER_SEC) {
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+   return;
+   }
+   sdev->latest_event_times[sdev->latest_event_idx] = time_ns;
+   sdev->latest_event_idx = (sdev->latest_event_idx + 1) %
+   MAX_SENSE_EVENT_PER_SECOND;
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+
if (!evt)
return;
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f..67b3f71 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -301,6 +301,10 @@ static struct scsi_device *scsi_alloc_sdev(struct 
scsi_target *starget,
}
}
 
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   spin_lock_init(>latest_event_lock);
+#endif
+
return sdev;
 
 out_device_destroy:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5b6f06d..9217dae 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -214,12 +214,22 @@ struct scsi_device {
atomic_t ioerr_cnt;
 
 #ifdef CONFIG_SCSI_SENSE_UEVENT
+#define MAX_SENSE_EVENT_PER_SECOND 16
/*
 * filter of sense code uevent
 * setting bit X (0x00 - 0x0e) of sense_event_filter enables
 * uevent for sense key X
 */
-   unsigned long   sense_event_filter;
+   unsigned long   sense_event_filter;
+   /*
+* To rate limit uevents to MAX_SENSE_EVENT_PER_SECOND, we track
+* nano second time of MAX_SENSE_EVENT_PER_SECOND most recent
+* events. If there are already MAX_SENSE_EVENT_PER_SECOND in the
+* past seconds, new event is dropped.
+*/
+   u64 latest_event_times[MAX_SENSE_EVENT_PER_SECOND];
+   int latest_event_idx;
+   spinlock_t  latest_event_lock;
 #endif
 
struct device   sdev_gendev,
-- 
2.9.3

[RFC v2 1/2] scsi: generate uevent for SCSI sense code

[Song Liu]

This patch adds capability for SCSI layer to generate uevent for SCSI
sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.

We can configure which sense keys generate uevent for each device
through sysfs entry sense_event_filter, which is a bitmap of
"sense keys to generate uevent" For example, the following enables
uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04) on scsi
drive sdc:

echo 0x000c > /sys/block/sdc/device/sense_event_filter

Here is an example output captured by udevadm:

KERNEL[587.353177] change   /devices/pci:00/XX
ACTION=change
CDB=\x88\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00
DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XX
DEVTYPE=scsi_device
DRIVER=sd
MODALIAS=scsi:t-0x00
SDEV_SENSE=1
SENSE_BUFFER=\x72\x03\x11\x14\x00\x00\x00\x34\x00\x0a\x80 
SENSE_CODE=3/11/14
SEQNUM=4796
SUBSYSTEM=scsi

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/scsi_error.c  | 43 ++
 drivers/scsi/scsi_lib.c| 58 +-
 drivers/scsi/scsi_sysfs.c  | 51 
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 27 -
 6 files changed, 197 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 3c52867..fd25e14 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -237,6 +237,20 @@ config SCSI_LOGGING
  there should be no noticeable performance impact as long as you have
  logging turned off.
 
+config SCSI_SENSE_UEVENT
+   bool "SCSI sense code logging"
+   depends on SCSI
+   default n
+   ---help---
+ This turns on uevent for SCSI sense code.
+
+ You can configure which sense keys generate uevent for each device
+ through sysfs entry sense_event_filter. For example, the following
+ enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
+ on scsi drive sdc:
+
+ echo 0x000c > /sys/block/sdc/device/sense_event_filter
+
 config SCSI_SCAN_ASYNC
bool "Asynchronous SCSI scanning"
depends on SCSI
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ecc07da..a49c63a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -426,6 +426,48 @@ static void scsi_report_sense(struct scsi_device *sdev,
}
 }
 
+/*
+ * generate uevent when receiving sense code from device
+ */
+static void scsi_send_sense_uevent(struct scsi_device *sdev,
+  struct scsi_cmnd *scmd,
+  struct scsi_sense_hdr *sshdr)
+{
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   struct scsi_event *evt;
+   unsigned char sb_len;
+
+   if (!test_bit(sshdr->sense_key & 0xf,
+ >sense_event_filter))
+   return;
+   evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
+   if (!evt)
+   return;
+
+   evt->sense_evt_data.cmnd = kzalloc(scmd->cmd_len, GFP_ATOMIC);
+   if (!evt->sense_evt_data.cmnd)
+   goto alloc_cmd_fail;
+
+   sb_len = scsi_sense_data_length(scmd->sense_buffer);
+
+   evt->sense_evt_data.sense_buffer = kzalloc(sb_len, GFP_ATOMIC);
+   if (!evt->sense_evt_data.sense_buffer)
+   goto alloc_sense_fail;
+
+   evt->sense_evt_data.cmd_len = scmd->cmd_len;
+   evt->sense_evt_data.sb_len = sb_len;
+   memcpy(evt->sense_evt_data.cmnd, scmd->cmnd, scmd->cmd_len);
+   memcpy(evt->sense_evt_data.sense_buffer, scmd->sense_buffer, sb_len);
+
+   sdev_evt_send(sdev, evt);
+   return;
+alloc_sense_fail:
+   kfree(evt->sense_evt_data.cmnd);
+alloc_cmd_fail:
+   kfree(evt);
+#endif
+}
+
 /**
  * scsi_check_sense - Examine scsi cmd sense
  * @scmd:  Cmd to have sense checked.
@@ -446,6 +488,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
return FAILED;  /* no valid sense data */
 
scsi_report_sense(sdev, );
+   scsi_send_sense_uevent(sdev, scmd, );
 
if (scsi_sense_is_deferred())
return NEEDS_RETRY;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e31f1cc..37b3b7e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2660,7 +2660,15 @@ 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[5];  /* SDEV_EVT_SCSI_SENSE needs most entries (4) */
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   char *buf = NULL;
+   int i;
+   int buf_size;
+   int offset;
+   struct scsi_sense_hdr sshdr;
+#endif
+
 
switch (evt->evt_type) {
case SDEV_EVT_MEDIA_CHANGE:
@@ -2685,6 +

[RFC v2 0/2] generate uevent for SCSI sense code

[Song Liu]

This change is to follow up our discussion on event log for media
management during LSF/MM 2017.

I have included feedbacks from Hannes Reinecke, Ewan D. Milne, and
Benjamin Block.

Please kindly let me know your thoughts on this.

Thanks,
Song

Song Liu (2):
  scsi: generate uevent for SCSI sense code
  scsi: add rate limit to scsi sense code uevent

 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/scsi_error.c  | 58 ++
 drivers/scsi/scsi_lib.c| 58 +-
 drivers/scsi/scsi_scan.c   |  4 
 drivers/scsi/scsi_sysfs.c  | 51 
 include/scsi/scsi_common.h |  6 +
 include/scsi/scsi_device.h | 37 -
 7 files changed, 226 insertions(+), 2 deletions(-)

--
2.9.3

Re: [RFC] scsi: generate uevent for SCSI sense code

[Song Liu]

> On May 16, 2017, at 10:00 AM, Benjamin Block <bbl...@linux.vnet.ibm.com> 
> wrote:
> 
> Hello,
> 
> On Wed, May 03, 2017 at 05:50:13PM -0700, Song Liu wrote:
>> This patch adds capability for SCSI layer to generate uevent for SCSI
>> sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.
>> 
>> We can configure which sense keys generate uevent for each device
>> through sysfs entry sense_event_filter. For example, the following
>> enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
>> on scsi drive sdc:
>> 
>>echo 0x000c > /sys/block/sdc/device/sense_event_filter
>> 
> 
> I know its an RFC, but the user-interface definitely needs better
> documentation. You also don't mention the 'wildcard' you document in the
> code below.
> 
> Would there be any public tooling for this, when it gets in?
> 
> There is already some events we can generate.. for example when the
> host-mapping changed, but I am not aware of any tooling that would ever
> make use of them (although, this probably would even be vert useful).
> And in the end that seems like dead-code for me. But maybe thats just me
> not knowing the tooling.

Hi Benjamin, 

Thanks for these feedbacks. I will incorporate them into next version. 

In term of tooling, we will open source useful tools we build to consume
these events.  

>> 
>> Here is an example output captured by udevadm:
>> 
>> KERNEL[1214.945358] change   /devices/pci:00/XXX
>> ACTION=change
>> DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XXX
>> DEVTYPE=scsi_device
>> DRIVER=sd
>> LBA=0
>> MODALIAS=scsi:t-0x00
>> SDEV_UA=SCSI_SENSE
>> SENSE_CODE=3/11/14
>> SEQNUM=4536
>> SIZE=4096
>> SUBSYSTEM=scsi
>> 
>> Signed-off-by: Song Liu <songliubrav...@fb.com>
>> ---
>> drivers/scsi/Kconfig   | 14 +++
>> drivers/scsi/scsi_error.c  | 26 
>> drivers/scsi/scsi_lib.c| 27 +++--
>> drivers/scsi/scsi_sysfs.c  | 60 
>> ++
>> include/scsi/scsi_device.h | 26 +++-
>> 5 files changed, 150 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 3c52867..4f7f211 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -237,6 +237,20 @@ config SCSI_LOGGING
>>there should be no noticeable performance impact as long as you have
>>logging turned off.
>> 
>> +config SCSI_SENSE_UEVENT
>> +bool "SCSI sense code logging"
>> +depends on SCSI
>> +default n
>> +---help---
>> +  This turns on uevent for SCSI sense code.
>> +
>> +  You can configure which sense keys generate uevent for each device
>> +  through sysfs entry sense_event_filter. For example, the following
>> +  enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
>> +  on scsi drive sdc:
>> +
>> +  echo 0x000c > /sys/block/sdc/device/sense_event_filter
>> +
>> config SCSI_SCAN_ASYNC
>>  bool "Asynchronous SCSI scanning"
>>  depends on SCSI
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index d70c67c..eda150e 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -426,6 +426,31 @@ static void scsi_report_sense(struct scsi_device *sdev,
>>  }
>> }
>> 
>> +/*
>> + * generate uevent when receiving sense code from device
>> + */
>> +static void scsi_send_sense_uevent(struct scsi_device *sdev,
>> +   struct scsi_cmnd *scmd,
>> +   struct scsi_sense_hdr *sshdr)
>> +{
>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>> +struct scsi_event *evt;
>> +
>> +if (!test_bit(sshdr->sense_key & 0xf,
>> +  >sense_event_filter))
>> +return;
>> +evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
>> +if (!evt)
>> +return;
>> +
>> +evt->sense_evt_data.lba = scsi_get_lba(scmd);
>> +evt->sense_evt_data.size = blk_rq_bytes(scmd->request);
>> +memcpy(>sense_evt_data.sshdr, sshdr,
>> +   sizeof(struct scsi_sense_hdr));
>> +sdev_evt_send(sdev, evt);
>> +#endif
>> +}
>> +
> 
> So when I turn this CONFIG_ off, do I get all kinds of warning about
> unused variables?

I don't get any warning at this time. I will keep testing w/ and w/o
the config enabled. 

Thanks,
Song

Re: [RFC] scsi: generate uevent for SCSI sense code

[Song Liu]

> On May 15, 2017, at 5:31 AM, Ewan D. Milne  wrote:
> 
> On Mon, 2017-05-15 at 08:04 +0200, Hannes Reinecke wrote:
>> In general I like the idea; however, the 'filter' thingie is somewhat
>> odd. I could somewhat buy into the idea of filtering for sense keys, but
>> then I would have expected to use the 'sense_event_filter' as a bitmap
>> of allowed sense keys.
>> With your current design you can only filter for one specific sense key,
>> _and_ you leave the remaining bits of the sense_event_filter variable
>> unused.
>> So either turn it into a bitmap and allow for several sense keys to be
>> filtered, or treat it as mask for the _entire_ sense, but then you'd
>> need for ASC and ASCQ, too.
>> 
>> Cheers,
>> 
>> Hannes
> 
> I think what would help here is understanding what you are trying to
> accomplish with this change.  Are you trying to allow generation of
> a uevent on a particular sense KCQ combination you know in advance,
> or a set of them, or do you want to be able to normally generate
> uevents for every sense keys/codes (i.e. all the bitmap bits set)
> and then perhaps narrow down to a specific one when you are looking
> for a problem?

Thanks for these feedbacks!

My goal here is to generate uevent for a set of sense keys. I would 
like to store these information for a fleet of many HDDs over a 
couple years. We are hoping to use these information to get insights
in the quality of the HDDs. 

> When I added the sense code uevent generation for Unit Attentions,
> one big concern I had was how many events per second would be generated.
> The userspace event handling is very slow, and can only handle a few
> hundred events per second before it starts backing up, and then you
> can lose events, which you don't want.

Thanks for this information. I am trying to limit the rate with the 
filter. But I am not sure whether this is sufficient. 

> Generating a uevent on MEDIUM ERROR is somewhat worrying, when a drive
> goes bad you could easily get these on every I/O.

MEDIUM ERROR is an important event that we would like to keep. Maybe 
we need better mechanism for rate limit, for example, no more than 10 
MEDIUM ERROR per second per drive. 

> 
> Your actual uevent emit:
> 
> +#ifdef CONFIG_SCSI_SENSE_UEVENT
> +   case SDEV_EVT_SCSI_SENSE:
> +   envp[idx++] = "SDEV_UA=SCSI_SENSE";
> +   for (i = idx; i < idx + 3; ++i) {
> +   envp[i] = kzalloc(32, GFP_ATOMIC);
> +   if (!envp[i])
> +   break;
> +   free_envp = i;
> +   }
> +   snprintf(envp[idx++], 32, "LBA=%lu", evt->sense_evt_data.lba);
> +   snprintf(envp[idx++], 32, "SIZE=%d", 
> evt->sense_evt_data.size);
> +   snprintf(envp[idx++], 32, "SENSE_CODE=%1x/%02x/%02x",
> +evt->sense_evt_data.sshdr.sense_key,
> +evt->sense_evt_data.sshdr.asc,
> +evt->sense_evt_data.sshdr.ascq);
> +   break;
> +#endif
> 
> Is a little strange.  The "SDEV_UA" property implies that it was a
> UNIT ATTENTION, but you are filtering on other sense types, so you
> would probably want a different property.  Many SCSI commands do not
> have a LOGICAL BLOCK ADDRESS field, or a TRANSFER LENGTH field, so
> you might be reporting garbage if they don't.  And, since this is a
> SCSI command, you probably want to report the length from the command,
> not the count in bytes from the request structure.

I didn't realize SDEV_UA means UNIT ATTENTION. Let me think about a 
different name. 

For the data being logged, I was thinking to log the whole SCSI command
and full sense buffer. Let me try a few different things. 

Thanks again,
Song

Re: [RFC] scsi: generate uevent for SCSI sense code

[Song Liu]

> On May 14, 2017, at 11:04 PM, Hannes Reinecke <h...@suse.de> wrote:
> 
> On 05/12/2017 09:02 PM, Song Liu wrote:
>> 
>>> On May 3, 2017, at 5:50 PM, Song Liu <songliubrav...@fb.com> wrote:
>>> 
>>> This patch adds capability for SCSI layer to generate uevent for SCSI
>>> sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.
>>> 
>>> We can configure which sense keys generate uevent for each device
>>> through sysfs entry sense_event_filter. For example, the following
>>> enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
>>> on scsi drive sdc:
>>> 
>>>   echo 0x000c > /sys/block/sdc/device/sense_event_filter
>>> 
>>> Here is an example output captured by udevadm:
>>> 
>>> KERNEL[1214.945358] change   /devices/pci:00/XXX
>>> ACTION=change
>>> DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XXX
>>> DEVTYPE=scsi_device
>>> DRIVER=sd
>>> LBA=0
>>> MODALIAS=scsi:t-0x00
>>> SDEV_UA=SCSI_SENSE
>>> SENSE_CODE=3/11/14
>>> SEQNUM=4536
>>> SIZE=4096
>>> SUBSYSTEM=scsi
>>> 
>>> Signed-off-by: Song Liu <songliubrav...@fb.com>
>>> ---
>>> drivers/scsi/Kconfig   | 14 +++
>>> drivers/scsi/scsi_error.c  | 26 
>>> drivers/scsi/scsi_lib.c| 27 +++--
>>> drivers/scsi/scsi_sysfs.c  | 60 
>>> ++
>>> include/scsi/scsi_device.h | 26 +++-
>>> 5 files changed, 150 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>>> index 3c52867..4f7f211 100644
>>> --- a/drivers/scsi/Kconfig
>>> +++ b/drivers/scsi/Kconfig
>>> @@ -237,6 +237,20 @@ config SCSI_LOGGING
>>>   there should be no noticeable performance impact as long as you have
>>>   logging turned off.
>>> 
>>> +config SCSI_SENSE_UEVENT
>>> +   bool "SCSI sense code logging"
>>> +   depends on SCSI
>>> +   default n
>>> +   ---help---
>>> + This turns on uevent for SCSI sense code.
>>> +
>>> + You can configure which sense keys generate uevent for each device
>>> + through sysfs entry sense_event_filter. For example, the following
>>> + enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
>>> + on scsi drive sdc:
>>> +
>>> + echo 0x000c > /sys/block/sdc/device/sense_event_filter
>>> +
>>> config SCSI_SCAN_ASYNC
>>> bool "Asynchronous SCSI scanning"
>>> depends on SCSI
>>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>>> index d70c67c..eda150e 100644
>>> --- a/drivers/scsi/scsi_error.c
>>> +++ b/drivers/scsi/scsi_error.c
>>> @@ -426,6 +426,31 @@ static void scsi_report_sense(struct scsi_device *sdev,
>>> }
>>> }
>>> 
>>> +/*
>>> + * generate uevent when receiving sense code from device
>>> + */
>>> +static void scsi_send_sense_uevent(struct scsi_device *sdev,
>>> +  struct scsi_cmnd *scmd,
>>> +  struct scsi_sense_hdr *sshdr)
>>> +{
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> +   struct scsi_event *evt;
>>> +
>>> +   if (!test_bit(sshdr->sense_key & 0xf,
>>> + >sense_event_filter))
>>> +   return;
>>> +   evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
>>> +   if (!evt)
>>> +   return;
>>> +
>>> +   evt->sense_evt_data.lba = scsi_get_lba(scmd);
>>> +   evt->sense_evt_data.size = blk_rq_bytes(scmd->request);
>>> +   memcpy(>sense_evt_data.sshdr, sshdr,
>>> +  sizeof(struct scsi_sense_hdr));
>>> +   sdev_evt_send(sdev, evt);
>>> +#endif
>>> +}
>>> +
>>> /**
>>> * scsi_check_sense - Examine scsi cmd sense
>>> * @scmd:Cmd to have sense checked.
>>> @@ -446,6 +471,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
>>> return FAILED;  /* no valid sense data */
>>> 
>>> scsi_report_sense(sdev, );
>>> +   scsi_send_sense_uevent(sdev, scmd, );
>>> 
>>> if (scsi_sense_is_deferred())
>>> return NEEDS_RETRY;
>>> diff --git a/drivers/scsi/scsi_lib.c b/dri

Re: [RFC] scsi: generate uevent for SCSI sense code

[Song Liu]

> On May 3, 2017, at 5:50 PM, Song Liu <songliubrav...@fb.com> wrote:
> 
> This patch adds capability for SCSI layer to generate uevent for SCSI
> sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.
> 
> We can configure which sense keys generate uevent for each device
> through sysfs entry sense_event_filter. For example, the following
> enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
> on scsi drive sdc:
> 
>echo 0x000c > /sys/block/sdc/device/sense_event_filter
> 
> Here is an example output captured by udevadm:
> 
> KERNEL[1214.945358] change   /devices/pci:00/XXX
> ACTION=change
> DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XXX
> DEVTYPE=scsi_device
> DRIVER=sd
> LBA=0
> MODALIAS=scsi:t-0x00
> SDEV_UA=SCSI_SENSE
> SENSE_CODE=3/11/14
> SEQNUM=4536
> SIZE=4096
> SUBSYSTEM=scsi
> 
> Signed-off-by: Song Liu <songliubrav...@fb.com>
> ---
> drivers/scsi/Kconfig   | 14 +++
> drivers/scsi/scsi_error.c  | 26 
> drivers/scsi/scsi_lib.c| 27 +++--
> drivers/scsi/scsi_sysfs.c  | 60 ++
> include/scsi/scsi_device.h | 26 +++-
> 5 files changed, 150 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 3c52867..4f7f211 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -237,6 +237,20 @@ config SCSI_LOGGING
> there should be no noticeable performance impact as long as you have
> logging turned off.
> 
> +config SCSI_SENSE_UEVENT
> + bool "SCSI sense code logging"
> + depends on SCSI
> + default n
> + ---help---
> +   This turns on uevent for SCSI sense code.
> +
> +   You can configure which sense keys generate uevent for each device
> +   through sysfs entry sense_event_filter. For example, the following
> +   enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
> +   on scsi drive sdc:
> +
> +   echo 0x000c > /sys/block/sdc/device/sense_event_filter
> +
> config SCSI_SCAN_ASYNC
>   bool "Asynchronous SCSI scanning"
>   depends on SCSI
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index d70c67c..eda150e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -426,6 +426,31 @@ static void scsi_report_sense(struct scsi_device *sdev,
>   }
> }
> 
> +/*
> + * generate uevent when receiving sense code from device
> + */
> +static void scsi_send_sense_uevent(struct scsi_device *sdev,
> +struct scsi_cmnd *scmd,
> +struct scsi_sense_hdr *sshdr)
> +{
> +#ifdef CONFIG_SCSI_SENSE_UEVENT
> + struct scsi_event *evt;
> +
> + if (!test_bit(sshdr->sense_key & 0xf,
> +   >sense_event_filter))
> + return;
> + evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
> + if (!evt)
> + return;
> +
> + evt->sense_evt_data.lba = scsi_get_lba(scmd);
> + evt->sense_evt_data.size = blk_rq_bytes(scmd->request);
> + memcpy(>sense_evt_data.sshdr, sshdr,
> +sizeof(struct scsi_sense_hdr));
> + sdev_evt_send(sdev, evt);
> +#endif
> +}
> +
> /**
>  * scsi_check_sense - Examine scsi cmd sense
>  * @scmd: Cmd to have sense checked.
> @@ -446,6 +471,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
>   return FAILED;  /* no valid sense data */
> 
>   scsi_report_sense(sdev, );
> + scsi_send_sense_uevent(sdev, scmd, );
> 
>   if (scsi_sense_is_deferred())
>   return NEEDS_RETRY;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 95f963b..1095f27 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2656,8 +2656,9 @@ 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];
> + int idx = 0, i;
> + char *envp[5];  /* SDEV_EVT_SCSI_SENSE needs most entries (4) */
> + int free_envp = -1;
> 
>   switch (evt->evt_type) {
>   case SDEV_EVT_MEDIA_CHANGE:
> @@ -2682,6 +2683,23 @@ static void scsi_evt_emit(struct scsi_device *sdev, 
> struct scsi_event *evt)
>   case SDEV_EVT_ALUA_STATE_CHANGE_REPORTED:
>   envp[idx++] = "SDEV_UA=ASYMMETRIC_ACCESS_STATE_CHANGED";
>   break;
> +#ifdef CONFIG_SCSI_SENSE_UEVENT
> + case SDEV_EVT_SCSI_SENSE:
> + envp[idx++]

[RFC] scsi: generate uevent for SCSI sense code

[Song Liu]

This patch adds capability for SCSI layer to generate uevent for SCSI
sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.

We can configure which sense keys generate uevent for each device
through sysfs entry sense_event_filter. For example, the following
enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
on scsi drive sdc:

echo 0x000c > /sys/block/sdc/device/sense_event_filter

Here is an example output captured by udevadm:

KERNEL[1214.945358] change   /devices/pci:00/XXX
ACTION=change
DEVPATH=/devices/pci:00/:00:01.0/:01:00.0/host6/XXX
DEVTYPE=scsi_device
DRIVER=sd
LBA=0
MODALIAS=scsi:t-0x00
SDEV_UA=SCSI_SENSE
SENSE_CODE=3/11/14
SEQNUM=4536
SIZE=4096
SUBSYSTEM=scsi

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/scsi_error.c  | 26 
 drivers/scsi/scsi_lib.c| 27 +++--
 drivers/scsi/scsi_sysfs.c  | 60 ++
 include/scsi/scsi_device.h | 26 +++-
 5 files changed, 150 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 3c52867..4f7f211 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -237,6 +237,20 @@ config SCSI_LOGGING
  there should be no noticeable performance impact as long as you have
  logging turned off.
 
+config SCSI_SENSE_UEVENT
+   bool "SCSI sense code logging"
+   depends on SCSI
+   default n
+   ---help---
+ This turns on uevent for SCSI sense code.
+
+ You can configure which sense keys generate uevent for each device
+ through sysfs entry sense_event_filter. For example, the following
+ enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
+ on scsi drive sdc:
+
+ echo 0x000c > /sys/block/sdc/device/sense_event_filter
+
 config SCSI_SCAN_ASYNC
bool "Asynchronous SCSI scanning"
depends on SCSI
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d70c67c..eda150e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -426,6 +426,31 @@ static void scsi_report_sense(struct scsi_device *sdev,
}
 }
 
+/*
+ * generate uevent when receiving sense code from device
+ */
+static void scsi_send_sense_uevent(struct scsi_device *sdev,
+  struct scsi_cmnd *scmd,
+  struct scsi_sense_hdr *sshdr)
+{
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   struct scsi_event *evt;
+
+   if (!test_bit(sshdr->sense_key & 0xf,
+ >sense_event_filter))
+   return;
+   evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
+   if (!evt)
+   return;
+
+   evt->sense_evt_data.lba = scsi_get_lba(scmd);
+   evt->sense_evt_data.size = blk_rq_bytes(scmd->request);
+   memcpy(>sense_evt_data.sshdr, sshdr,
+  sizeof(struct scsi_sense_hdr));
+   sdev_evt_send(sdev, evt);
+#endif
+}
+
 /**
  * scsi_check_sense - Examine scsi cmd sense
  * @scmd:  Cmd to have sense checked.
@@ -446,6 +471,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
return FAILED;  /* no valid sense data */
 
scsi_report_sense(sdev, );
+   scsi_send_sense_uevent(sdev, scmd, );
 
if (scsi_sense_is_deferred())
return NEEDS_RETRY;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 95f963b..1095f27 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2656,8 +2656,9 @@ 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];
+   int idx = 0, i;
+   char *envp[5];  /* SDEV_EVT_SCSI_SENSE needs most entries (4) */
+   int free_envp = -1;
 
switch (evt->evt_type) {
case SDEV_EVT_MEDIA_CHANGE:
@@ -2682,6 +2683,23 @@ static void scsi_evt_emit(struct scsi_device *sdev, 
struct scsi_event *evt)
case SDEV_EVT_ALUA_STATE_CHANGE_REPORTED:
envp[idx++] = "SDEV_UA=ASYMMETRIC_ACCESS_STATE_CHANGED";
break;
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   case SDEV_EVT_SCSI_SENSE:
+   envp[idx++] = "SDEV_UA=SCSI_SENSE";
+   for (i = idx; i < idx + 3; ++i) {
+   envp[i] = kzalloc(32, GFP_ATOMIC);
+   if (!envp[i])
+   break;
+   free_envp = i;
+   }
+   snprintf(envp[idx++], 32, "LBA=%lu", evt->sense_evt_data.lba);
+   snprintf(envp[idx++], 32, "SIZE=%d", evt->sense_evt_data.size);
+   snprintf(envp[idx++], 32, "SENSE_CODE=%1x/%02x/%02x",
+evt->sense_evt_data.sshdr.sen

[RFC] generate uevent for SCSI sense code

[Song Liu]

This change is to follow up our discussion on event log for media
management during LSF/MM 2017.

Please kindly let me know your thoughts on this.

Thanks,
Song


Song Liu (1):
  scsi: generate uevent for SCSI sense code

 drivers/scsi/Kconfig   | 14 +++
 drivers/scsi/scsi_error.c  | 26 
 drivers/scsi/scsi_lib.c| 27 +++--
 drivers/scsi/scsi_sysfs.c  | 60 ++
 include/scsi/scsi_device.h | 26 +++-
 5 files changed, 150 insertions(+), 3 deletions(-)

--
2.9.3

Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

> On Apr 25, 2017, at 3:17 PM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Tue, 2017-04-25 at 21:17 +, Song Liu wrote:
>> I am not sure I fully understand the problem here. If I understand the logic 
>> correctly, when a device is being removed, it will stay in 
>> scsi_host->__devices
>> until fully the remove routine is finished. And LUN scanning in parallel will
>> find the device with scsi_device_lookup_by_target(), and thus it would not 
>> rescan the device until the device is fully removed? Did I miss anything 
>> here?
> 
> Hello Song,
> 
> The SCSI core is already complicated enough. Please don't complicate it 
> further
> by making subtle changes to the semantics of scan_mutex. Please also note that
> I have proposed an alternative, namely to make the START STOP UNIT command
> asynchronous.
> 

Hello Bart, 

I total agree with your concern in making SCSI core more complicated. However, 
I am
not sure whether how asynchronous START STOP UNIT command makes multiple 
deletes run
in parallel. If I understand correctly, each device delete has the following 
steps:
  1. lock scan_mutex
  2. issue async START STOP UNIT 
  3. wait START STOP UNIT to complete
  4. unlock scan_mutex

scan_mutex will prevent multiple device deletes to run in parallel. 

On the other hand, maybe the problem is simpler than I thought? Once we ensure 
all
slave_destroy() holds proper lock to access host level data, something as 
follows
might just work. In this way, echoing into delete handle is not protected by 
scan_mutex. However, when the sysfs entry exists, the device should be fully 
added 
to the system. And before delete operation finishes, future scan should not 
re-add 
the device (as the device can be found by scsi_device_lookup_by_target). 

Am I too optimistic here?

Thanks,
Song


diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07..d1c3338 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -710,7 +710,7 @@ sdev_store_delete(struct device *dev, struct 
device_attribute *attr,
  const char *buf, size_t count)
 {
if (device_remove_file_self(dev, attr))
-   scsi_remove_device(to_scsi_device(dev));
+   __scsi_remove_device(to_scsi_device(dev));
return count;
 }; 

Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

> On Apr 25, 2017, at 1:59 PM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Fri, 2017-04-21 at 22:31 +, Song Liu wrote:
>> On Apr 21, 2017, at 2:20 PM, Bart Van Assche <bart.vanass...@sandisk.com> 
>> wrote:
>>> On Fri, 2017-04-21 at 14:13 -0700, Song Liu wrote:
>>>> On the other hand, some devices do long latency IO during deletion,
>>>> for example, sd_shutdown() may do sync cache and/or start_stop.
>>>> It is not necessary for these commands to run in series.
>>> 
>>> Have you noticed my patch series that makes sd_shutdown() submit the
>>> SYNCHRONIZE CACHE command asynchronously? Have you tried whether that
>>> patch series would be a good alternative?
>> 
>> The asynchronous SYNCHRONIZE CACHE will not help our use case, where the 
>> latency comes from sd_start_stop_device(). Seems it is not easy to make 
>> the START STOP UNIT command async. 
> 
> Hello Song,
> 
> It should be possible to make the START STOP UNIT command asynchronous too
> by issuing it asynchronously from inside sd_sync_cache_done(). To avoid
> dereferencing a stale struct scsi_disk pointer you will either have to hold
> an additional reference as long as the SYNCHRONIZE CACHE command is in
> progress or copy the data needed from struct scsi_disk into the SCSI request.
> 

Hello Bart, 

As you stated in the patch: blk_cleanup_queue() in __scsi_remove_device() 
will wait for async SYNCHRONIZE CACHE and START STOP UNIT command to complete. 
Since __scsi_remove_device() is called with scan_mutex held, simultaneous 
START STOP UNIT requires are still serialized by the scan_mutex. 

Song

Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

> On Apr 25, 2017, at 10:52 AM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Tue, 2017-04-25 at 17:42 +, Song Liu wrote:
>> I have been studying the code recently. I am wondering whether the following 
>> would work:
>> 
>> 1. Introduce a new mutex for scsi_device to protect most operations in the 
>>   list you gathered above;
>> 
>> 2. For operations like host->slave_destroy(), ensure they access scsi_host 
>>   data with host_lock (or another spin lock). 
>> 
>>   I looked into all instances of slave_destroy, only 2 of them: 
>>   dc395x_slave_destroy() and visorhba_slave_destroy() access scsi_host data 
>>   without protection of spin lock. 
>> 
>> 3. Once 1 and 2 is ready, __scsi_remove_device() only need to hold the mutex
>>   for the scsi_device. scan_mutex is no longer required. 
>> 
>> Is this a valid path?
> 
> Sorry but I don't think so. Unlocking and reacquiring scan_mutex would create
> the potential that LUN scanning occurs in the meantime and hence that it fails
> because LUN removal is incomplete.
> 


Hello Bart, 

I am not sure I fully understand the problem here. If I understand the logic 
correctly, when a device is being removed, it will stay in scsi_host->__devices
until fully the remove routine is finished. And LUN scanning in parallel will
find the device with scsi_device_lookup_by_target(), and thus it would not 
rescan the device until the device is fully removed? Did I miss anything here?

Thanks,
Song

Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

> On Apr 25, 2017, at 10:23 AM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Fri, 2017-04-21 at 14:13 -0700, Song Liu wrote:
>> When a device is deleted through sysfs handle "delete", the code
>> locks shost->scan_mutex. If multiple devices are deleted at the
>> same time, these deletes will be handled in series.
>> 
>> On the other hand, some devices do long latency IO during deletion,
>> for example, sd_shutdown() may do sync cache and/or start_stop.
>> It is not necessary for these commands to run in series.
>> 
>> To reduce latency of parallel "delete" requests, this patch reduces
>> the protection of scan_mutex. The only function with Scsi_Host
>> called in __scsi_remove_device() is the optional slave_destroy().
>> Therefore, the protection of scan_mutex is only necessary for this
>> function.
> 
> Hello Song,
> 
> What you wrote above is wrong. It is necessary to serialize SCSI
> scanning against SCSI device removal. That is why scan_mutex is held
> around the __scsi_remove_device() call. When adding a LUN the following
> (and several other) actions are performed:
> * Allocation of memory for struct scsi_device.
> * Allocation of a block layer request queue.
> * Initialization of the .sdev_gendev and .sdev_dev device structures.
> * Association of the transport layer driver with the SCSI device.
> * Association of a device handler with the SCSI device (e.g. ALUA).
> * Making the .sdev_gendev and .sdev_dev devices visible in sysfs.
> * Making the transport layer attributes visible in sysfs.
> * Creating a bsg device node in sysfs.
> * Association of an upper layer driver
> (e.g. sd or sr) with the SCSI LUN.
> 
> Removal of a LUN means undoing all of the above. If adding and removing
> a LUN would not be not serialized then there would be a risk that removal
> and immediate reregistration of a LUN will fail due to the reregistration
> process trying to add sysfs attributes that were not yet removed by the
> removal step.
> 
> Bart.

Hello Bart, 

Thanks a lot for looking into this. 

I have been studying the code recently. I am wondering whether the following 
would work:

1. Introduce a new mutex for scsi_device to protect most operations in the 
   list you gathered above;

2. For operations like host->slave_destroy(), ensure they access scsi_host 
   data with host_lock (or another spin lock). 

   I looked into all instances of slave_destroy, only 2 of them: 
   dc395x_slave_destroy() and visorhba_slave_destroy() access scsi_host data 
   without protection of spin lock. 

3. Once 1 and 2 is ready, __scsi_remove_device() only need to hold the mutex
   for the scsi_device. scan_mutex is no longer required. 

Is this a valid path?

Thanks again,
Song

Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

> On Apr 21, 2017, at 2:20 PM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Fri, 2017-04-21 at 14:13 -0700, Song Liu wrote:
>> On the other hand, some devices do long latency IO during deletion,
>> for example, sd_shutdown() may do sync cache and/or start_stop.
>> It is not necessary for these commands to run in series.
> 
> Have you noticed my patch series that makes sd_shutdown() submit the
> SYNCHRONIZE CACHE command asynchronously? Have you tried whether that
> patch series would be a good alternative?
> 
> Thanks,
> 
> Bart.

The asynchronous SYNCHRONIZE CACHE will not help our use case, where the 
latency comes from sd_start_stop_device(). Seems it is not easy to make 
the START STOP UNIT command async. 

Thanks,
Song

Re: [RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

> On Apr 21, 2017, at 2:17 PM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Fri, 2017-04-21 at 14:13 -0700, Song Liu wrote:
>> When a device is deleted through sysfs handle "delete", [ ... ]
> 
> If I try to use that sysfs attribute then I encounter a deadlock (see
> also the report below). How is it possible that you have not hit that
> deadlock in your tests?
> 
> Bart.


Hmm... Seems my test environment (VM) doesn't call the following path

   __scsi_remove_device+0xe9/0x120
   scsi_forget_host+0x60/0x70
   scsi_remove_host+0x71/0x110

Let me fix it. 

Thanks,
Song

[RFC] scsi: reduce protection of scan_mutex in scsi_remove_device

[Song Liu]

When a device is deleted through sysfs handle "delete", the code
locks shost->scan_mutex. If multiple devices are deleted at the
same time, these deletes will be handled in series.

On the other hand, some devices do long latency IO during deletion,
for example, sd_shutdown() may do sync cache and/or start_stop.
It is not necessary for these commands to run in series.

To reduce latency of parallel "delete" requests, this patch reduces
the protection of scan_mutex. The only function with Scsi_Host
called in __scsi_remove_device() is the optional slave_destroy().
Therefore, the protection of scan_mutex is only necessary for this
function.

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/scsi_sysfs.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07..e7a9e28 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -610,7 +610,7 @@ static int scsi_sdev_check_buf_bit(const char *buf)
return 1;
else if (buf[0] == '0')
return 0;
-   else 
+   else
return -EINVAL;
} else
return -EINVAL;
@@ -774,7 +774,7 @@ store_queue_type_field(struct device *dev, struct 
device_attribute *attr,
 
if (!sdev->tagged_supported)
return -EINVAL;
-   
+
sdev_printk(KERN_INFO, sdev,
"ignoring write to deprecated queue_type attribute");
return count;
@@ -1302,8 +1302,11 @@ void __scsi_remove_device(struct scsi_device *sdev)
blk_cleanup_queue(sdev->request_queue);
cancel_work_sync(>requeue_work);
 
-   if (sdev->host->hostt->slave_destroy)
+   if (sdev->host->hostt->slave_destroy) {
+   mutex_lock(>host->scan_mutex);
sdev->host->hostt->slave_destroy(sdev);
+   mutex_unlock(>host->scan_mutex);
+   }
transport_destroy_device(dev);
 
/*
@@ -1322,11 +1325,7 @@ void __scsi_remove_device(struct scsi_device *sdev)
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-   struct Scsi_Host *shost = sdev->host;
-
-   mutex_lock(>scan_mutex);
__scsi_remove_device(sdev);
-   mutex_unlock(>scan_mutex);
 }
 EXPORT_SYMBOL(scsi_remove_device);
 
-- 
2.9.3

Re: [PATCH v2] scsi: ses: don't get power status of SES device slot on probe

[Song Liu]

/ses.c b/drivers/scsi/ses.c
> index 50adabbb5808..f1cdf32d7514 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -548,7 +548,6 @@ static void ses_enclosure_data_process(struct 
> enclosure_device *edev,
>   ecomp = >component[components++];
> 
>   if (!IS_ERR(ecomp)) {
> - ses_get_power_status(edev, ecomp);
>   if (addl_desc_ptr)
>   ses_process_descriptor(
>   ecomp,
> -- 
> 1.8.3.1
> 

Reviewed-by: Song Liu <songliubrav...@fb.com>

Re: [PATCH v4] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

Hi Christoph, 

IMHO, scan_mutex maybe still needed for other part of scsi_remove_device(). 

For example, device_del() in sd_remove() and scsi_sysfs_add_devices() in 
scsi_finish_async_scan() should not run in parallel? 

On the other hand, other parts of sd_remove(), including sd_shutdown(), 
does not touch shared resource.  So it is safe to run these sections without
holding scan_mutex. 

Another (and maybe better) solution is to move mutex_lock/unlock() of 
scan_mutex to each _remove function. So we only hold the lock when accessing
scsi_host related data. 

Thanks,
Song


> On Feb 9, 2017, at 12:51 AM, Christoph Hellwig  wrote:
> 
> And what is the lock protecting if we can just release and reacquire it
> safely?  Probably nothing, but someone needs to do the audit carefully.
> 
> Once that is done we can just apply a patch like the one below and
> be done with it:
> 
> ---
> From 47572d7f0758cc0cbd979b1a2c3791f3b94c566e Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig 
> Date: Thu, 9 Feb 2017 09:49:04 +0100
> Subject: scsi: remove scan_mutex
> 
> All our lookup structures are properly locked these days, there shouldn't
> be any need to serialize the high-level scan process.
> 
> Signed-off-by: Christoph Hellwig 
> ---
> drivers/scsi/scsi_scan.c | 15 ---
> 1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6f7128f49c30..c92be1ad486c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1490,7 +1490,6 @@ struct scsi_device *__scsi_add_device(struct Scsi_Host 
> *shost, uint channel,
>   return ERR_PTR(-ENOMEM);
>   scsi_autopm_get_target(starget);
> 
> - mutex_lock(>scan_mutex);
>   if (!shost->async_scan)
>   scsi_complete_async_scans();
> 
> @@ -1498,7 +1497,6 @@ struct scsi_device *__scsi_add_device(struct Scsi_Host 
> *shost, uint channel,
>   scsi_probe_and_add_lun(starget, lun, NULL, , 1, hostdata);
>   scsi_autopm_put_host(shost);
>   }
> - mutex_unlock(>scan_mutex);
>   scsi_autopm_put_target(starget);
>   /*
>* paired with scsi_alloc_target().  Target will be destroyed unless
> @@ -1629,7 +1627,6 @@ void scsi_scan_target(struct device *parent, unsigned 
> int channel,
>   strncmp(scsi_scan_type, "manual", 6) == 0)
>   return;
> 
> - mutex_lock(>scan_mutex);
>   if (!shost->async_scan)
>   scsi_complete_async_scans();
> 
> @@ -1637,7 +1634,6 @@ void scsi_scan_target(struct device *parent, unsigned 
> int channel,
>   __scsi_scan_target(parent, channel, id, lun, rescan);
>   scsi_autopm_put_host(shost);
>   }
> - mutex_unlock(>scan_mutex);
> }
> EXPORT_SYMBOL(scsi_scan_target);
> 
> @@ -1686,7 +1682,6 @@ int scsi_scan_host_selected(struct Scsi_Host *shost, 
> unsigned int channel,
>   ((lun != SCAN_WILD_CARD) && (lun >= shost->max_lun)))
>   return -EINVAL;
> 
> - mutex_lock(>scan_mutex);
>   if (!shost->async_scan)
>   scsi_complete_async_scans();
> 
> @@ -1700,7 +1695,6 @@ int scsi_scan_host_selected(struct Scsi_Host *shost, 
> unsigned int channel,
>   scsi_scan_channel(shost, channel, id, lun, rescan);
>   scsi_autopm_put_host(shost);
>   }
> - mutex_unlock(>scan_mutex);
> 
>   return 0;
> }
> @@ -1752,11 +1746,9 @@ static struct async_scan_data 
> *scsi_prep_async_scan(struct Scsi_Host *shost)
>   goto err;
>   init_completion(>prev_finished);
> 
> - mutex_lock(>scan_mutex);
>   spin_lock_irqsave(shost->host_lock, flags);
>   shost->async_scan = 1;
>   spin_unlock_irqrestore(shost->host_lock, flags);
> - mutex_unlock(>scan_mutex);
> 
>   spin_lock(_scan_lock);
>   if (list_empty(_hosts))
> @@ -1789,12 +1781,9 @@ static void scsi_finish_async_scan(struct 
> async_scan_data *data)
> 
>   shost = data->shost;
> 
> - mutex_lock(>scan_mutex);
> -
>   if (!shost->async_scan) {
>   shost_printk(KERN_INFO, shost, "%s called twice\n", __func__);
>   dump_stack();
> - mutex_unlock(>scan_mutex);
>   return;
>   }
> 
> @@ -1806,8 +1795,6 @@ static void scsi_finish_async_scan(struct 
> async_scan_data *data)
>   shost->async_scan = 0;
>   spin_unlock_irqrestore(shost->host_lock, flags);
> 
> - mutex_unlock(>scan_mutex);
> -
>   spin_lock(_scan_lock);
>   list_del(>list);
>   if (!list_empty(_hosts)) {
> @@ -1915,7 +1902,6 @@ struct scsi_device *scsi_get_host_dev(struct Scsi_Host 
> *shost)
>   struct scsi_device *sdev = NULL;
>   struct scsi_target *starget;
> 
> - mutex_lock(>scan_mutex);
>   if (!scsi_host_scan_allowed(shost))
>   goto out;
>   starget = scsi_alloc_target(>shost_gendev, 0, shost->this_id);
> @@ -1929,7 +1915,6 @@ struct scsi_device 

[PATCH v4] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

When a device is deleted through sysfs handle "delete", the code
locks shost->scan_mutex. If multiple devices are deleted at the
same time, these deletes will be handled in series.

On the other hand, sd_shutdown() sometimes issues long latency
commands: sync cache and start_stop. It is not necessary for these
commands to run in series.

To reduce latency of parallel "delete" requests, this patch unlock
shost->scan_mutex before calling sd_shutdown(), relock the mutex
afterwards.

Fixed bug from previous version.

Reported-by: kernel test robot <fengguang...@intel.com>
Signed-off-by: Song Liu <songliubrav...@fb.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Bart Van Assche <bart.vanass...@sandisk.com>
---
 drivers/scsi/sd.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9e0783b..61ea308 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3211,9 +3211,13 @@ static int sd_probe(struct device *dev)
 static int sd_remove(struct device *dev)
 {
struct scsi_disk *sdkp;
+   struct scsi_device *sdev;
+   struct Scsi_Host *shost;
dev_t devt;
 
sdkp = dev_get_drvdata(dev);
+   sdev = sdkp->device;
+   shost = sdev->host;
devt = disk_devt(sdkp->disk);
scsi_autopm_get_device(sdkp->device);
 
@@ -3221,7 +3225,9 @@ static int sd_remove(struct device *dev)
async_synchronize_full_domain(_sd_probe_domain);
device_del(>dev);
del_gendisk(sdkp->disk);
+   mutex_unlock(>scan_mutex);
sd_shutdown(dev);
+   mutex_lock(>scan_mutex);
 
sd_zbc_remove(sdkp);
 
-- 
2.9.3

Re: [PATCH v3] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

> On Feb 8, 2017, at 2:58 PM, Bart Van Assche <bart.vanass...@sandisk.com> 
> wrote:
> 
> On Wed, 2017-02-08 at 14:53 -0800, Song Liu wrote:
>> +try_lock_scan_mutex = mutex_trylock(>scan_mutex);
> 
> This is at least as bad as the approach of your previous patch because
> whether or not this mutex_trylock() call succeeds not only depends on
> whether or not the caller holds the scan_mutex but also on whether any
> other thread accidentally holds that mutex. Please stop hacking and do
> what Christoph proposed, namely address the caller of this method.
> 
> Bart.
> Western Digital Corporation (and its subsidiaries) E-mail Confidentiality 
> Notice & Disclaimer:
> 
> This e-mail and any files transmitted with it may contain confidential or 
> legally privileged information of WDC and/or its affiliates, and are intended 
> solely for the use of the individual or entity to which they are addressed. 
> If you are not the intended recipient, any disclosure, copying, distribution 
> or any action taken or omitted to be taken in reliance on it, is prohibited. 
> If you have received this e-mail in error, please notify the sender 
> immediately and delete the e-mail in its entirety from your system.
> 

Thanks for the feedback. I will dig more in the caller side. 

Song

Re: [PATCH v3] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

> On Feb 8, 2017, at 2:53 PM, Song Liu <songliubrav...@fb.com> wrote:
> 
> When a device is deleted through sysfs handle "delete", the code
> locks shost->scan_mutex. If multiple devices are deleted at the
> same time, these deletes will be handled in series.
> 
> On the other hand, sd_shutdown() sometimes issues long latency
> commands: sync cache and start_stop. It is not necessary for these
> commands to run in series.
> 
> To reduce latency of parallel "delete" requests, this patch unlock
> shost->scan_mutex before long latency commands and relock the mutex
> after the command.
> 
> Fixed bug from previous version.
> 
> Reported-by: kernel test robot <fengguang...@intel.com>
> Signed-off-by: Song Liu <songliubrav...@fb.com>
> ---
> drivers/scsi/sd.c | 14 ++
> 1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 9e0783b..14c5815 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3304,6 +3304,9 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, 
> int start)
> static void sd_shutdown(struct device *dev)
> {
>   struct scsi_disk *sdkp = dev_get_drvdata(dev);
> + struct scsi_device *sdev;
> + struct Scsi_Host *shost;
> + int try_lock_scan_mutex;
> 
>   if (!sdkp)
>   return; /* this can happen */
> @@ -3311,15 +3314,26 @@ static void sd_shutdown(struct device *dev)
>   if (pm_runtime_suspended(dev))
>   return;
> 
> + sdev = sdkp->device;
> + shost = sdev->host;
> + try_lock_scan_mutex = mutex_trylock(>scan_mutex);
> +
>   if (sdkp->WCE && sdkp->media_present) {
> + mutex_unlock(>scan_mutex);
>   sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
>   sd_sync_cache(sdkp);
> + mutex_lock(>scan_mutex);
>   }
> 
>   if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> + mutex_unlock(>scan_mutex);
>   sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
>   sd_start_stop_device(sdkp, 0);
> + mutex_lock(>scan_mutex);
>   }
> +
> + if (try_lock_scan_mutex)
> + mutex_unlock(>scan_mutex);
> }
> 
> static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
> -- 
> 2.9.3
> 

Forgot to CC Christoph

[PATCH v3] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

When a device is deleted through sysfs handle "delete", the code
locks shost->scan_mutex. If multiple devices are deleted at the
same time, these deletes will be handled in series.

On the other hand, sd_shutdown() sometimes issues long latency
commands: sync cache and start_stop. It is not necessary for these
commands to run in series.

To reduce latency of parallel "delete" requests, this patch unlock
shost->scan_mutex before long latency commands and relock the mutex
after the command.

Fixed bug from previous version.

Reported-by: kernel test robot <fengguang...@intel.com>
Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/sd.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9e0783b..14c5815 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3304,6 +3304,9 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, 
int start)
 static void sd_shutdown(struct device *dev)
 {
struct scsi_disk *sdkp = dev_get_drvdata(dev);
+   struct scsi_device *sdev;
+   struct Scsi_Host *shost;
+   int try_lock_scan_mutex;
 
if (!sdkp)
return; /* this can happen */
@@ -3311,15 +3314,26 @@ static void sd_shutdown(struct device *dev)
if (pm_runtime_suspended(dev))
return;
 
+   sdev = sdkp->device;
+   shost = sdev->host;
+   try_lock_scan_mutex = mutex_trylock(>scan_mutex);
+
if (sdkp->WCE && sdkp->media_present) {
+   mutex_unlock(>scan_mutex);
sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
sd_sync_cache(sdkp);
+   mutex_lock(>scan_mutex);
}
 
if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
+   mutex_unlock(>scan_mutex);
sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
sd_start_stop_device(sdkp, 0);
+   mutex_lock(>scan_mutex);
}
+
+   if (try_lock_scan_mutex)
+   mutex_unlock(>scan_mutex);
 }
 
 static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
-- 
2.9.3

Re: [PATCH v2] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

> On Feb 8, 2017, at 1:02 PM, Christoph Hellwig <h...@infradead.org> wrote:
> 
> On Wed, Feb 08, 2017 at 12:42:40PM -0800, Song Liu wrote:
>> When a device is deleted through sysfs handle "delete", the code
>> locks shost->scan_mutex. If multiple devices are deleted at the
>> same time, these deletes will be handled in series.
>> 
>> On the other hand, sd_shutdown() sometimes issues long latency
>> commands: sync cache and start_stop. It is not necessary for these
>> commands to run in series.
>> 
>> To reduce latency of parallel "delete" requests, this patch unlock
>> shost->scan_mutex before long latency commands and relock the mutex
>> after the command.
>> 
>> Fixed bug from previous version.
>> 
>> Reported-by: kernel test robot <fengguang...@intel.com>
>> Signed-off-by: Song Liu <songliubrav...@fb.com>
>> ---
>> drivers/scsi/sd.c | 15 +++
>> 1 file changed, 15 insertions(+)
>> 
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 9e0783b..22add77 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -3304,6 +3304,9 @@ static int sd_start_stop_device(struct scsi_disk 
>> *sdkp, int start)
>> static void sd_shutdown(struct device *dev)
>> {
>>  struct scsi_disk *sdkp = dev_get_drvdata(dev);
>> +struct scsi_device *sdev;
>> +struct Scsi_Host *shost;
>> +bool scan_mutex_locked;
>> 
>>  if (!sdkp)
>>  return; /* this can happen */
>> @@ -3311,14 +3314,26 @@ static void sd_shutdown(struct device *dev)
>>  if (pm_runtime_suspended(dev))
>>  return;
>> 
>> +sdev = sdkp->device;
>> +shost = sdev->host;
>> +scan_mutex_locked = mutex_is_locked(>scan_mutex);
> 
> The use of mutex_is_locked outside of asserts is always bogus.
> 
> The fix you want is most like in the caller of the method.

Hi Christoph, 

Thanks for the feedback. The other caller (that does not hold scan_mutex) 
of sd_shutdown() is device_shutdown() in drivers/base/core.c. I could not 
think of a good way to lock scan_mutex there. 

Would some variation of mutex_trylock() be a better option here? 

Thanks,
Song

[PATCH v2] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

When a device is deleted through sysfs handle "delete", the code
locks shost->scan_mutex. If multiple devices are deleted at the
same time, these deletes will be handled in series.

On the other hand, sd_shutdown() sometimes issues long latency
commands: sync cache and start_stop. It is not necessary for these
commands to run in series.

To reduce latency of parallel "delete" requests, this patch unlock
shost->scan_mutex before long latency commands and relock the mutex
after the command.

Fixed bug from previous version.

Reported-by: kernel test robot <fengguang...@intel.com>
Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/sd.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 9e0783b..22add77 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3304,6 +3304,9 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, 
int start)
 static void sd_shutdown(struct device *dev)
 {
struct scsi_disk *sdkp = dev_get_drvdata(dev);
+   struct scsi_device *sdev;
+   struct Scsi_Host *shost;
+   bool scan_mutex_locked;
 
if (!sdkp)
return; /* this can happen */
@@ -3311,14 +3314,26 @@ static void sd_shutdown(struct device *dev)
if (pm_runtime_suspended(dev))
return;
 
+   sdev = sdkp->device;
+   shost = sdev->host;
+   scan_mutex_locked = mutex_is_locked(>scan_mutex);
+
if (sdkp->WCE && sdkp->media_present) {
+   if (scan_mutex_locked)
+   mutex_unlock(>scan_mutex);
sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
sd_sync_cache(sdkp);
+   if (scan_mutex_locked)
+   mutex_lock(>scan_mutex);
}
 
if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
+   if (scan_mutex_locked)
+   mutex_unlock(>scan_mutex);
sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
sd_start_stop_device(sdkp, 0);
+   if (scan_mutex_locked)
+   mutex_lock(>scan_mutex);
}
 }
 
-- 
2.9.3

[PATCH] scsi/sd: release scan_mutex during sync_cache and start_stop

[Song Liu]

When a device is deleted through sysfs handle "delete", the code
locks shost->scan_mutex. If multiple devices are deleted at the
same time, these deletes will be handled in series.

On the other hand, sd_shutdown() sometimes issues long latency
commands: sync cache and start_stop. It is not necessary for these
commands to run in series.

To reduce latency of parallel "delete" requests, this patch unlock
shost->scan_mutex before long latency commands and relock the mutex
after the command.

Signed-off-by: Song Liu <songliubrav...@fb.com>
---
 drivers/scsi/sd.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2ea3568..39aa209 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3241,6 +3241,8 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, 
int start)
 static void sd_shutdown(struct device *dev)
 {
struct scsi_disk *sdkp = dev_get_drvdata(dev);
+   struct scsi_device *sdev = sdkp->device;
+   struct Scsi_Host *shost = sdev->host;
 
if (!sdkp)
return; /* this can happen */
@@ -3249,13 +3251,17 @@ static void sd_shutdown(struct device *dev)
return;
 
if (sdkp->WCE && sdkp->media_present) {
+   mutex_unlock(>scan_mutex);
sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
sd_sync_cache(sdkp);
+   mutex_lock(>scan_mutex);
}
 
if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
+   mutex_unlock(>scan_mutex);
sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
sd_start_stop_device(sdkp, 0);
+   mutex_lock(>scan_mutex);
}
 }
 
-- 
2.9.3

[PATCH] ses: clear warning for unused variable

[Song Liu]

Remove the declaration of variable err in function
'enclosure_component_alloc':

 All warnings:
  drivers/misc/enclosure.c: In function 'enclosure_component_alloc':
   drivers/misc/enclosure.c:295:6: warning: unused variable 'err'
   [-Wunused-variable]
 int err, i;
 ^
Signed-off-by: Song Liu songliubrav...@fb.com
Acked-by: Dan Williams dan.j.willi...@intel.com
Reported-by: kbuild test robot fengguang...@intel.com
---
 drivers/misc/enclosure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 3289d4d..38552a3 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -295,7 +295,7 @@ enclosure_component_alloc(struct enclosure_device *edev,
 {
struct enclosure_component *ecomp;
struct device *cdev;
-   int err, i;
+   int i;
char newname[COMPONENT_NAME_SIZE];
 
if (number = edev-components)
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] ses: add reliable slot attribute

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

The name provided by firmware is in a vendor specific format, publish
the slot number to have a reliable mechanism for identifying slots
across firmware implementations.  If the enclosure does not provide a
slot number fallback to the component number which is guaranteed unique,
and usually mirrors the slot number.

Cleaned up the unused ses_component.desc in the process.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Reviewed-by: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 20 +++-
 drivers/scsi/ses.c| 17 -
 include/linux/enclosure.h |  1 +
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index ab4de85..b7995ed 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -145,8 +145,10 @@ enclosure_register(struct device *dev, const char *name, 
int components,
if (err)
goto err;
 
-   for (i = 0; i  components; i++)
+   for (i = 0; i  components; i++) {
edev-component[i].number = -1;
+   edev-component[i].slot = -1;
+   }
 
mutex_lock(container_list_lock);
list_add_tail(edev-node, container_list);
@@ -589,6 +591,20 @@ static ssize_t get_component_type(struct device *cdev,
return snprintf(buf, 40, %s\n, enclosure_type[ecomp-type]);
 }
 
+static ssize_t get_component_slot(struct device *cdev,
+ struct device_attribute *attr, char *buf)
+{
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int slot;
+
+   /* if the enclosure does not override then use 'number' as a stand-in */
+   if (ecomp-slot = 0)
+   slot = ecomp-slot;
+   else
+   slot = ecomp-number;
+
+   return snprintf(buf, 40, %d\n, slot);
+}
 
 static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
set_component_fault);
@@ -599,6 +615,7 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
+static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
dev_attr_fault.attr,
@@ -606,6 +623,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_active.attr,
dev_attr_locate.attr,
dev_attr_type.attr,
+   dev_attr_slot.attr,
NULL
 };
 ATTRIBUTE_GROUPS(enclosure_component);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 1041556..433de8e 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -47,7 +47,6 @@ struct ses_device {
 
 struct ses_component {
u64 addr;
-   unsigned char *desc;
 };
 
 static int ses_probe(struct device *dev)
@@ -307,19 +306,26 @@ static void ses_process_descriptor(struct 
enclosure_component *ecomp,
int invalid = desc[0]  0x80;
enum scsi_protocol proto = desc[0]  0x0f;
u64 addr = 0;
+   int slot = -1;
struct ses_component *scomp = ecomp-scratch;
unsigned char *d;
 
-   scomp-desc = desc;
-
if (invalid)
return;
 
switch (proto) {
+   case SCSI_PROTOCOL_FCP:
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
+   }
+   break;
case SCSI_PROTOCOL_SAS:
-   if (eip)
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
d = desc + 8;
-   else
+   } else
d = desc + 4;
/* only take the phy0 addr */
addr = (u64)d[12]  56 |
@@ -335,6 +341,7 @@ static void ses_process_descriptor(struct 
enclosure_component *ecomp,
/* FIXME: Need to add more protocols than just SAS */
break;
}
+   ecomp-slot = slot;
scomp-addr = addr;
 }
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 807622b..0f826c1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -92,6 +92,7 @@ struct enclosure_component {
int fault;
int active;
int locate;
+   int slot;
enum enclosure_status status;
 };
 
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ses: close potential registration race

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

The slot and address fields have a small window of instability when
userspace can read them before initialization. Separate
enclosure_component
allocation from registration.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 35 +--
 drivers/scsi/ses.c| 21 ++---
 include/linux/enclosure.h |  5 +++--
 3 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 180a544..d566f0f 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -273,23 +273,22 @@ enclosure_component_find_by_name(struct enclosure_device 
*edev,
 static const struct attribute_group *enclosure_component_groups[];
 
 /**
- * enclosure_component_register - add a particular component to an enclosure
+ * enclosure_component_alloc - prepare a new enclosure component
  * @edev:  the enclosure to add the component
  * @num:   the device number
  * @type:  the type of component being added
  * @name:  an optional name to appear in sysfs (leave NULL if none)
  *
- * Registers the component.  The name is optional for enclosures that
- * give their components a unique name.  If not, leave the field NULL
- * and a name will be assigned.
+ * The name is optional for enclosures that give their components a unique
+ * name.  If not, leave the field NULL and a name will be assigned.
  *
  * Returns a pointer to the enclosure component or an error.
  */
 struct enclosure_component *
-enclosure_component_register(struct enclosure_device *edev,
-unsigned int number,
-enum enclosure_component_type type,
-const char *name)
+enclosure_component_alloc(struct enclosure_device *edev,
+ unsigned int number,
+ enum enclosure_component_type type,
+ const char *name)
 {
struct enclosure_component *ecomp;
struct device *cdev;
@@ -327,14 +326,30 @@ enclosure_component_register(struct enclosure_device 
*edev,
cdev-release = enclosure_component_release;
cdev-groups = enclosure_component_groups;
 
+   return ecomp;
+}
+EXPORT_SYMBOL_GPL(enclosure_component_alloc);
+
+/**
+ * enclosure_component_register - publishes an initialized enclosure component
+ * @ecomp: component to add
+ *
+ * Returns 0 on successful registration, releases the component otherwise
+ */
+int enclosure_component_register(struct enclosure_component *ecomp)
+{
+   struct device *cdev;
+   int err;
+
+   cdev = ecomp-cdev;
err = device_register(cdev);
if (err) {
ecomp-number = -1;
put_device(cdev);
-   return ERR_PTR(err);
+   return err;
}
 
-   return ecomp;
+   return 0;
 }
 EXPORT_SYMBOL_GPL(enclosure_component_register);
 
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index b7e79e7..7dd9cf5 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -423,16 +423,23 @@ static void ses_enclosure_data_process(struct 
enclosure_device *edev,
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
 
if (create)
-   ecomp = 
enclosure_component_register(edev,
-
components++,
-
type_ptr[0],
-
name);
+   ecomp = enclosure_component_alloc(
+   edev,
+   components++,
+   type_ptr[0],
+   name);
else
ecomp = edev-component[components++];
 
-   if (!IS_ERR(ecomp)  addl_desc_ptr)
-   ses_process_descriptor(ecomp,
-  addl_desc_ptr);
+   if (!IS_ERR(ecomp)) {
+   if (addl_desc_ptr)
+   ses_process_descriptor(
+   ecomp,
+   addl_desc_ptr);
+   if (create)
+   enclosure_component_register(
+   ecomp

[PATCH 5/5] ses: Add power_status to SES device slot

[Song Liu]

Add power_status to SES device slot, so we can power on/off the
HDDs behind the enclosure.

Check firmware status in ses_set_* before sending control pages to
firmware.

Signed-off-by: Song Liu songliubrav...@fb.com
Acked-by: Dan Williams dan.j.willi...@intel.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 38 ++
 drivers/scsi/ses.c| 98 ++-
 include/linux/enclosure.h |  6 +++
 3 files changed, 133 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index b7995ed..3289d4d 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char *name, 
int components,
for (i = 0; i  components; i++) {
edev-component[i].number = -1;
edev-component[i].slot = -1;
+   edev-component[i].power_status = 1;
}
 
mutex_lock(container_list_lock);
@@ -583,6 +584,40 @@ static ssize_t set_component_locate(struct device *cdev,
return count;
 }
 
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+   if (edev-cb-get_power_status)
+   edev-cb-get_power_status(edev, ecomp);
+   return snprintf(buf, 40, %s\n, ecomp-power_status ? on : off);
+}
+
+static ssize_t set_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int val;
+
+   if (strncmp(buf, on, 2) == 0 
+   (buf[2] == '\n' || buf[2] == '\0'))
+   val = 1;
+   else if (strncmp(buf, off, 3) == 0 
+   (buf[3] == '\n' || buf[3] == '\0'))
+   val = 0;
+   else
+   return -EINVAL;
+
+   if (edev-cb-set_power_status)
+   edev-cb-set_power_status(edev, ecomp, val);
+   return count;
+}
+
 static ssize_t get_component_type(struct device *cdev,
  struct device_attribute *attr, char *buf)
 {
@@ -614,6 +649,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,
   set_component_active);
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
+  set_component_power_status);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
 static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
@@ -622,6 +659,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_status.attr,
dev_attr_active.attr,
dev_attr_locate.attr,
+   dev_attr_power_status.attr,
dev_attr_type.attr,
dev_attr_slot.attr,
NULL
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 433de8e..dcb0d76 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev)
 #define SES_TIMEOUT (30 * HZ)
 #define SES_RETRIES 3
 
+static void init_device_slot_control(unsigned char *dest_desc,
+struct enclosure_component *ecomp,
+unsigned char *status)
+{
+   memcpy(dest_desc, status, 4);
+   dest_desc[0] = 0;
+   /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+   if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
+   dest_desc[1] = 0;
+   dest_desc[2] = 0xde;
+   dest_desc[3] = 0x3c;
+}
+
+
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 void *buf, int bufflen)
 {
@@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device *edev,
  struct enclosure_component *ecomp,
 enum enclosure_component_setting val)
 {
-   unsigned char desc[4] = {0 };
+   unsigned char desc[4];
+   unsigned char *desc_ptr;
+
+   desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+   if (!desc_ptr)
+   return -EIO;
+
+   init_device_slot_control(desc, ecomp, desc_ptr);
 
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
-   /* zero is disabled */
+   desc[3] = 0xdf;
break;
case ENCLOSURE_SETTING_ENABLED:
-   desc[3] = 0x20;
+   desc[3] |= 0x20;
break;
default

[PATCH 0/5] Feature enhancements for ses module

[Song Liu]

1: close potential race condition by at enclosure race condition

2,3,4: add enclosure id and device slot, so we can create symlink
   in /dev/disk/by-slot:
  # ls -d /dev/disk/by-slot/*
/dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0

5: add ability to power on/off device with power_status file in
   sysfs.

Due to the complexity of SES standard, the module is not to replace implement \
all features of sg_ses (sg3_utils).

Patch 5 and existing features for device element and array device elements 
control \
of HDDs. It is helpful to handle some HDD related fields in the kernel, as the \
kernel can generate mapping between a device to the SES device element (or 
array \
device element):

/sys/block/sdc/device/enclosure_deviceXXX/

With patch 5, we can easily power off a running HDD by

echo off  /sys/block/sdc/device/enclosure_deviceXXX/power_status

This is very useful for systems like Cold Storage, where HDDs are being powered 
\
on/off frequently


Dan Williams (4):
  ses: close potential registration race
  ses: generate KOBJ_CHANGE on enclosure attach
  ses: add enclosure logical id
  ses: add reliable slot attribute

Song Liu (1):
  ses: Add power_status to SES device slot

 drivers/misc/enclosure.c  | 106 +
 drivers/scsi/ses.c| 148 +++---
 include/linux/enclosure.h |  13 +++-
 3 files changed, 232 insertions(+), 35 deletions(-)

-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] ses: add enclosure logical id

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

Export the NAA logical id for the enclosure.  This is optionally
available from the sas_transport_class, but it is really a property of
the enclosure.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 13 +
 drivers/scsi/ses.c|  9 +
 include/linux/enclosure.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index d566f0f..ab4de85 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -432,8 +432,21 @@ static ssize_t components_show(struct device *cdev,
 }
 static DEVICE_ATTR_RO(components);
 
+static ssize_t id_show(struct device *cdev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev);
+
+   if (edev-cb-show_id)
+   return edev-cb-show_id(edev, buf);
+   return -EINVAL;
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *enclosure_class_attrs[] = {
dev_attr_components.attr,
+   dev_attr_id.attr,
NULL,
 };
 ATTRIBUTE_GROUPS(enclosure_class);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 6662b0c..1041556 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_device *edev,
return ses_set_page2_descriptor(edev, ecomp, desc);
 }
 
+static int ses_show_id(struct enclosure_device *edev, char *buf)
+{
+   struct ses_device *ses_dev = edev-scratch;
+   unsigned long long id = get_unaligned_be64(ses_dev-page1+8+4);
+
+   return sprintf(buf, %#llx\n, id);
+}
+
 static struct enclosure_component_callbacks ses_enclosure_callbacks = {
.get_fault  = ses_get_fault,
.set_fault  = ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks 
ses_enclosure_callbacks = {
.get_locate = ses_get_locate,
.set_locate = ses_set_locate,
.set_active = ses_set_active,
+   .show_id= ses_show_id,
 };
 
 struct ses_host_edev {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a835d33..807622b 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -79,6 +79,7 @@ struct enclosure_component_callbacks {
int (*set_locate)(struct enclosure_device *,
  struct enclosure_component *,
  enum enclosure_component_setting);
+   int (*show_id)(struct enclosure_device *, char *buf);
 };
 
 
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ses: generate KOBJ_CHANGE on enclosure attach

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

In support of a /dev/disk/by-slot populated with data from the enclosure
and ses modules udev needs notification when the new interface
files/links are available.  Otherwise, any udev rules specified for the
disk cannot assume that the enclosure topology has settled.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Reviewed-by: Hannes Reinecke h...@suse.de
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 7dd9cf5..6662b0c 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -349,7 +349,8 @@ static int ses_enclosure_find_by_addr(struct 
enclosure_device *edev,
if (scomp-addr != efd-addr)
continue;
 
-   enclosure_add_device(edev, i, efd-dev);
+   if (enclosure_add_device(edev, i, efd-dev) == 0)
+   kobject_uevent(efd-dev-kobj, KOBJ_CHANGE);
return 1;
}
return 0;
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH RESEND 0/5] Feature enhancements for ses module

[Song Liu]

Hi Christoph, 

I just send the patches (http://marc.info/?l=linux-scsim=141997962124647w=2). 

Thanks,
Song

 -Original Message-
 From: Christoph Hellwig [mailto:h...@infradead.org]
 Sent: Tuesday, December 30, 2014 4:45 AM
 To: Song Liu
 Cc: linux-scsi@vger.kernel.org; Dan Williams; Hannes Reinecke; Jens Axboe;
 dgilb...@interlog.com; Christoph Hellwig
 Subject: Re: [PATCH RESEND 0/5] Feature enhancements for ses module
 
 This series fails to apply for me, most likely due to whitespace corruption.  
 Can
 you resend it usign git-send-email, please?
 
 Also please replace the SES: prefix with ses:, thanks!

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 2/5] SES: generate KOBJ_CHANGE on enclosure attach

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

In support of a /dev/disk/by-slot populated with data from the enclosure and 
ses modules udev needs notification when the new interface files/links are 
available.  Otherwise, any udev rules specified for the disk cannot assume that 
the enclosure topology has settled.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Reviewed-by: Hannes Reinecke h...@suse.de
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index c52fd98..87cf970b 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -349,7 +349,8 @@ static int ses_enclosure_find_by_addr(struct 
enclosure_device *edev,
if (scomp-addr != efd-addr)
continue;
 
-   enclosure_add_device(edev, i, efd-dev);
+   if (enclosure_add_device(edev, i, efd-dev) == 0)
+   kobject_uevent(efd-dev-kobj, KOBJ_CHANGE);
return 1;
}
return 0;
--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 3/5] SES: add enclosure logical id

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

Export the NAA logical id for the enclosure.  This is optionally available from 
the sas_transport_class, but it is really a property of the enclosure.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 13 +
 drivers/scsi/ses.c|  9 +
 include/linux/enclosure.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
15faf61..18b87de 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -395,8 +395,21 @@ static ssize_t components_show(struct device *cdev,  }  
static DEVICE_ATTR_RO(components);
 
+static ssize_t id_show(struct device *cdev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev);
+
+   if (edev-cb-show_id)
+   return edev-cb-show_id(edev, buf);
+   return -EINVAL;
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *enclosure_class_attrs[] = {
dev_attr_components.attr,
+   dev_attr_id.attr,
NULL,
 };
 ATTRIBUTE_GROUPS(enclosure_class);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 87cf970b..696d5d8 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_device *edev,
return ses_set_page2_descriptor(edev, ecomp, desc);  }
 
+static int ses_show_id(struct enclosure_device *edev, char *buf) {
+   struct ses_device *ses_dev = edev-scratch;
+   unsigned long long id = get_unaligned_be64(ses_dev-page1+8+4);
+
+   return sprintf(buf, %#llx\n, id);
+}
+
 static struct enclosure_component_callbacks ses_enclosure_callbacks = {
.get_fault  = ses_get_fault,
.set_fault  = ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks 
ses_enclosure_callbacks = {
.get_locate = ses_get_locate,
.set_locate = ses_set_locate,
.set_active = ses_set_active,
+   .show_id= ses_show_id,
 };
 
 struct ses_host_edev {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h index 
a835d33..807622b 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -79,6 +79,7 @@ struct enclosure_component_callbacks {
int (*set_locate)(struct enclosure_device *,
  struct enclosure_component *,
  enum enclosure_component_setting);
+   int (*show_id)(struct enclosure_device *, char *buf);
 };
 
 
--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 4/5] SES: add reliable slot attribute

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

The name provided by firmware is in a vendor specific format, publish the slot 
number to have a reliable mechanism for identifying slots across firmware 
implementations.  If the enclosure does not provide a slot number fallback to 
the component number which is guaranteed unique, and usually mirrors the slot 
number.

Cleaned up the unused ses_component.desc in the process.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Reviewed-by: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 20 +++-
 drivers/scsi/ses.c| 17 -
 include/linux/enclosure.h |  1 +
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
18b87de..2e3eafa 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -145,8 +145,10 @@ enclosure_register(struct device *dev, const char *name, 
int components,
if (err)
goto err;
 
-   for (i = 0; i  components; i++)
+   for (i = 0; i  components; i++) {
edev-component[i].number = -1;
+   edev-component[i].slot = -1;
+   }
 
mutex_lock(container_list_lock);
list_add_tail(edev-node, container_list); @@ -552,6 +554,20 @@ 
static ssize_t get_component_type(struct device *cdev,
return snprintf(buf, 40, %s\n, enclosure_type[ecomp-type]);  }
 
+static ssize_t get_component_slot(struct device *cdev,
+ struct device_attribute *attr, char *buf) {
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int slot;
+
+   /* if the enclosure does not override then use 'number' as a stand-in */
+   if (ecomp-slot = 0)
+   slot = ecomp-slot;
+   else
+   slot = ecomp-number;
+
+   return snprintf(buf, 40, %d\n, slot); }
 
 static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
set_component_fault);
@@ -562,6 +578,7 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,  static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, 
get_component_locate,
   set_component_locate);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
+static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
dev_attr_fault.attr,
@@ -569,6 +586,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_active.attr,
dev_attr_locate.attr,
dev_attr_type.attr,
+   dev_attr_slot.attr,
NULL
 };
 ATTRIBUTE_GROUPS(enclosure_component);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 696d5d8..3e59a5d 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -47,7 +47,6 @@ struct ses_device {
 
 struct ses_component {
u64 addr;
-   unsigned char *desc;
 };
 
 static int ses_probe(struct device *dev) @@ -307,19 +306,26 @@ static void 
ses_process_descriptor(struct enclosure_component *ecomp,
int invalid = desc[0]  0x80;
enum scsi_protocol proto = desc[0]  0x0f;
u64 addr = 0;
+   int slot = -1;
struct ses_component *scomp = ecomp-scratch;
unsigned char *d;
 
-   scomp-desc = desc;
-
if (invalid)
return;
 
switch (proto) {
+   case SCSI_PROTOCOL_FCP:
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
+   }
+   break;
case SCSI_PROTOCOL_SAS:
-   if (eip)
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
d = desc + 8;
-   else
+   } else
d = desc + 4;
/* only take the phy0 addr */
addr = (u64)d[12]  56 |
@@ -335,6 +341,7 @@ static void ses_process_descriptor(struct 
enclosure_component *ecomp,
/* FIXME: Need to add more protocols than just SAS */
break;
}
+   ecomp-slot = slot;
scomp-addr = addr;
 }
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h index 
807622b..0f826c1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -92,6 +92,7 @@ struct enclosure_component {
int fault;
int active;
int locate;
+   int slot;
enum enclosure_status status;
 };
 
--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 0/5] Feature enhancements for ses module

[Song Liu]

These patches include a few enhancements to ses module:

1: close potential race condition by at enclosure race condition

2,3,4: add enclosure id and device slot, so we can create symlink
   in /dev/disk/by-slot:
  # ls -d /dev/disk/by-slot/*
/dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0

5: add ability to power on/off device with power_status file in
   sysfs.

Due to the complexity of SES standard, the module is not to replace implement 
all features (which is the goal of sg_ses from sg3_utils). Patch 5 and existing 
features for device element and array device elements control of HDDs. It is 
helpful to handle some HDD related fields in the kernel, as the kernel can 
generate mapping between a device to the SES device element (or array device 
element): 

/sys/block/sdc/device/enclosure_deviceXXX/

With patch 5, we can easily power off a running HDD by 

echo off  /sys/block/sdc/device/enclosure_deviceXXX/power_status

This is very useful for systems like Cold Storage, where HDDs are being powered 
\ on/off frequently

Dan Williams (4):
  SES: close potential registration race
  SES: generate KOBJ_CHANGE on enclosure attach
  SES: add enclosure logical id
  SES: add reliable slot attribute

Song Liu (1):
  SES: Add power_status to SES enclosure component

 drivers/misc/enclosure.c  | 107 +
 drivers/scsi/ses.c| 148 +++---
 include/linux/enclosure.h |  13 +++-
 3 files changed, 232 insertions(+), 36 deletions(-)

--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 5/5] SES: Add power_status to SES enclosure component

[Song Liu]

Add power_status to SES enclosure component, so we can power on/off the HDDs 
behind the enclosure.

Check firmware status in ses_set_* before sending control pages to firmware.

Signed-off-by: Song Liu songliubrav...@fb.com
Acked-by: Dan Williams dan.j.willi...@intel.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 38 ++
 drivers/scsi/ses.c| 98 ++-
 include/linux/enclosure.h |  6 +++
 3 files changed, 133 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
2e3eafa..819f2f2 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char *name, 
int components,
for (i = 0; i  components; i++) {
edev-component[i].number = -1;
edev-component[i].slot = -1;
+   edev-component[i].power_status = 1;
}
 
mutex_lock(container_list_lock);
@@ -546,6 +547,40 @@ static ssize_t set_component_locate(struct device *cdev,
return count;
 }
 
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+   if (edev-cb-get_power_status)
+   edev-cb-get_power_status(edev, ecomp);
+   return snprintf(buf, 40, %s\n, ecomp-power_status ? on : off); 
+}
+
+static ssize_t set_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int val;
+
+   if (strncmp(buf, on, 2) == 0 
+   (buf[2] == '\n' || buf[2] == '\0'))
+   val = 1;
+   else if (strncmp(buf, off, 3) == 0 
+   (buf[3] == '\n' || buf[3] == '\0'))
+   val = 0;
+   else
+   return -EINVAL;
+
+   if (edev-cb-set_power_status)
+   edev-cb-set_power_status(edev, ecomp, val);
+   return count;
+}
+
 static ssize_t get_component_type(struct device *cdev,
  struct device_attribute *attr, char *buf)  { 
@@ -577,6 +612,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,
   set_component_active);
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
+  set_component_power_status);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);  static 
DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
@@ -585,6 +622,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_status.attr,
dev_attr_active.attr,
dev_attr_locate.attr,
+   dev_attr_power_status.attr,
dev_attr_type.attr,
dev_attr_slot.attr,
NULL
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 3e59a5d..8ba3c78 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev)  #define 
SES_TIMEOUT (30 * HZ)  #define SES_RETRIES 3
 
+static void init_device_slot_control(unsigned char *dest_desc,
+struct enclosure_component *ecomp,
+unsigned char *status)
+{
+   memcpy(dest_desc, status, 4);
+   dest_desc[0] = 0;
+   /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+   if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
+   dest_desc[1] = 0;
+   dest_desc[2] = 0xde;
+   dest_desc[3] = 0x3c;
+}
+
+
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 void *buf, int bufflen)
 {
@@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device *edev,
  struct enclosure_component *ecomp,
 enum enclosure_component_setting val)  {
-   unsigned char desc[4] = {0 };
+   unsigned char desc[4];
+   unsigned char *desc_ptr;
+
+   desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+   if (!desc_ptr)
+   return -EIO;
+
+   init_device_slot_control(desc, ecomp, desc_ptr);
 
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
-   /* zero is disabled */
+   desc[3] = 0xdf;
break;
case ENCLOSURE_SETTING_ENABLED:
-   desc[3] = 0x20;
+   desc[3] |= 0x20;
break;
default

[PATCH RESEND 1/5] SES: close potential registration race

[Song Liu]

From: Dan Williams dan.j.willi...@intel.com

The slot and address fields have a small window of instability when userspace 
can read them before initialization. Separate enclosure_component allocation 
from registration.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 36 +---
 drivers/scsi/ses.c| 21 ++---
 include/linux/enclosure.h |  5 +++--
 3 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
2cf2bbc..15faf61 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -249,27 +249,25 @@ static void enclosure_component_release(struct device 
*dev)  static const struct attribute_group *enclosure_component_groups[];
 
 /**
- * enclosure_component_register - add a particular component to an enclosure
+ * enclosure_component_alloc - prepare a new enclosure component
  * @edev:  the enclosure to add the component
  * @num:   the device number
  * @type:  the type of component being added
  * @name:  an optional name to appear in sysfs (leave NULL if none)
  *
- * Registers the component.  The name is optional for enclosures that
- * give their components a unique name.  If not, leave the field NULL
- * and a name will be assigned.
+ * The name is optional for enclosures that give their components a 
+ unique
+ * name.  If not, leave the field NULL and a name will be assigned.
  *
  * Returns a pointer to the enclosure component or an error.
  */
 struct enclosure_component *
-enclosure_component_register(struct enclosure_device *edev,
-unsigned int number,
-enum enclosure_component_type type,
-const char *name)
+enclosure_component_alloc(struct enclosure_device *edev,
+ unsigned int number,
+ enum enclosure_component_type type,
+ const char *name)
 {
struct enclosure_component *ecomp;
struct device *cdev;
-   int err;
 
if (number = edev-components)
return ERR_PTR(-EINVAL);
@@ -291,14 +289,30 @@ enclosure_component_register(struct enclosure_device 
*edev,
cdev-release = enclosure_component_release;
cdev-groups = enclosure_component_groups;
 
+   return ecomp;
+}
+EXPORT_SYMBOL_GPL(enclosure_component_alloc);
+
+/**
+ * enclosure_component_register - publishes an initialized enclosure component
+ * @ecomp: component to add
+ *
+ * Returns 0 on successful registration, releases the component 
+otherwise  */ int enclosure_component_register(struct 
+enclosure_component *ecomp) {
+   struct device *cdev;
+   int err;
+
+   cdev = ecomp-cdev;
err = device_register(cdev);
if (err) {
ecomp-number = -1;
put_device(cdev);
-   return ERR_PTR(err);
+   return err;
}
 
-   return ecomp;
+   return 0;
 }
 EXPORT_SYMBOL_GPL(enclosure_component_register);
 
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 80bfece..c52fd98 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -423,16 +423,23 @@ static void ses_enclosure_data_process(struct 
enclosure_device *edev,
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
 
if (create)
-   ecomp = 
enclosure_component_register(edev,
-
components++,
-
type_ptr[0],
-
name);
+   ecomp = enclosure_component_alloc(
+   edev,
+   components++,
+   type_ptr[0],
+   name);
else
ecomp = edev-component[components++];
 
-   if (!IS_ERR(ecomp)  addl_desc_ptr)
-   ses_process_descriptor(ecomp,
-  addl_desc_ptr);
+   if (!IS_ERR(ecomp)) {
+   if (addl_desc_ptr)
+   ses_process_descriptor(
+   ecomp,
+   addl_desc_ptr);
+   if (create

RE: [PATCH 0/5] Feature enhancements for ses module

[Song Liu]

Hi Doug, 

I agree what it is difficult to handle all elements, and thus using sg_ses 
probably makes more sense. However, it helps to handle some HDD related fields 
in the kernel, as the kernel can generate mapping between a device to the SES 
device element (or array device element): 

/sys/block/sdc/device/enclosure_deviceXXX/

With patch 5, we can easily power off a running HDD by 

echo off  /sys/block/sdc/device/enclosure_deviceXXX/power_status

This is very useful for systems like Cold Storage, where HDDs are being powered 
on/off frequently. 


In current code, ses_set_page2_descriptor already clear reserved field for all 
elements, and only send non-zero for the device element or array device. Patch 
5 also handles reserved field of these two elements in init_device_slot_control:

dest_desc[2] = 0xde;
dest_desc[3] = 0x3c;

So I think we can control fault, locate, active, and power_status of each HDD 
without issue. 

Please let me know your suggestion on this. 

Thanks,
Song

 -Original Message-
 From: Song Liu
 Sent: Wednesday, October 22, 2014 8:42 PM
 To: 'dgilb...@interlog.com'; Jens Axboe; linux-scsi@vger.kernel.org
 Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
 Subject: RE: [PATCH 0/5] Feature enhancements for ses module
 
 Hi Doug,
 
 The power on/off field together with fault, locate, and active are for 
 HDD
 (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other
 elements. So we are not dealing with power off duration, etc. here.
 
 Thanks,
 Song
 
  -Original Message-
  From: Douglas Gilbert [mailto:dgilb...@interlog.com]
  Sent: Wednesday, October 22, 2014 6:17 PM
  To: Song Liu; Jens Axboe; linux-scsi@vger.kernel.org
  Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
  Subject: Re: [PATCH 0/5] Feature enhancements for ses module
 
  On 14-10-23 01:01 AM, Song Liu wrote:
   Hi Doug,
  
   I am not sure whether I fully understand the scenario. Actually
   patch 5 tries to
  clear all reserved bits:
  
   + dest_desc[0] = 0;
   + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if
   + (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
   +   dest_desc[1] = 0;
   + dest_desc[2] = 0xde;
   + dest_desc[3] = 0x3c;
  
   Would this work for device that rejects request with 1 in RESERVED areas?
 
 
  That is a pretty asymmetric element type, assuming we are talking
  about the enclosure control and enclosure status elements (i.e.
  etc=0xe). My guess would be:
 
   dest_desc[0] = 0x80 | (src_desc[0]  40);
   dest_desc[1] = 0x80  src_desc[1];
   dest_desc[2] = (pc_req  6) | pc_delay;
   dest_desc[3] = 0xff  src_desc[3]; or if you have a new
  power_off_duration:
   dest_desc[3] = (power_off_duration  2) | (src_desc[3]  0x3);
 
  In byte 0 the top bit (SELECT) must be set or the enclosure will
  ignore any other settings in that element. If the PRDFAIL bit is
  already set, then that setting will be maintained.
  SES-3 has a note about clearing DISABLE and SWAP.
 
  In byte 1 is if the identifier (LED ?) is active (saying blinking)
  prior to this power cycle request, then it will stay blinking until
  the power drops. If the enclosure was really clever it might keep
  blinking after the power cycle :-)
 
  Also notice that the requested power cycle can be cancelled up to the
  time until power cycle drops to zero.
 
   -Original Message-
   From: Douglas Gilbert [mailto:dgilb...@interlog.com]
   Sent: Wednesday, October 22, 2014 3:29 PM
   To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
   Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
   Subject: Re: [PATCH 0/5] Feature enhancements for ses module
  
   On 14-10-22 09:12 PM, Jens Axboe wrote:
   On 08/25/2014 11:34 AM, Song Liu wrote:
   From: Song Liu [mailto:songliubrav...@fb.com]
   Sent: Monday, August 25, 2014 10:26 AM
   To: Song Liu
   Subject: [PATCH 0/5] Feature enhancements for ses module
  
   These patches include a few enhancements to ses module:
  
   1: close potential race condition by at enclosure race condition
  
   2,3,4: add enclosure id and device slot, so we can create symlink
in /dev/disk/by-slot:
   # ls -d /dev/disk/by-slot/*
 /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
  
   5: add ability to power on/off device with power_status file in
sysfs.
  
   Had a rude awakening with sg_ses recently when setting a field in
   the enclosure control dpage. That is what is being done in point 5: 
   above.
  
   The time honoured technique is to read the corresponding enclosure
   status dpage, find the correct element, twiddle the field of
   interest, set the SELECT bit and write it back. The idea is
   maintain any other
  field settings in that element.
   And this is what the ses module does.
  
   There is at least one SES device out there that rejects the write
   if there are bits set in RESERVED locations. According to SPC-4 a
   device may do that. Look at the status (read) and control

RE: [PATCH 0/5] Feature enhancements for ses module

[Song Liu]

Hi Doug, 

I am not sure whether I fully understand the scenario. Actually patch 5 tries 
to clear all reserved bits:

+ dest_desc[0] = 0;
+ /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+ if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
+   dest_desc[1] = 0;
+ dest_desc[2] = 0xde;
+ dest_desc[3] = 0x3c;

Would this work for device that rejects request with 1 in RESERVED areas? 

Thanks,
Song

 -Original Message-
 From: Douglas Gilbert [mailto:dgilb...@interlog.com]
 Sent: Wednesday, October 22, 2014 3:29 PM
 To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
 Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
 Subject: Re: [PATCH 0/5] Feature enhancements for ses module
 
 On 14-10-22 09:12 PM, Jens Axboe wrote:
  On 08/25/2014 11:34 AM, Song Liu wrote:
  From: Song Liu [mailto:songliubrav...@fb.com]
  Sent: Monday, August 25, 2014 10:26 AM
  To: Song Liu
  Subject: [PATCH 0/5] Feature enhancements for ses module
 
  These patches include a few enhancements to ses module:
 
  1: close potential race condition by at enclosure race condition
 
  2,3,4: add enclosure id and device slot, so we can create symlink
  in /dev/disk/by-slot:
 # ls -d /dev/disk/by-slot/*
   /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
 
  5: add ability to power on/off device with power_status file in
  sysfs.
 
 Had a rude awakening with sg_ses recently when setting a field in the
 enclosure control dpage. That is what is being done in point 5: above.
 
 The time honoured technique is to read the corresponding enclosure status
 dpage, find the correct element, twiddle the field of interest, set the 
 SELECT bit
 and write it back. The idea is maintain any other field settings in that 
 element.
 And this is what the ses module does.
 
 There is at least one SES device out there that rejects the write if there 
 are bits
 set in RESERVED locations. According to SPC-4 a device may do that. Look at
 the status (read) and control (write) variants of each element type: many 
 times
 the control variant has less fields.
 
 To fix that the read-modify-write cycle needs to be changed to a read-mask-
 modify-write cycle where the mask is the allowable write mask (there would
 be one for each element type).
 
 This is ugly and may create problems in the future if and when
 T10 adds a new field in an element. About that time T10 should think about
 refining the meaning of RESERVED in SES to outlaw SES devices creating this
 particular time waster.
 
 Doug Gilbert
 
  Dan Williams (4):
 SES: close potential registration race
 SES: generate KOBJ_CHANGE on enclosure attach
 SES: add enclosure logical id
 SES: add reliable slot attribute
 
  Song Liu (1):
 SES: Add power_status to SES enclosure component
 
  Guys, where are we with these? Seemed like they got reviewed (and
  updates/fixes posted), then nothing else happened. Would be nice to
  get these upstream, so we don't have to carry them at FB.
 

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/5] Feature enhancements for ses module

[Song Liu]

Hi Doug, 

The power on/off field together with fault, locate, and active are for 
HDD (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other 
elements. So we are not dealing with power off duration, etc. here. 

Thanks,
Song

 -Original Message-
 From: Douglas Gilbert [mailto:dgilb...@interlog.com]
 Sent: Wednesday, October 22, 2014 6:17 PM
 To: Song Liu; Jens Axboe; linux-scsi@vger.kernel.org
 Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
 Subject: Re: [PATCH 0/5] Feature enhancements for ses module
 
 On 14-10-23 01:01 AM, Song Liu wrote:
  Hi Doug,
 
  I am not sure whether I fully understand the scenario. Actually patch 5 
  tries to
 clear all reserved bits:
 
  + dest_desc[0] = 0;
  + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if
  + (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
  +   dest_desc[1] = 0;
  + dest_desc[2] = 0xde;
  + dest_desc[3] = 0x3c;
 
  Would this work for device that rejects request with 1 in RESERVED areas?
 
 
 That is a pretty asymmetric element type, assuming we are talking about the
 enclosure control and enclosure status elements (i.e. etc=0xe). My guess
 would be:
 
  dest_desc[0] = 0x80 | (src_desc[0]  40);
  dest_desc[1] = 0x80  src_desc[1];
  dest_desc[2] = (pc_req  6) | pc_delay;
  dest_desc[3] = 0xff  src_desc[3];
 or if you have a new power_off_duration:
  dest_desc[3] = (power_off_duration  2) | (src_desc[3]  0x3);
 
 In byte 0 the top bit (SELECT) must be set or the enclosure will ignore any 
 other
 settings in that element. If the PRDFAIL bit is already set, then that 
 setting will
 be maintained.
 SES-3 has a note about clearing DISABLE and SWAP.
 
 In byte 1 is if the identifier (LED ?) is active (saying blinking) prior to 
 this power
 cycle request, then it will stay blinking until the power drops. If the 
 enclosure
 was really clever it might keep blinking after the power cycle :-)
 
 Also notice that the requested power cycle can be cancelled up to the time
 until power cycle drops to zero.
 
  -Original Message-
  From: Douglas Gilbert [mailto:dgilb...@interlog.com]
  Sent: Wednesday, October 22, 2014 3:29 PM
  To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org
  Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig
  Subject: Re: [PATCH 0/5] Feature enhancements for ses module
 
  On 14-10-22 09:12 PM, Jens Axboe wrote:
  On 08/25/2014 11:34 AM, Song Liu wrote:
  From: Song Liu [mailto:songliubrav...@fb.com]
  Sent: Monday, August 25, 2014 10:26 AM
  To: Song Liu
  Subject: [PATCH 0/5] Feature enhancements for ses module
 
  These patches include a few enhancements to ses module:
 
  1: close potential race condition by at enclosure race condition
 
  2,3,4: add enclosure id and device slot, so we can create symlink
   in /dev/disk/by-slot:
  # ls -d /dev/disk/by-slot/*
/dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0
 
  5: add ability to power on/off device with power_status file in
   sysfs.
 
  Had a rude awakening with sg_ses recently when setting a field in the
  enclosure control dpage. That is what is being done in point 5: above.
 
  The time honoured technique is to read the corresponding enclosure
  status dpage, find the correct element, twiddle the field of
  interest, set the SELECT bit and write it back. The idea is maintain any 
  other
 field settings in that element.
  And this is what the ses module does.
 
  There is at least one SES device out there that rejects the write if
  there are bits set in RESERVED locations. According to SPC-4 a device
  may do that. Look at the status (read) and control (write) variants
  of each element type: many times the control variant has less fields.
 
  To fix that the read-modify-write cycle needs to be changed to a
  read-mask- modify-write cycle where the mask is the allowable write
  mask (there would be one for each element type).
 
  This is ugly and may create problems in the future if and when
  T10 adds a new field in an element. About that time T10 should think
  about refining the meaning of RESERVED in SES to outlaw SES devices
  creating this particular time waster.
 
  Doug Gilbert
 
  Dan Williams (4):
  SES: close potential registration race
  SES: generate KOBJ_CHANGE on enclosure attach
  SES: add enclosure logical id
  SES: add reliable slot attribute
 
  Song Liu (1):
  SES: Add power_status to SES enclosure component
 
  Guys, where are we with these? Seemed like they got reviewed (and
  updates/fixes posted), then nothing else happened. Would be nice to
  get these upstream, so we don't have to carry them at FB.
 
 
  --
  To unsubscribe from this list: send the line unsubscribe linux-scsi
  in the body of a message to majord...@vger.kernel.org More majordomo
  info at
  https://urldefense.proofpoint.com/v1/url?u=http://vger.kernel.org/majo
  rdomo-
 info.htmlk=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0Ar=J3jGe56dIPfS5TN6DM
 
 82UYbbeR1j2viaiSJI40tv6lE%3D%0Am

RE: [PATCH 1/5] SES: close potential registration race

[Song Liu]

From 22d9dbcd783b315aaf59ad3eb928addaf0fba5fa Mon Sep 17 00:00:00 2001
From: Dan Williams dan.j.willi...@intel.com
Date: Thu, 21 Aug 2014 11:42:46 -0700
Subject: [PATCH 1/5] SES: close potential registration race

The slot and address fields have a small window of instability when
userspace can read them before initialization. Separate
enclosure_component
allocation from registration.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 36 +---
 drivers/scsi/ses.c| 21 ++---
 include/linux/enclosure.h |  5 +++--
 3 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 2cf2bbc..15faf61 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -249,27 +249,25 @@ static void enclosure_component_release(struct device 
*dev)
 static const struct attribute_group *enclosure_component_groups[];
 
 /**
- * enclosure_component_register - add a particular component to an enclosure
+ * enclosure_component_alloc - prepare a new enclosure component
  * @edev:  the enclosure to add the component
  * @num:   the device number
  * @type:  the type of component being added
  * @name:  an optional name to appear in sysfs (leave NULL if none)
  *
- * Registers the component.  The name is optional for enclosures that
- * give their components a unique name.  If not, leave the field NULL
- * and a name will be assigned.
+ * The name is optional for enclosures that give their components a unique
+ * name.  If not, leave the field NULL and a name will be assigned.
  *
  * Returns a pointer to the enclosure component or an error.
  */
 struct enclosure_component *
-enclosure_component_register(struct enclosure_device *edev,
-unsigned int number,
-enum enclosure_component_type type,
-const char *name)
+enclosure_component_alloc(struct enclosure_device *edev,
+ unsigned int number,
+ enum enclosure_component_type type,
+ const char *name)
 {
struct enclosure_component *ecomp;
struct device *cdev;
-   int err;
 
if (number = edev-components)
return ERR_PTR(-EINVAL);
@@ -291,14 +289,30 @@ enclosure_component_register(struct enclosure_device 
*edev,
cdev-release = enclosure_component_release;
cdev-groups = enclosure_component_groups;
 
+   return ecomp;
+}
+EXPORT_SYMBOL_GPL(enclosure_component_alloc);
+
+/**
+ * enclosure_component_register - publishes an initialized enclosure component
+ * @ecomp: component to add
+ *
+ * Returns 0 on successful registration, releases the component otherwise
+ */
+int enclosure_component_register(struct enclosure_component *ecomp)
+{
+   struct device *cdev;
+   int err;
+
+   cdev = ecomp-cdev;
err = device_register(cdev);
if (err) {
ecomp-number = -1;
put_device(cdev);
-   return ERR_PTR(err);
+   return err;
}
 
-   return ecomp;
+   return 0;
 }
 EXPORT_SYMBOL_GPL(enclosure_component_register);
 
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 80bfece..c52fd98 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -423,16 +423,23 @@ static void ses_enclosure_data_process(struct 
enclosure_device *edev,
type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
 
if (create)
-   ecomp = 
enclosure_component_register(edev,
-
components++,
-
type_ptr[0],
-
name);
+   ecomp = enclosure_component_alloc(
+   edev,
+   components++,
+   type_ptr[0],
+   name);
else
ecomp = edev-component[components++];
 
-   if (!IS_ERR(ecomp)  addl_desc_ptr)
-   ses_process_descriptor(ecomp,
-  addl_desc_ptr);
+   if (!IS_ERR(ecomp)) {
+   if (addl_desc_ptr)
+   ses_process_descriptor(
+   ecomp

RE: [PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach

[Song Liu]

From 741aa4b6043af741e28c0c414d48249f878353dc Mon Sep 17 00:00:00 2001
From: Dan Williams dan.j.willi...@intel.com
Date: Thu, 21 Aug 2014 11:43:22 -0700
Subject: [PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach

In support of a /dev/disk/by-slot populated with data from the enclosure
and ses modules udev needs notification when the new interface
files/links are available.  Otherwise, any udev rules specified for the
disk cannot assume that the enclosure topology has settled.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Reviewed-by: Hannes Reinecke h...@suse.de
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index c52fd98..87cf970b 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -349,7 +349,8 @@ static int ses_enclosure_find_by_addr(struct 
enclosure_device *edev,
if (scomp-addr != efd-addr)
continue;
 
-   enclosure_add_device(edev, i, efd-dev);
+   if (enclosure_add_device(edev, i, efd-dev) == 0)
+   kobject_uevent(efd-dev-kobj, KOBJ_CHANGE);
return 1;
}
return 0;
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/5] SES: add reliable slot attribute

[Song Liu]

From 1cb4f9b4c57425cc3462b9615ac5bd013f7f4a88 Mon Sep 17 00:00:00 2001
From: Dan Williams dan.j.willi...@intel.com
Date: Thu, 21 Aug 2014 11:43:26 -0700
Subject: [PATCH 4/5] SES: add reliable slot attribute

The name provided by firmware is in a vendor specific format, publish
the slot number to have a reliable mechanism for identifying slots
across firmware implementations.  If the enclosure does not provide a
slot number fallback to the component number which is guaranteed unique,
and usually mirrors the slot number.

Cleaned up the unused ses_component.desc in the process.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Reviewed-by: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 20 +++-
 drivers/scsi/ses.c| 17 -
 include/linux/enclosure.h |  1 +
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 18b87de..2e3eafa 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -145,8 +145,10 @@ enclosure_register(struct device *dev, const char *name, 
int components,
if (err)
goto err;
 
-   for (i = 0; i  components; i++)
+   for (i = 0; i  components; i++) {
edev-component[i].number = -1;
+   edev-component[i].slot = -1;
+   }
 
mutex_lock(container_list_lock);
list_add_tail(edev-node, container_list);
@@ -552,6 +554,20 @@ static ssize_t get_component_type(struct device *cdev,
return snprintf(buf, 40, %s\n, enclosure_type[ecomp-type]);
 }
 
+static ssize_t get_component_slot(struct device *cdev,
+ struct device_attribute *attr, char *buf)
+{
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int slot;
+
+   /* if the enclosure does not override then use 'number' as a stand-in */
+   if (ecomp-slot = 0)
+   slot = ecomp-slot;
+   else
+   slot = ecomp-number;
+
+   return snprintf(buf, 40, %d\n, slot);
+}
 
 static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
set_component_fault);
@@ -562,6 +578,7 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
+static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
dev_attr_fault.attr,
@@ -569,6 +586,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_active.attr,
dev_attr_locate.attr,
dev_attr_type.attr,
+   dev_attr_slot.attr,
NULL
 };
 ATTRIBUTE_GROUPS(enclosure_component);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 696d5d8..3e59a5d 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -47,7 +47,6 @@ struct ses_device {
 
 struct ses_component {
u64 addr;
-   unsigned char *desc;
 };
 
 static int ses_probe(struct device *dev)
@@ -307,19 +306,26 @@ static void ses_process_descriptor(struct 
enclosure_component *ecomp,
int invalid = desc[0]  0x80;
enum scsi_protocol proto = desc[0]  0x0f;
u64 addr = 0;
+   int slot = -1;
struct ses_component *scomp = ecomp-scratch;
unsigned char *d;
 
-   scomp-desc = desc;
-
if (invalid)
return;
 
switch (proto) {
+   case SCSI_PROTOCOL_FCP:
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
+   }
+   break;
case SCSI_PROTOCOL_SAS:
-   if (eip)
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
d = desc + 8;
-   else
+   } else
d = desc + 4;
/* only take the phy0 addr */
addr = (u64)d[12]  56 |
@@ -335,6 +341,7 @@ static void ses_process_descriptor(struct 
enclosure_component *ecomp,
/* FIXME: Need to add more protocols than just SAS */
break;
}
+   ecomp-slot = slot;
scomp-addr = addr;
 }
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 807622b..0f826c1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -92,6 +92,7 @@ struct enclosure_component {
int fault;
int active;
int locate;
+   int slot;
enum enclosure_status status;
 };
 
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] SES: add enclosure logical id

[Song Liu]

From 849dd255bdd2158cb621697e41448cf6b230d2f4 Mon Sep 17 00:00:00 2001
From: Dan Williams dan.j.willi...@intel.com
Date: Thu, 21 Aug 2014 11:43:24 -0700
Subject: [PATCH 3/5] SES: add enclosure logical id

Export the NAA logical id for the enclosure.  This is optionally
available from the sas_transport_class, but it is really a property of
the enclosure.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 13 +
 drivers/scsi/ses.c|  9 +
 include/linux/enclosure.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 15faf61..18b87de 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -395,8 +395,21 @@ static ssize_t components_show(struct device *cdev,
 }
 static DEVICE_ATTR_RO(components);
 
+static ssize_t id_show(struct device *cdev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev);
+
+   if (edev-cb-show_id)
+   return edev-cb-show_id(edev, buf);
+   return -EINVAL;
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *enclosure_class_attrs[] = {
dev_attr_components.attr,
+   dev_attr_id.attr,
NULL,
 };
 ATTRIBUTE_GROUPS(enclosure_class);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 87cf970b..696d5d8 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_device *edev,
return ses_set_page2_descriptor(edev, ecomp, desc);
 }
 
+static int ses_show_id(struct enclosure_device *edev, char *buf)
+{
+   struct ses_device *ses_dev = edev-scratch;
+   unsigned long long id = get_unaligned_be64(ses_dev-page1+8+4);
+
+   return sprintf(buf, %#llx\n, id);
+}
+
 static struct enclosure_component_callbacks ses_enclosure_callbacks = {
.get_fault  = ses_get_fault,
.set_fault  = ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks 
ses_enclosure_callbacks = {
.get_locate = ses_get_locate,
.set_locate = ses_set_locate,
.set_active = ses_set_active,
+   .show_id= ses_show_id,
 };
 
 struct ses_host_edev {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a835d33..807622b 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -79,6 +79,7 @@ struct enclosure_component_callbacks {
int (*set_locate)(struct enclosure_device *,
  struct enclosure_component *,
  enum enclosure_component_setting);
+   int (*show_id)(struct enclosure_device *, char *buf);
 };
 
 
-- 
1.8.1


 -Original Message-
 From: Hannes Reinecke [mailto:h...@suse.de]
 Sent: Thursday, September 4, 2014 12:55 AM
 To: Song Liu; linux-scsi@vger.kernel.org
 Cc: Dan Williams; Jens Axboe
 Subject: Re: [PATCH 3/5] SES: add enclosure logical id
 
 On 08/25/2014 07:34 PM, Song Liu wrote:
  From: Song Liu [mailto:songliubrav...@fb.com]
  Sent: Monday, August 25, 2014 10:26 AM
  To: Song Liu
  Cc: Dan Williams; Hannes Reinecke
  Subject: [PATCH 3/5] SES: add enclosure logical id
 
  From: Dan Williams dan.j.willi...@intel.com
 
  Export the NAA logical id for the enclosure.  This is optionally available
 from the sas_transport_class, but it is really a property of the enclosure.
 
  Signed-off-by: Dan Williams dan.j.willi...@intel.com
  Signed-off-by: Song Liu songliubrav...@fb.com
  Reviewed-by: Jens Axboe ax...@fb.com
  Cc: Hannes Reinecke h...@suse.de
  ---
   drivers/misc/enclosure.c  | 13 +
   drivers/scsi/ses.c|  9 +
   include/linux/enclosure.h |  1 +
   3 files changed, 23 insertions(+)
 
  diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index
  15faf61..646068a 100644
  --- a/drivers/misc/enclosure.c
  +++ b/drivers/misc/enclosure.c
  @@ -395,8 +395,21 @@ static ssize_t components_show(struct device
  *cdev,  }  static DEVICE_ATTR_RO(components);
 
  +static ssize_t id_show(struct device *cdev,
  +struct device_attribute *attr,
  +char *buf)
  +{
  +   struct enclosure_device *edev = to_enclosure_device(cdev);
  +
  +   if (edev-cb-show_id)
  +   return edev-cb-show_id(edev, buf);
  +   return 0;
  +}
  +static DEVICE_ATTR_RO(id);
  +
   static struct attribute *enclosure_class_attrs[] = {
  dev_attr_components.attr,
  +   dev_attr_id.attr,
  NULL,
   };
   ATTRIBUTE_GROUPS(enclosure_class);
 
 Maybe you should return -EINVAL or something here; '0' would mean an
 enclosure id of length '0', which is a different meaning from 'enclosure id 
 not
 available'.
 
  diff --git

RE: [PATCH 5/5] SES: Add power_status to SES enclosure component

[Song Liu]

New patch in next email...

Change to let power_status show on or off

Initialization of desc was removed because it will be initialized in 
init_device_slot_control, so no need to initialize at define.

Thanks,
Song

 -Original Message-
 From: Hannes Reinecke [mailto:h...@suse.de]
 Sent: Thursday, September 4, 2014 12:59 AM
 To: Song Liu; linux-scsi@vger.kernel.org
 Cc: Dan Williams; Jens Axboe
 Subject: Re: [PATCH 5/5] SES: Add power_status to SES enclosure component
 
 On 08/25/2014 07:34 PM, Song Liu wrote:
  From: Song Liu [mailto:songliubrav...@fb.com]
  Sent: Monday, August 25, 2014 10:26 AM
  To: Song Liu
  Cc: Hannes Reinecke
  Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component
 
  Add power_status to SES enclosure component, so we can power on/off
 the HDDs behind the enclosure.
 
  Check firmware status in ses_set_* before sending control pages to
 firmware.
 
  Signed-off-by: Song Liu songliubrav...@fb.com
  Acked-by: Dan Williams dan.j.willi...@intel.com
  Reviewed-by: Jens Axboe ax...@fb.com
  Cc: Hannes Reinecke h...@suse.de
  ---
   drivers/misc/enclosure.c  | 29 ++
   drivers/scsi/ses.c| 98
 ++-
   include/linux/enclosure.h |  6 +++
   3 files changed, 124 insertions(+), 9 deletions(-)
 
  diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index
  de335bc..0331dfe 100644
  --- a/drivers/misc/enclosure.c
  +++ b/drivers/misc/enclosure.c
  @@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char
 *name, int components,
  for (i = 0; i  components; i++) {
  edev-component[i].number = -1;
  edev-component[i].slot = -1;
  +   edev-component[i].power_status = 1;
  }
 
  mutex_lock(container_list_lock);
  @@ -546,6 +547,31 @@ static ssize_t set_component_locate(struct
 device *cdev,
  return count;
   }
 
  +static ssize_t get_component_power_status(struct device *cdev,
  + struct device_attribute *attr,
  + char *buf)
  +{
  +   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
  +   struct enclosure_component *ecomp =
 to_enclosure_component(cdev);
  +
  +   if (edev-cb-get_power_status)
  +   edev-cb-get_power_status(edev, ecomp);
  +   return snprintf(buf, 40, %d\n, ecomp-power_status); }
  +
  +static ssize_t set_component_power_status(struct device *cdev,
  + struct device_attribute *attr,
  + const char *buf, size_t count) {
  +   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
  +   struct enclosure_component *ecomp =
 to_enclosure_component(cdev);
  +   int val = simple_strtoul(buf, NULL, 0);
  +
  +   if (edev-cb-set_power_status)
  +   edev-cb-set_power_status(edev, ecomp, val);
  +   return count;
  +}
  +
 Just using a number here doesn't seem to be very instructive; what is this
 number? Can't we have a decode setting here to allow even the uninitiated
 to do some sensible decisions here?
 
   static ssize_t get_component_type(struct device *cdev,
struct device_attribute *attr, char *buf)
 { @@ -577,6 +603,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR,
 get_component_active,
 set_component_active);
   static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
 set_component_locate);
  +static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR,
 get_component_power_status,
  +  set_component_power_status);
   static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);  static
  DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
  @@ -585,6 +613,7 @@ static struct attribute
 *enclosure_component_attrs[] = {
  dev_attr_status.attr,
  dev_attr_active.attr,
  dev_attr_locate.attr,
  +   dev_attr_power_status.attr,
  dev_attr_type.attr,
  dev_attr_slot.attr,
  NULL
  diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index
  bafa301..ea6b262 100644
  --- a/drivers/scsi/ses.c
  +++ b/drivers/scsi/ses.c
  @@ -67,6 +67,20 @@ static int ses_probe(struct device *dev)  #define
  SES_TIMEOUT (30 * HZ)  #define SES_RETRIES 3
 
  +static void init_device_slot_control(unsigned char *dest_desc,
  +struct enclosure_component *ecomp,
  +unsigned char *status)
  +{
  +   memcpy(dest_desc, status, 4);
  +   dest_desc[0] = 0;
  +   /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
  +   if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
  +   dest_desc[1] = 0;
  +   dest_desc[2] = 0xde;
  +   dest_desc[3] = 0x3c;
  +}
  +
  +
   static int ses_recv_diag(struct scsi_device *sdev, int page_code,
   void *buf, int bufflen)
   {
  @@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device
 *edev,
struct

RE: [PATCH 5/5] SES: Add power_status to SES enclosure component

[Song Liu]

From 53549716d9f965e59f3a84feb5ebae9d18232b52 Mon Sep 17 00:00:00 2001
From: Song Liu songliubrav...@fb.com
Date: Tue, 19 Aug 2014 23:58:58 -0700
Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component

Add power_status to SES enclosure component, so we can power on/off the
HDDs behind the enclosure.

Check firmware status in ses_set_* before sending control pages to
firmware.

Signed-off-by: Song Liu songliubrav...@fb.com
Acked-by: Dan Williams dan.j.willi...@intel.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 38 ++
 drivers/scsi/ses.c| 98 ++-
 include/linux/enclosure.h |  6 +++
 3 files changed, 133 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 2e3eafa..819f2f2 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char *name, 
int components,
for (i = 0; i  components; i++) {
edev-component[i].number = -1;
edev-component[i].slot = -1;
+   edev-component[i].power_status = 1;
}
 
mutex_lock(container_list_lock);
@@ -546,6 +547,40 @@ static ssize_t set_component_locate(struct device *cdev,
return count;
 }
 
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+   if (edev-cb-get_power_status)
+   edev-cb-get_power_status(edev, ecomp);
+   return snprintf(buf, 40, %s\n, ecomp-power_status ? on : off);
+}
+
+static ssize_t set_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int val;
+
+   if (strncmp(buf, on, 2) == 0 
+   (buf[2] == '\n' || buf[2] == '\0'))
+   val = 1;
+   else if (strncmp(buf, off, 3) == 0 
+   (buf[3] == '\n' || buf[3] == '\0'))
+   val = 0;
+   else
+   return -EINVAL;
+
+   if (edev-cb-set_power_status)
+   edev-cb-set_power_status(edev, ecomp, val);
+   return count;
+}
+
 static ssize_t get_component_type(struct device *cdev,
  struct device_attribute *attr, char *buf)
 {
@@ -577,6 +612,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,
   set_component_active);
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
+  set_component_power_status);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
 static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
@@ -585,6 +622,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_status.attr,
dev_attr_active.attr,
dev_attr_locate.attr,
+   dev_attr_power_status.attr,
dev_attr_type.attr,
dev_attr_slot.attr,
NULL
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 3e59a5d..8ba3c78 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev)
 #define SES_TIMEOUT (30 * HZ)
 #define SES_RETRIES 3
 
+static void init_device_slot_control(unsigned char *dest_desc,
+struct enclosure_component *ecomp,
+unsigned char *status)
+{
+   memcpy(dest_desc, status, 4);
+   dest_desc[0] = 0;
+   /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+   if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
+   dest_desc[1] = 0;
+   dest_desc[2] = 0xde;
+   dest_desc[3] = 0x3c;
+}
+
+
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 void *buf, int bufflen)
 {
@@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device *edev,
  struct enclosure_component *ecomp,
 enum enclosure_component_setting val)
 {
-   unsigned char desc[4] = {0 };
+   unsigned char desc[4];
+   unsigned char *desc_ptr;
+
+   desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+
+   if (!desc_ptr)
+   return -EIO;
+
+   init_device_slot_control(desc, ecomp, desc_ptr);
 
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
-   /* zero

[PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach

[Song Liu]

From: Song Liu [mailto:songliubrav...@fb.com] 
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach

From: Dan Williams dan.j.willi...@intel.com

In support of a /dev/disk/by-slot populated with data from the enclosure and 
ses modules udev needs notification when the new interface files/links are 
available.  Otherwise, any udev rules specified for the disk cannot assume that 
the enclosure topology has settled.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index c2e8a98..8f0a62a 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -349,7 +349,8 @@ static int ses_enclosure_find_by_addr(struct 
enclosure_device *edev,
if (scomp-addr != efd-addr)
continue;
 
-   enclosure_add_device(edev, i, efd-dev);
+   if (enclosure_add_device(edev, i, efd-dev) == 0)
+   kobject_uevent(efd-dev-kobj, KOBJ_CHANGE);
return 1;
}
return 0;
--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] SES: add reliable slot attribute

[Song Liu]

From: Song Liu [mailto:songliubrav...@fb.com] 
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 4/5] SES: add reliable slot attribute

From: Dan Williams dan.j.willi...@intel.com

The name provided by firmware is in a vendor specific format, publish the slot 
number to have a reliable mechanism for identifying slots across firmware 
implementations.  If the enclosure does not provide a slot number fallback to 
the component number which is guaranteed unique, and usually mirrors the slot 
number.

Cleaned up the unused ses_component.desc in the process.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 20 +++-
 drivers/scsi/ses.c| 17 -
 include/linux/enclosure.h |  1 +
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
646068a..de335bc 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -145,8 +145,10 @@ enclosure_register(struct device *dev, const char *name, 
int components,
if (err)
goto err;
 
-   for (i = 0; i  components; i++)
+   for (i = 0; i  components; i++) {
edev-component[i].number = -1;
+   edev-component[i].slot = -1;
+   }
 
mutex_lock(container_list_lock);
list_add_tail(edev-node, container_list); @@ -552,6 +554,20 @@ 
static ssize_t get_component_type(struct device *cdev,
return snprintf(buf, 40, %s\n, enclosure_type[ecomp-type]);  }
 
+static ssize_t get_component_slot(struct device *cdev,
+ struct device_attribute *attr, char *buf) {
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int slot;
+
+   /* if the enclosure does not override then use 'number' as a stand-in */
+   if (ecomp-slot = 0)
+   slot = ecomp-slot;
+   else
+   slot = ecomp-number;
+
+   return snprintf(buf, 40, %d\n, slot); }
 
 static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
set_component_fault);
@@ -562,6 +578,7 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,  static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, 
get_component_locate,
   set_component_locate);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
+static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
 static struct attribute *enclosure_component_attrs[] = {
dev_attr_fault.attr,
@@ -569,6 +586,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_active.attr,
dev_attr_locate.attr,
dev_attr_type.attr,
+   dev_attr_slot.attr,
NULL
 };
 ATTRIBUTE_GROUPS(enclosure_component);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 61deb4e..bafa301 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -47,7 +47,6 @@ struct ses_device {
 
 struct ses_component {
u64 addr;
-   unsigned char *desc;
 };
 
 static int ses_probe(struct device *dev) @@ -307,19 +306,26 @@ static void 
ses_process_descriptor(struct enclosure_component *ecomp,
int invalid = desc[0]  0x80;
enum scsi_protocol proto = desc[0]  0x0f;
u64 addr = 0;
+   int slot = -1;
struct ses_component *scomp = ecomp-scratch;
unsigned char *d;
 
-   scomp-desc = desc;
-
if (invalid)
return;
 
switch (proto) {
+   case SCSI_PROTOCOL_FCP:
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
+   }
+   break;
case SCSI_PROTOCOL_SAS:
-   if (eip)
+   if (eip) {
+   d = desc + 4;
+   slot = d[3];
d = desc + 8;
-   else
+   } else
d = desc + 4;
/* only take the phy0 addr */
addr = (u64)d[12]  56 |
@@ -335,6 +341,7 @@ static void ses_process_descriptor(struct 
enclosure_component *ecomp,
/* FIXME: Need to add more protocols than just SAS */
break;
}
+   ecomp-slot = slot;
scomp-addr = addr;
 }
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h index 
807622b..0f826c1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -92,6 +92,7 @@ struct enclosure_component {
int fault;
int active;
int locate;
+   int slot;
enum enclosure_status status;
 };
 
--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] SES: Add power_status to SES enclosure component

[Song Liu]

From: Song Liu [mailto:songliubrav...@fb.com] 
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Hannes Reinecke
Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component

Add power_status to SES enclosure component, so we can power on/off the HDDs 
behind the enclosure.

Check firmware status in ses_set_* before sending control pages to firmware.

Signed-off-by: Song Liu songliubrav...@fb.com
Acked-by: Dan Williams dan.j.willi...@intel.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 29 ++
 drivers/scsi/ses.c| 98 ++-
 include/linux/enclosure.h |  6 +++
 3 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
de335bc..0331dfe 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char *name, 
int components,
for (i = 0; i  components; i++) {
edev-component[i].number = -1;
edev-component[i].slot = -1;
+   edev-component[i].power_status = 1;
}
 
mutex_lock(container_list_lock);
@@ -546,6 +547,31 @@ static ssize_t set_component_locate(struct device *cdev,
return count;
 }
 
+static ssize_t get_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+   if (edev-cb-get_power_status)
+   edev-cb-get_power_status(edev, ecomp);
+   return snprintf(buf, 40, %d\n, ecomp-power_status); }
+
+static ssize_t set_component_power_status(struct device *cdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev-parent);
+   struct enclosure_component *ecomp = to_enclosure_component(cdev);
+   int val = simple_strtoul(buf, NULL, 0);
+
+   if (edev-cb-set_power_status)
+   edev-cb-set_power_status(edev, ecomp, val);
+   return count;
+}
+
 static ssize_t get_component_type(struct device *cdev,
  struct device_attribute *attr, char *buf)  { 
@@ -577,6 +603,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, 
get_component_active,
   set_component_active);
 static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
   set_component_locate);
+static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
+  set_component_power_status);
 static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);  static 
DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
 
@@ -585,6 +613,7 @@ static struct attribute *enclosure_component_attrs[] = {
dev_attr_status.attr,
dev_attr_active.attr,
dev_attr_locate.attr,
+   dev_attr_power_status.attr,
dev_attr_type.attr,
dev_attr_slot.attr,
NULL
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index bafa301..ea6b262 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -67,6 +67,20 @@ static int ses_probe(struct device *dev)  #define 
SES_TIMEOUT (30 * HZ)  #define SES_RETRIES 3
 
+static void init_device_slot_control(unsigned char *dest_desc,
+struct enclosure_component *ecomp,
+unsigned char *status)
+{
+   memcpy(dest_desc, status, 4);
+   dest_desc[0] = 0;
+   /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
+   if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE)
+   dest_desc[1] = 0;
+   dest_desc[2] = 0xde;
+   dest_desc[3] = 0x3c;
+}
+
+
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 void *buf, int bufflen)
 {
@@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device *edev,
  struct enclosure_component *ecomp,
 enum enclosure_component_setting val)  {
-   unsigned char desc[4] = {0 };
+   unsigned char desc[4];
+   unsigned char *desc_ptr;
+
+   desc_ptr = ses_get_page2_descriptor(edev, ecomp);
+ 
+   if (!desc_ptr)
+   return -EIO;
+
+   init_device_slot_control(desc, ecomp, desc_ptr);
 
switch (val) {
case ENCLOSURE_SETTING_DISABLED:
-   /* zero is disabled */
+   desc[3] = 0xdf;
break;
case ENCLOSURE_SETTING_ENABLED:
-   desc[3] = 0x20;
+   desc[3] |= 0x20;
break;
default:
/* SES doesn't do the SGPIO blink settings */ @@ -219,14

[PATCH 0/5] Feature enhancements for ses module

[Song Liu]

From: Song Liu [mailto:songliubrav...@fb.com] 
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Subject: [PATCH 0/5] Feature enhancements for ses module

These patches include a few enhancements to ses module:

1: close potential race condition by at enclosure race condition

2,3,4: add enclosure id and device slot, so we can create symlink
   in /dev/disk/by-slot:
  # ls -d /dev/disk/by-slot/*
/dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0

5: add ability to power on/off device with power_status file in
   sysfs.

Dan Williams (4):
  SES: close potential registration race
  SES: generate KOBJ_CHANGE on enclosure attach
  SES: add enclosure logical id
  SES: add reliable slot attribute

Song Liu (1):
  SES: Add power_status to SES enclosure component

 drivers/misc/enclosure.c  |  98 +++
 drivers/scsi/ses.c| 145 +++---
 include/linux/enclosure.h |  13 -
 3 files changed, 220 insertions(+), 36 deletions(-)

-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] SES: add enclosure logical id

[Song Liu]

From: Song Liu [mailto:songliubrav...@fb.com] 
Sent: Monday, August 25, 2014 10:26 AM
To: Song Liu
Cc: Dan Williams; Hannes Reinecke
Subject: [PATCH 3/5] SES: add enclosure logical id

From: Dan Williams dan.j.willi...@intel.com

Export the NAA logical id for the enclosure.  This is optionally available from 
the sas_transport_class, but it is really a property of the enclosure.

Signed-off-by: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Song Liu songliubrav...@fb.com
Reviewed-by: Jens Axboe ax...@fb.com
Cc: Hannes Reinecke h...@suse.de
---
 drivers/misc/enclosure.c  | 13 +
 drivers/scsi/ses.c|  9 +
 include/linux/enclosure.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index 
15faf61..646068a 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -395,8 +395,21 @@ static ssize_t components_show(struct device *cdev,  }  
static DEVICE_ATTR_RO(components);
 
+static ssize_t id_show(struct device *cdev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct enclosure_device *edev = to_enclosure_device(cdev);
+
+   if (edev-cb-show_id)
+   return edev-cb-show_id(edev, buf);
+   return 0;
+}
+static DEVICE_ATTR_RO(id);
+
 static struct attribute *enclosure_class_attrs[] = {
dev_attr_components.attr,
+   dev_attr_id.attr,
NULL,
 };
 ATTRIBUTE_GROUPS(enclosure_class);
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 8f0a62a..61deb4e 
100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_device *edev,
return ses_set_page2_descriptor(edev, ecomp, desc);  }
 
+static int ses_show_id(struct enclosure_device *edev, char *buf) {
+   struct ses_device *ses_dev = edev-scratch;
+   unsigned long long id = get_unaligned_be64(ses_dev-page1+8+4);
+
+   return sprintf(buf, %#llx\n, id);
+}
+
 static struct enclosure_component_callbacks ses_enclosure_callbacks = {
.get_fault  = ses_get_fault,
.set_fault  = ses_set_fault,
@@ -265,6 +273,7 @@ static struct enclosure_component_callbacks 
ses_enclosure_callbacks = {
.get_locate = ses_get_locate,
.set_locate = ses_set_locate,
.set_active = ses_set_active,
+   .show_id= ses_show_id,
 };
 
 struct ses_host_edev {
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h index 
a835d33..807622b 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -79,6 +79,7 @@ struct enclosure_component_callbacks {
int (*set_locate)(struct enclosure_device *,
  struct enclosure_component *,
  enum enclosure_component_setting);
+   int (*show_id)(struct enclosure_device *, char *buf);
 };
 
 
--
1.8.1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html