diff mbox series

[RFC,v2,1/3] Add support for seven more indicators in enclosure driver

Message ID d71f2483ed6cb6ea74a267301982c14e6bfa79a7.1643822289.git.stuart.w.hayes@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series Add PCIe enclosure management support | expand

Commit Message

stuart hayes Feb. 2, 2022, 5:59 p.m. UTC 
This patch adds support for seven additional indicators (ok, rebuild,
prdfail, hotspare, ica, ifa, disabled) to the enclosure driver, which
currently only supports three (fault, active, locate). It also reduces
duplicated code for the set and show functions for the sysfs attributes
for all of the indicators, and allows users of the driver to provide
common get_led and set_led callbacks to control all indicators (though
it continues to support the existing callbacks for the three currently
supported indicators, so it does not require any changes to code that
already uses the enclosure driver).

This will be used by the pcie_em driver.

Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
---
 .../ABI/testing/sysfs-class-enclosure         |  14 ++
 drivers/misc/enclosure.c                      | 191 +++++++++++-------
 include/linux/enclosure.h                     |  22 ++
 3 files changed, 149 insertions(+), 78 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-enclosure

Comments

Bjorn Helgaas Feb. 2, 2022, 11:55 p.m. UTC | #1 
Follow subject line convention (also applies to other patches):

  $ git log --oneline drivers/misc/enclosure.c
  714f1af14bb0 misc: enclosure: replace snprintf in show functions with sysfs_emit
  6a57251c70a4 misc: enclosure: Update enclosure_remove_device() documentation to match reality
  82f5b473d91a misc: enclosure: Fix some kerneldoc anomalies
  529244bd1afc scsi: enclosure: Fix stale device oops with hot replug
  e3575c1201f0 misc: enclosure: Use struct_size() in kzalloc()
  750b54deb569 misc: enclosure: Remove unnecessary error check

I don't think "seven" is really relevant for the subject or even the
commit log since you list them all anyway.

On Wed, Feb 02, 2022 at 11:59:11AM -0600, Stuart Hayes wrote:
> This patch adds support for seven additional indicators (ok, rebuild,
> prdfail, hotspare, ica, ifa, disabled) to the enclosure driver, which
> currently only supports three (fault, active, locate). It also reduces
> duplicated code for the set and show functions for the sysfs attributes
> for all of the indicators, and allows users of the driver to provide
> common get_led and set_led callbacks to control all indicators (though
> it continues to support the existing callbacks for the three currently
> supported indicators, so it does not require any changes to code that
> already uses the enclosure driver).

This restructures things (replacing get_component_fault(),
set_component_fault(), etc with attributes) and adds things (the new
indicators and also maybe the .get_led() and .set_led() hooks).  It
would be nice to split all those into separate patches.

Also consider using imperative mood:
  https://chris.beams.io/posts/git-commit/
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134

A few more minor comments below.

> @@ -0,0 +1,14 @@
> +What:		/sys/class/enclosure/<enclosure>/<component>/rebuild
> +What:		/sys/class/enclosure/<enclosure>/<component>/disabled
> +What:		/sys/class/enclosure/<enclosure>/<component>/hotspare
> +What:		/sys/class/enclosure/<enclosure>/<component>/ica
> +What:		/sys/class/enclosure/<enclosure>/<component>/ifa
> +What:		/sys/class/enclosure/<enclosure>/<component>/ok
> +What:		/sys/class/enclosure/<enclosure>/<component>/prdfail
> +Date:		February 2022
> +Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
> +Description:
> +		(RW) Get or set the indicator (1 is on, 0 is off) that
> +		corresponds to the attribute name, for a component in an
> +		enclosure. The "ica" and "ifa" states are "in critical
> +		array" and "in failed array".

What's "prdfail"?

> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 1b010d9267c9..485d6a3c655b 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -473,30 +473,6 @@ static const char *const enclosure_type[] = {
>  	[ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
>  };
>  
> -static ssize_t get_component_fault(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_fault)
> -		edev->cb->get_fault(edev, ecomp);
> -	return sysfs_emit(buf, "%d\n", ecomp->fault);
> -}
> -
> -static ssize_t set_component_fault(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_fault)
> -		edev->cb->set_fault(edev, ecomp, val);
> -	return count;
> -}
> -
>  static ssize_t get_component_status(struct device *cdev,
>  				    struct device_attribute *attr,char *buf)
>  {
> @@ -531,54 +507,6 @@ static ssize_t set_component_status(struct device *cdev,
>  		return -EINVAL;
>  }
>  
> -static ssize_t get_component_active(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_active)
> -		edev->cb->get_active(edev, ecomp);
> -	return sysfs_emit(buf, "%d\n", ecomp->active);
> -}
> -
> -static ssize_t set_component_active(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_active)
> -		edev->cb->set_active(edev, ecomp, val);
> -	return count;
> -}
> -
> -static ssize_t get_component_locate(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_locate)
> -		edev->cb->get_locate(edev, ecomp);
> -	return sysfs_emit(buf, "%d\n", ecomp->locate);
> -}
> -
> -static ssize_t set_component_locate(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_locate)
> -		edev->cb->set_locate(edev, ecomp, val);
> -	return count;
> -}
> -
>  static ssize_t get_component_power_status(struct device *cdev,
>  					  struct device_attribute *attr,
>  					  char *buf)
> @@ -641,29 +569,136 @@ static ssize_t get_component_slot(struct device *cdev,
>  	return sysfs_emit(buf, "%d\n", slot);
>  }
>  
> -static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
> -		    set_component_fault);
> +/*
> + * callbacks for attrs using enum enclosure_component_setting (LEDs)
> + */
> +static ssize_t led_show(struct device *cdev,
> +			enum enclosure_component_led led,
> +			char *buf)
> +{
> +	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> +	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +
> +	if (edev->cb->get_led)
> +		edev->cb->get_led(edev, ecomp, led);
> +	else

The switch is technically one statement, but it's so long that I think
it really needs braces around it.

> +		/*
> +		 * support old callbacks for fault/active/locate
> +		 */
> +		switch (led) {
> +		case ENCLOSURE_LED_FAULT:
> +			if (edev->cb->get_fault) {
> +				edev->cb->get_fault(edev, ecomp);
> +				ecomp->led[led] = ecomp->fault;
> +			}
> +			break;
> +		case ENCLOSURE_LED_ACTIVE:
> +			if (edev->cb->get_active) {
> +				edev->cb->get_active(edev, ecomp);
> +				ecomp->led[led] = ecomp->active;
> +			}
> +			break;
> +		case ENCLOSURE_LED_LOCATE:
> +			if (edev->cb->get_locate) {
> +				edev->cb->get_locate(edev, ecomp);
> +				ecomp->led[led] = ecomp->locate;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +
> +	return sysfs_emit(buf, "%d\n", ecomp->led[led]);
> +}
> +
> +static ssize_t led_set(struct device *cdev,
> +		       enum enclosure_component_led led,
> +		       const char *buf, size_t count)
> +{
> +	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> +	struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +	int err, val;
> +
> +	err = kstrtoint(buf, 0, &val);
> +	if (err)
> +		return err;
> +
> +	if (edev->cb->set_led)
> +		edev->cb->set_led(edev, ecomp, led, val);
> +	else

Same here.  Or you could just return early for the cb->set_led case
and unindent the below.

> +		/*
> +		 * support old callbacks for fault/active/locate
> +		 */
> +		switch (led) {
> +		case ENCLOSURE_LED_FAULT:
> +			if (edev->cb->set_fault)
> +				edev->cb->set_fault(edev, ecomp, val);
> +			break;
> +		case ENCLOSURE_LED_ACTIVE:
> +			if (edev->cb->set_active)
> +				edev->cb->set_active(edev, ecomp, val);
> +			break;
> +		case ENCLOSURE_LED_LOCATE:
> +			if (edev->cb->set_locate)
> +				edev->cb->set_locate(edev, ecomp, val);
> +			break;
> +		default:
> +			break;
> +		}

I guess you rely on the callbacks to set ecomp->led[led] (or
ecomp->fault, etc)?

Maybe do the "ecomp->led[led] = ecomp->fault" bits here instead of in
led_show() so that crashdumps will show the same info regardless of
whether led_show() has been run?  ...

Oh, never mind, I see that you only ever update led[led] in the
.get_led() callbacks.  In that case, why do you even *store* the
result in led[led]?  Couldn't you just return it from .get_led()?

> +	return count;
> +}
> +
> +#define LED_ATTR_RW(led_attr, led)					\
> +static ssize_t led_attr##_show(struct device *cdev,			\
> +			       struct device_attribute *attr,		\
> +			       char *buf)				\
> +{									\
> +	return led_show(cdev, led, buf);				\
> +}									\
> +static ssize_t led_attr##_store(struct device *cdev,			\
> +				struct device_attribute *attr,		\
> +				const char *buf, size_t count)		\
> +{									\
> +	return led_set(cdev, led, buf, count);				\
> +}									\
> +static DEVICE_ATTR_RW(led_attr)
> +
>  static DEVICE_ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
>  		   set_component_status);
> -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);
> +LED_ATTR_RW(fault, ENCLOSURE_LED_FAULT);
> +LED_ATTR_RW(active, ENCLOSURE_LED_ACTIVE);
> +LED_ATTR_RW(locate, ENCLOSURE_LED_LOCATE);
> +LED_ATTR_RW(ok, ENCLOSURE_LED_OK);
> +LED_ATTR_RW(rebuild, ENCLOSURE_LED_REBUILD);
> +LED_ATTR_RW(prdfail, ENCLOSURE_LED_PRDFAIL);
> +LED_ATTR_RW(hotspare, ENCLOSURE_LED_HOTSPARE);
> +LED_ATTR_RW(ica, ENCLOSURE_LED_ICA);
> +LED_ATTR_RW(ifa, ENCLOSURE_LED_IFA);
> +LED_ATTR_RW(disabled, ENCLOSURE_LED_DISABLED);
>  
>  static struct attribute *enclosure_component_attrs[] = {
>  	&dev_attr_fault.attr,
>  	&dev_attr_status.attr,
>  	&dev_attr_active.attr,
>  	&dev_attr_locate.attr,
> +	&dev_attr_ok.attr,
> +	&dev_attr_rebuild.attr,
> +	&dev_attr_prdfail.attr,
> +	&dev_attr_hotspare.attr,
> +	&dev_attr_ica.attr,
> +	&dev_attr_ifa.attr,
> +	&dev_attr_disabled.attr,
>  	&dev_attr_power_status.attr,
>  	&dev_attr_type.attr,
>  	&dev_attr_slot.attr,
>  	NULL
>  };
> +

Spurious diff?  I see both with and without this blank line, but seems
more common without.

>  ATTRIBUTE_GROUPS(enclosure_component);

>  
>  static int __init enclosure_init(void)
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index 1c630e2c2756..98dd0f05efb7 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -49,6 +49,20 @@ enum enclosure_component_setting {
>  	ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
>  };
>  
> +enum enclosure_component_led {
> +	ENCLOSURE_LED_FAULT,
> +	ENCLOSURE_LED_ACTIVE,
> +	ENCLOSURE_LED_LOCATE,
> +	ENCLOSURE_LED_OK,
> +	ENCLOSURE_LED_REBUILD,
> +	ENCLOSURE_LED_PRDFAIL,
> +	ENCLOSURE_LED_HOTSPARE,
> +	ENCLOSURE_LED_ICA,
> +	ENCLOSURE_LED_IFA,
> +	ENCLOSURE_LED_DISABLED,
> +	ENCLOSURE_LED_MAX,
> +};
> +
>  struct enclosure_device;
>  struct enclosure_component;
>  struct enclosure_component_callbacks {
> @@ -72,6 +86,13 @@ struct enclosure_component_callbacks {
>  	int (*set_locate)(struct enclosure_device *,
>  			  struct enclosure_component *,
>  			  enum enclosure_component_setting);
> +	void (*get_led)(struct enclosure_device *edev,
> +			struct enclosure_component *ecomp,
> +			enum enclosure_component_led);
> +	int (*set_led)(struct enclosure_device *edev,
> +		       struct enclosure_component *ecomp,
> +		       enum enclosure_component_led,
> +		       enum enclosure_component_setting);
>  	void (*get_power_status)(struct enclosure_device *,
>  				 struct enclosure_component *);
>  	int (*set_power_status)(struct enclosure_device *,
> @@ -90,6 +111,7 @@ struct enclosure_component {
>  	int fault;
>  	int active;
>  	int locate;
> +	int led[ENCLOSURE_LED_MAX];
>  	int slot;
>  	enum enclosure_status status;
>  	int power_status;
> -- 
> 2.31.1
>
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-enclosure b/Documentation/ABI/testing/sysfs-class-enclosure
new file mode 100644
index 000000000000..25d91e42d768
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-enclosure
@@ -0,0 +1,14 @@ 
+What:		/sys/class/enclosure/<enclosure>/<component>/rebuild
+What:		/sys/class/enclosure/<enclosure>/<component>/disabled
+What:		/sys/class/enclosure/<enclosure>/<component>/hotspare
+What:		/sys/class/enclosure/<enclosure>/<component>/ica
+What:		/sys/class/enclosure/<enclosure>/<component>/ifa
+What:		/sys/class/enclosure/<enclosure>/<component>/ok
+What:		/sys/class/enclosure/<enclosure>/<component>/prdfail
+Date:		February 2022
+Contact:	Stuart Hayes <stuart.w.hayes@gmail.com>
+Description:
+		(RW) Get or set the indicator (1 is on, 0 is off) that
+		corresponds to the attribute name, for a component in an
+		enclosure. The "ica" and "ifa" states are "in critical
+		array" and "in failed array".
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 1b010d9267c9..485d6a3c655b 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -473,30 +473,6 @@  static const char *const enclosure_type[] = {
 	[ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
 };
 
-static ssize_t get_component_fault(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_fault)
-		edev->cb->get_fault(edev, ecomp);
-	return sysfs_emit(buf, "%d\n", ecomp->fault);
-}
-
-static ssize_t set_component_fault(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_fault)
-		edev->cb->set_fault(edev, ecomp, val);
-	return count;
-}
-
 static ssize_t get_component_status(struct device *cdev,
 				    struct device_attribute *attr,char *buf)
 {
@@ -531,54 +507,6 @@  static ssize_t set_component_status(struct device *cdev,
 		return -EINVAL;
 }
 
-static ssize_t get_component_active(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_active)
-		edev->cb->get_active(edev, ecomp);
-	return sysfs_emit(buf, "%d\n", ecomp->active);
-}
-
-static ssize_t set_component_active(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_active)
-		edev->cb->set_active(edev, ecomp, val);
-	return count;
-}
-
-static ssize_t get_component_locate(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_locate)
-		edev->cb->get_locate(edev, ecomp);
-	return sysfs_emit(buf, "%d\n", ecomp->locate);
-}
-
-static ssize_t set_component_locate(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_locate)
-		edev->cb->set_locate(edev, ecomp, val);
-	return count;
-}
-
 static ssize_t get_component_power_status(struct device *cdev,
 					  struct device_attribute *attr,
 					  char *buf)
@@ -641,29 +569,136 @@  static ssize_t get_component_slot(struct device *cdev,
 	return sysfs_emit(buf, "%d\n", slot);
 }
 
-static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
-		    set_component_fault);
+/*
+ * callbacks for attrs using enum enclosure_component_setting (LEDs)
+ */
+static ssize_t led_show(struct device *cdev,
+			enum enclosure_component_led led,
+			char *buf)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+	if (edev->cb->get_led)
+		edev->cb->get_led(edev, ecomp, led);
+	else
+		/*
+		 * support old callbacks for fault/active/locate
+		 */
+		switch (led) {
+		case ENCLOSURE_LED_FAULT:
+			if (edev->cb->get_fault) {
+				edev->cb->get_fault(edev, ecomp);
+				ecomp->led[led] = ecomp->fault;
+			}
+			break;
+		case ENCLOSURE_LED_ACTIVE:
+			if (edev->cb->get_active) {
+				edev->cb->get_active(edev, ecomp);
+				ecomp->led[led] = ecomp->active;
+			}
+			break;
+		case ENCLOSURE_LED_LOCATE:
+			if (edev->cb->get_locate) {
+				edev->cb->get_locate(edev, ecomp);
+				ecomp->led[led] = ecomp->locate;
+			}
+			break;
+		default:
+			break;
+		}
+
+	return sysfs_emit(buf, "%d\n", ecomp->led[led]);
+}
+
+static ssize_t led_set(struct device *cdev,
+		       enum enclosure_component_led led,
+		       const char *buf, size_t count)
+{
+	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	int err, val;
+
+	err = kstrtoint(buf, 0, &val);
+	if (err)
+		return err;
+
+	if (edev->cb->set_led)
+		edev->cb->set_led(edev, ecomp, led, val);
+	else
+		/*
+		 * support old callbacks for fault/active/locate
+		 */
+		switch (led) {
+		case ENCLOSURE_LED_FAULT:
+			if (edev->cb->set_fault)
+				edev->cb->set_fault(edev, ecomp, val);
+			break;
+		case ENCLOSURE_LED_ACTIVE:
+			if (edev->cb->set_active)
+				edev->cb->set_active(edev, ecomp, val);
+			break;
+		case ENCLOSURE_LED_LOCATE:
+			if (edev->cb->set_locate)
+				edev->cb->set_locate(edev, ecomp, val);
+			break;
+		default:
+			break;
+		}
+
+	return count;
+}
+
+#define LED_ATTR_RW(led_attr, led)					\
+static ssize_t led_attr##_show(struct device *cdev,			\
+			       struct device_attribute *attr,		\
+			       char *buf)				\
+{									\
+	return led_show(cdev, led, buf);				\
+}									\
+static ssize_t led_attr##_store(struct device *cdev,			\
+				struct device_attribute *attr,		\
+				const char *buf, size_t count)		\
+{									\
+	return led_set(cdev, led, buf, count);				\
+}									\
+static DEVICE_ATTR_RW(led_attr)
+
 static DEVICE_ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
 		   set_component_status);
-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);
+LED_ATTR_RW(fault, ENCLOSURE_LED_FAULT);
+LED_ATTR_RW(active, ENCLOSURE_LED_ACTIVE);
+LED_ATTR_RW(locate, ENCLOSURE_LED_LOCATE);
+LED_ATTR_RW(ok, ENCLOSURE_LED_OK);
+LED_ATTR_RW(rebuild, ENCLOSURE_LED_REBUILD);
+LED_ATTR_RW(prdfail, ENCLOSURE_LED_PRDFAIL);
+LED_ATTR_RW(hotspare, ENCLOSURE_LED_HOTSPARE);
+LED_ATTR_RW(ica, ENCLOSURE_LED_ICA);
+LED_ATTR_RW(ifa, ENCLOSURE_LED_IFA);
+LED_ATTR_RW(disabled, ENCLOSURE_LED_DISABLED);
 
 static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_fault.attr,
 	&dev_attr_status.attr,
 	&dev_attr_active.attr,
 	&dev_attr_locate.attr,
+	&dev_attr_ok.attr,
+	&dev_attr_rebuild.attr,
+	&dev_attr_prdfail.attr,
+	&dev_attr_hotspare.attr,
+	&dev_attr_ica.attr,
+	&dev_attr_ifa.attr,
+	&dev_attr_disabled.attr,
 	&dev_attr_power_status.attr,
 	&dev_attr_type.attr,
 	&dev_attr_slot.attr,
 	NULL
 };
+
 ATTRIBUTE_GROUPS(enclosure_component);
 
 static int __init enclosure_init(void)
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 1c630e2c2756..98dd0f05efb7 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -49,6 +49,20 @@  enum enclosure_component_setting {
 	ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
 };
 
+enum enclosure_component_led {
+	ENCLOSURE_LED_FAULT,
+	ENCLOSURE_LED_ACTIVE,
+	ENCLOSURE_LED_LOCATE,
+	ENCLOSURE_LED_OK,
+	ENCLOSURE_LED_REBUILD,
+	ENCLOSURE_LED_PRDFAIL,
+	ENCLOSURE_LED_HOTSPARE,
+	ENCLOSURE_LED_ICA,
+	ENCLOSURE_LED_IFA,
+	ENCLOSURE_LED_DISABLED,
+	ENCLOSURE_LED_MAX,
+};
+
 struct enclosure_device;
 struct enclosure_component;
 struct enclosure_component_callbacks {
@@ -72,6 +86,13 @@  struct enclosure_component_callbacks {
 	int (*set_locate)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
+	void (*get_led)(struct enclosure_device *edev,
+			struct enclosure_component *ecomp,
+			enum enclosure_component_led);
+	int (*set_led)(struct enclosure_device *edev,
+		       struct enclosure_component *ecomp,
+		       enum enclosure_component_led,
+		       enum enclosure_component_setting);
 	void (*get_power_status)(struct enclosure_device *,
 				 struct enclosure_component *);
 	int (*set_power_status)(struct enclosure_device *,
@@ -90,6 +111,7 @@  struct enclosure_component {
 	int fault;
 	int active;
 	int locate;
+	int led[ENCLOSURE_LED_MAX];
 	int slot;
 	enum enclosure_status status;
 	int power_status;