[PATCH] hpsa: add heartbeat sysfs host attribute

[PATCH] hpsa: add heartbeat sysfs host attribute

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

The cciss driver had a CCISS_HEARTBEAT ioctl which
was not implemented in hpsa.  This ioctl returned a
counter from a register on the Smart Array which the
firmware would periodically update.  It can be used
to detect certain kinds of faults (e.g. controller
lockup) by noticing when the value remains constant
for more than a second or two.

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
---
 Documentation/scsi/hpsa.txt |    8 ++++++++
 drivers/scsi/hpsa.c         |   18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/Documentation/scsi/hpsa.txt b/Documentation/scsi/hpsa.txt
index 891435a..0dac1e0 100644
--- a/Documentation/scsi/hpsa.txt
+++ b/Documentation/scsi/hpsa.txt
@@ -47,6 +47,7 @@ HPSA specific entries in /sys
   /sys/class/scsi_host/host*/firmware_revision
   /sys/class/scsi_host/host*/resettable
   /sys/class/scsi_host/host*/transport_mode
+  /sys/class/scsi_host/host*/heartbeat
 
   the host "rescan" attribute is a write only attribute.  Writing to this
   attribute will cause the driver to scan for new, changed, or removed devices
@@ -78,6 +79,13 @@ HPSA specific entries in /sys
   kexec tools to warn the user if they attempt to designate a device which is
   unable to honor the reset_devices kernel parameter as a dump device.
 
+  The "heartbeat" read-only attribute returns the value of a heartbeat
+  counter register on a Smart Array controller as a 32 bit unsigned
+  hexadecimal integer (e.g: "0x12345678").  The value should change
+  periodically, not less than once per second.  If this value fails to
+  change for a period longer than one second, it means something has
+  gone wrong (e.g. Smart Array controller firmware has locked up.)
+
   HPSA specific disk attributes:
   ------------------------------
 
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index b200b73..ce6bde4 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -340,6 +340,21 @@ static ssize_t host_show_resettable(struct device *dev,
 	return snprintf(buf, 20, "%d\n", ctlr_is_resettable(h->board_id));
 }
 
+static ssize_t host_show_heartbeat(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct ctlr_info *h;
+	struct Scsi_Host *shost = class_to_shost(dev);
+	u32 heartbeat;
+	unsigned long flags;
+
+	h = shost_to_hba(shost);
+	spin_lock_irqsave(&h->lock, flags);
+	heartbeat = readl(&h->cfgtable->HeartBeat);
+	spin_unlock_irqrestore(&h->lock, flags);
+	return snprintf(buf, 20, "0x%08x\n", heartbeat);
+}
+
 static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
 {
 	return (scsi3addr[3] & 0xC0) == 0x40;
@@ -448,6 +463,8 @@ static DEVICE_ATTR(transport_mode, S_IRUGO,
 	host_show_transport_mode, NULL);
 static DEVICE_ATTR(resettable, S_IRUGO,
 	host_show_resettable, NULL);
+static DEVICE_ATTR(heartbeat, S_IRUGO,
+	host_show_heartbeat, NULL);
 
 static struct device_attribute *hpsa_sdev_attrs[] = {
 	&dev_attr_raid_level,
@@ -462,6 +479,7 @@ static struct device_attribute *hpsa_shost_attrs[] = {
 	&dev_attr_commands_outstanding,
 	&dev_attr_transport_mode,
 	&dev_attr_resettable,
+	&dev_attr_heartbeat,
 	NULL,
 };
 

Re: [PATCH] hpsa: add heartbeat sysfs host attribute

[Greg KH]

On Fri, Aug 12, 2011 at 01:03:14PM -0500, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> The cciss driver had a CCISS_HEARTBEAT ioctl which
> was not implemented in hpsa.  This ioctl returned a
> counter from a register on the Smart Array which the
> firmware would periodically update.  It can be used
> to detect certain kinds of faults (e.g. controller
> lockup) by noticing when the value remains constant
> for more than a second or two.
> 
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> ---
>  Documentation/scsi/hpsa.txt |    8 ++++++++
>  drivers/scsi/hpsa.c         |   18 ++++++++++++++++++
>  2 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/scsi/hpsa.txt b/Documentation/scsi/hpsa.txt
> index 891435a..0dac1e0 100644
> --- a/Documentation/scsi/hpsa.txt
> +++ b/Documentation/scsi/hpsa.txt
> @@ -47,6 +47,7 @@ HPSA specific entries in /sys
>    /sys/class/scsi_host/host*/firmware_revision
>    /sys/class/scsi_host/host*/resettable
>    /sys/class/scsi_host/host*/transport_mode
> +  /sys/class/scsi_host/host*/heartbeat
>  
>    the host "rescan" attribute is a write only attribute.  Writing to this
>    attribute will cause the driver to scan for new, changed, or removed devices
> @@ -78,6 +79,13 @@ HPSA specific entries in /sys
>    kexec tools to warn the user if they attempt to designate a device which is
>    unable to honor the reset_devices kernel parameter as a dump device.
>  
> +  The "heartbeat" read-only attribute returns the value of a heartbeat
> +  counter register on a Smart Array controller as a 32 bit unsigned
> +  hexadecimal integer (e.g: "0x12345678").  The value should change
> +  periodically, not less than once per second.  If this value fails to
> +  change for a period longer than one second, it means something has
> +  gone wrong (e.g. Smart Array controller firmware has locked up.)
> +

This all belongs in Documentation/ABI/ care to move it there instead?

greg k-h

Re: [PATCH] hpsa: add heartbeat sysfs host attribute

[scameron]

On Fri, Aug 12, 2011 at 07:01:47PM -0700, Greg KH wrote:
> On Fri, Aug 12, 2011 at 01:03:14PM -0500, Stephen M. Cameron wrote:
> > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > 
> > The cciss driver had a CCISS_HEARTBEAT ioctl which
[...]
> >  
> > +  The "heartbeat" read-only attribute returns the value of a heartbeat
> > +  counter register on a Smart Array controller as a 32 bit unsigned
> > +  hexadecimal integer (e.g: "0x12345678").  The value should change
> > +  periodically, not less than once per second.  If this value fails to
> > +  change for a period longer than one second, it means something has
> > +  gone wrong (e.g. Smart Array controller firmware has locked up.)
> > +
> 
> This all belongs in Documentation/ABI/ care to move it there instead?

Sure.

-- steve

Re: [PATCH] hpsa: add heartbeat sysfs host attribute

[scameron]

On Mon, Aug 15, 2011 at 09:40:22AM -0500, scameron@beardog.cce.hp.com wrote:
> On Fri, Aug 12, 2011 at 07:01:47PM -0700, Greg KH wrote:
> > On Fri, Aug 12, 2011 at 01:03:14PM -0500, Stephen M. Cameron wrote:
> > > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > > 
> > > The cciss driver had a CCISS_HEARTBEAT ioctl which
> [...]
> > >  
> > > +  The "heartbeat" read-only attribute returns the value of a heartbeat
> > > +  counter register on a Smart Array controller as a 32 bit unsigned
> > > +  hexadecimal integer (e.g: "0x12345678").  The value should change
> > > +  periodically, not less than once per second.  If this value fails to
> > > +  change for a period longer than one second, it means something has
> > > +  gone wrong (e.g. Smart Array controller firmware has locked up.)
> > > +
> > 
> > This all belongs in Documentation/ABI/ care to move it there instead?
> 
> Sure.

Now that I look, I'm not seeing where any of the generic scsi
host or device attributes are documented in Documenation/ABI.

It's not very clear to me where in Documentation/ABI I should
document a driver's custom scsi device and host attributes as
even the generic scsi device and host attributes seem to be
undocumented.

from scsi_sysfs.c, I see these, for example:

        &dev_attr_device_blocked.attr,
        &dev_attr_type.attr,
        &dev_attr_scsi_level.attr,
        &dev_attr_vendor.attr,
        &dev_attr_model.attr,
        &dev_attr_rev.attr,
        &dev_attr_rescan.attr,
        &dev_attr_delete.attr,
        &dev_attr_state.attr,
        &dev_attr_timeout.attr,
        &dev_attr_iocounterbits.attr,
        &dev_attr_iorequest_cnt.attr,
        &dev_attr_iodone_cnt.attr,
        &dev_attr_ioerr_cnt.attr,
        &dev_attr_modalias.attr,
        REF_EVT(media_change),
        NULL
};

but I don't see them documented anywhere in Documentation/ABI,
and hpsa.c adds these custom sdev attrs:

static struct device_attribute *hpsa_sdev_attrs[] = {
        &dev_attr_raid_level,
        &dev_attr_lunid,
        &dev_attr_unique_id,
        NULL,
};


Likewise, there are generic scsi host attrs in scsi_sysfs.c:

static struct attribute *scsi_sysfs_shost_attrs[] = {
        &dev_attr_unique_id.attr,
        &dev_attr_host_busy.attr,
        &dev_attr_cmd_per_lun.attr,
        &dev_attr_can_queue.attr,
        &dev_attr_sg_tablesize.attr,
        &dev_attr_sg_prot_tablesize.attr,
        &dev_attr_unchecked_isa_dma.attr,
        &dev_attr_proc_name.attr,
        &dev_attr_scan.attr,
        &dev_attr_hstate.attr,
        &dev_attr_supported_mode.attr,
        &dev_attr_active_mode.attr,
        &dev_attr_prot_capabilities.attr,
        &dev_attr_prot_guard_type.attr,
        NULL

Also undocumented.

hpsa.c adds custom host attrs:
static struct device_attribute *hpsa_shost_attrs[] = {
        &dev_attr_rescan,
        &dev_attr_firmware_revision,
        &dev_attr_commands_outstanding,
        &dev_attr_transport_mode,
        &dev_attr_resettable,
        &dev_attr_heartbeat,
        NULL,
};


How should I proceed?

-- steve

Re: [PATCH] hpsa: add heartbeat sysfs host attribute

[Greg KH]

On Mon, Aug 15, 2011 at 10:04:09AM -0500, scameron@beardog.cce.hp.com wrote:
> On Mon, Aug 15, 2011 at 09:40:22AM -0500, scameron@beardog.cce.hp.com wrote:
> > On Fri, Aug 12, 2011 at 07:01:47PM -0700, Greg KH wrote:
> > > On Fri, Aug 12, 2011 at 01:03:14PM -0500, Stephen M. Cameron wrote:
> > > > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > > > 
> > > > The cciss driver had a CCISS_HEARTBEAT ioctl which
> > [...]
> > > >  
> > > > +  The "heartbeat" read-only attribute returns the value of a heartbeat
> > > > +  counter register on a Smart Array controller as a 32 bit unsigned
> > > > +  hexadecimal integer (e.g: "0x12345678").  The value should change
> > > > +  periodically, not less than once per second.  If this value fails to
> > > > +  change for a period longer than one second, it means something has
> > > > +  gone wrong (e.g. Smart Array controller firmware has locked up.)
> > > > +
> > > 
> > > This all belongs in Documentation/ABI/ care to move it there instead?
> > 
> > Sure.
> 
> Now that I look, I'm not seeing where any of the generic scsi
> host or device attributes are documented in Documenation/ABI.

Then that's a SCSI bug :)

> It's not very clear to me where in Documentation/ABI I should
> document a driver's custom scsi device and host attributes as
> even the generic scsi device and host attributes seem to be
> undocumented.
> 
> from scsi_sysfs.c, I see these, for example:
> 
>         &dev_attr_device_blocked.attr,
>         &dev_attr_type.attr,
>         &dev_attr_scsi_level.attr,
>         &dev_attr_vendor.attr,
>         &dev_attr_model.attr,
>         &dev_attr_rev.attr,
>         &dev_attr_rescan.attr,
>         &dev_attr_delete.attr,
>         &dev_attr_state.attr,
>         &dev_attr_timeout.attr,
>         &dev_attr_iocounterbits.attr,
>         &dev_attr_iorequest_cnt.attr,
>         &dev_attr_iodone_cnt.attr,
>         &dev_attr_ioerr_cnt.attr,
>         &dev_attr_modalias.attr,
>         REF_EVT(media_change),
>         NULL
> };
> 
> but I don't see them documented anywhere in Documentation/ABI,
> and hpsa.c adds these custom sdev attrs:
> 
> static struct device_attribute *hpsa_sdev_attrs[] = {
>         &dev_attr_raid_level,
>         &dev_attr_lunid,
>         &dev_attr_unique_id,
>         NULL,
> };
> 
> 
> Likewise, there are generic scsi host attrs in scsi_sysfs.c:
> 
> static struct attribute *scsi_sysfs_shost_attrs[] = {
>         &dev_attr_unique_id.attr,
>         &dev_attr_host_busy.attr,
>         &dev_attr_cmd_per_lun.attr,
>         &dev_attr_can_queue.attr,
>         &dev_attr_sg_tablesize.attr,
>         &dev_attr_sg_prot_tablesize.attr,
>         &dev_attr_unchecked_isa_dma.attr,
>         &dev_attr_proc_name.attr,
>         &dev_attr_scan.attr,
>         &dev_attr_hstate.attr,
>         &dev_attr_supported_mode.attr,
>         &dev_attr_active_mode.attr,
>         &dev_attr_prot_capabilities.attr,
>         &dev_attr_prot_guard_type.attr,
>         NULL
> 
> Also undocumented.
> 
> hpsa.c adds custom host attrs:
> static struct device_attribute *hpsa_shost_attrs[] = {
>         &dev_attr_rescan,
>         &dev_attr_firmware_revision,
>         &dev_attr_commands_outstanding,
>         &dev_attr_transport_mode,
>         &dev_attr_resettable,
>         &dev_attr_heartbeat,
>         NULL,
> };
> 
> 
> How should I proceed?

Start by documenting the ones you add, and if you want, and can, it
would be great to document the existing ones as well.

thanks,

greg k-h