The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Signed-off-by: James Bottomley <[email protected]>
---
See the additional ses patch for SCSI enclosure services users of this.
---
drivers/misc/Kconfig | 10 +
drivers/misc/Makefile | 1 +
drivers/misc/enclosure.c | 449 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/enclosure.h | 120 ++++++++++++
4 files changed, 580 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/enclosure.c
create mode 100644 include/linux/enclosure.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b5e67c0..c6e5c09 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -11,6 +11,7 @@ menuconfig MISC_DEVICES
If you say N, all options in this submenu will be skipped and disabled.
+
if MISC_DEVICES
config IBM_ASM
@@ -232,4 +233,13 @@ config ATMEL_SSC
If unsure, say N.
+config ENCLOSURE_SERVICES
+ tristate "Enclosure Services"
+ default n
+ help
+ Provides support for intelligent enclosures (bays which
+ contain storage devices). You also need either a host
+ driver (SCSI/ATA) which supports enclosures
+ or a SCSI enclosure device (SES) to use these services.
+
endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 87f2685..de9f1f5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o
obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
+obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
\ No newline at end of file
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
new file mode 100644
index 0000000..e4683cd
--- /dev/null
+++ b/drivers/misc/enclosure.c
@@ -0,0 +1,449 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley <[email protected]>
+ *
+**-----------------------------------------------------------------------------
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-----------------------------------------------------------------------------
+*/
+#include <linux/device.h>
+#include <linux/enclosure.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+static LIST_HEAD(container_list);
+static DEFINE_MUTEX(container_list_lock);
+static struct class enclosure_class;
+static struct class enclosure_component_class;
+
+/**
+ * enclosure_find - find an enclosure given a device
+ * @dev: the device to find for
+ *
+ * Looks through the list of registered enclosures to see
+ * if it can find a match for a device. Returns NULL if no
+ * enclosure is found.
+ */
+struct enclosure_device *enclosure_find(struct device *dev)
+{
+ struct enclosure_device *edev = NULL;
+
+ mutex_lock(&container_list_lock);
+ list_for_each_entry(edev, &container_list, node) {
+ if (edev->cdev.dev == dev) {
+ mutex_unlock(&container_list_lock);
+ return edev;
+ }
+ }
+ mutex_unlock(&container_list_lock);
+
+ return NULL;
+}
+EXPORT_SYMBOL(enclosure_find);
+
+/**
+ * enclosure_for_each_device - calls a function for each enclosure
+ * @fn: the function to call
+ * @data: the data to pass to each call
+ *
+ * Loops over all the enclosures calling the function.
+ *
+ * Note, this function uses a mutex which will be held across calls to
+ * @fn, so it must have user context, and @fn should not sleep or
+ * otherwise cause the mutex to be held for indefinite periods
+ */
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data)
+{
+ int error = 0;
+ struct enclosure_device *edev;
+
+ mutex_lock(&container_list_lock);
+ list_for_each_entry(edev, &container_list, node) {
+ error = fn(edev, data);
+ if (error)
+ break;
+ }
+ mutex_unlock(&container_list_lock);
+
+ return error;
+}
+EXPORT_SYMBOL_GPL(enclosure_for_each_device);
+
+/**
+ * enclosure_register - register device as an enclosure
+ *
+ * @dev: device containing the enclosure
+ * @components: number of components in the enclosure
+ *
+ * This sets up the device for being an enclosure. Note that @dev does
+ * not have to be a dedicated enclosure device. It may be some other type
+ * of device that additionally responds to enclosure services
+ */
+struct enclosure_device *
+enclosure_register(struct device *dev, const char *name, int components,
+ struct enclosure_component_callbacks *cb)
+{
+ struct enclosure_device *edev =
+ kzalloc(sizeof(struct enclosure_device) +
+ sizeof(struct enclosure_component)*components,
+ GFP_KERNEL);
+ int err, i;
+
+ if (!edev)
+ return ERR_PTR(-ENOMEM);
+
+ if (!cb)
+ return ERR_PTR(-EINVAL);
+
+ edev->components = components;
+
+ edev->cdev.class = &enclosure_class;
+ edev->cdev.dev = get_device(dev);
+ edev->cb = cb;
+ snprintf(edev->cdev.class_id, BUS_ID_SIZE, "%s", name);
+ err = class_device_register(&edev->cdev);
+ if (err)
+ goto err;
+
+ for (i = 0; i < components; i++)
+ edev->component[i].number = -1;
+
+ mutex_lock(&container_list_lock);
+ list_add_tail(&edev->node, &container_list);
+ mutex_unlock(&container_list_lock);
+
+ return edev;
+
+ err:
+ put_device(edev->cdev.dev);
+ kfree(edev);
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(enclosure_register);
+
+static struct enclosure_component_callbacks enclosure_null_callbacks;
+
+/**
+ * enclosure_unregister - remove an enclosure
+ *
+ * @edev: the registered enclosure to remove;
+ */
+void enclosure_unregister(struct enclosure_device *edev)
+{
+ int i;
+
+ if (!edev)
+ return;
+
+ mutex_lock(&container_list_lock);
+ list_del(&edev->node);
+ mutex_unlock(&container_list_lock);
+
+ for (i = 0; i < edev->components; i++)
+ if (edev->component[i].number != -1)
+ class_device_unregister(&edev->component[i].cdev);
+
+ /* prevent any callbacks into service user */
+ edev->cb = &enclosure_null_callbacks;
+ class_device_unregister(&edev->cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_unregister);
+
+static void enclosure_release(struct class_device *cdev)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev);
+
+ put_device(cdev->dev);
+ kfree(edev);
+}
+
+static void enclosure_component_release(struct class_device *cdev)
+{
+ if (cdev->dev)
+ put_device(cdev->dev);
+ class_device_put(cdev->parent);
+}
+
+struct enclosure_component *
+enclosure_component_register(struct enclosure_device *edev,
+ unsigned int number,
+ enum enclosure_component_type type,
+ const char *name)
+{
+ struct enclosure_component *ecomp;
+ struct class_device *cdev;
+ int err;
+
+ if (!edev || number >= edev->components)
+ return ERR_PTR(-EINVAL);
+
+ ecomp = &edev->component[number];
+
+ if (ecomp->number != -1)
+ return ERR_PTR(-EINVAL);
+
+ ecomp->type = type;
+ ecomp->number = number;
+ cdev = &ecomp->cdev;
+ cdev->parent = class_device_get(&edev->cdev);
+ cdev->class = &enclosure_component_class;
+ if (name)
+ snprintf(cdev->class_id, BUS_ID_SIZE, "%s", name);
+ else
+ snprintf(cdev->class_id, BUS_ID_SIZE, "%d", number);
+
+ err = class_device_register(cdev);
+ if (err)
+ ERR_PTR(err);
+
+ return ecomp;
+}
+EXPORT_SYMBOL_GPL(enclosure_component_register);
+
+int enclosure_add_device(struct enclosure_device *edev, int component,
+ struct device *dev)
+{
+ struct class_device *cdev;
+
+ if (!edev || component >= edev->components)
+ return -EINVAL;
+
+ cdev = &edev->component[component].cdev;
+
+ class_device_del(cdev);
+ if (cdev->dev)
+ put_device(cdev->dev);
+ cdev->dev = get_device(dev);
+ return class_device_add(cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_add_device);
+
+int enclosure_remove_device(struct enclosure_device *edev, int component)
+{
+ struct class_device *cdev;
+
+ if (!edev || component >= edev->components)
+ return -EINVAL;
+
+ cdev = &edev->component[component].cdev;
+
+ class_device_del(cdev);
+ if (cdev->dev)
+ put_device(cdev->dev);
+ cdev->dev = NULL;
+ return class_device_add(cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_remove_device);
+
+/*
+ * sysfs pieces below
+ */
+
+static ssize_t enclosure_show_components(struct class_device *cdev, char *buf)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev);
+
+ return snprintf(buf, 40, "%d\n", edev->components);
+}
+
+static struct class_device_attribute enclosure_attrs[] = {
+ __ATTR(components, S_IRUGO, enclosure_show_components, NULL),
+ __ATTR_NULL
+};
+
+static struct class enclosure_class = {
+ .name = "enclosure",
+ .owner = THIS_MODULE,
+ .release = enclosure_release,
+ .class_dev_attrs = enclosure_attrs,
+};
+
+static char *enclosure_status [] = {
+ [ENCLOSURE_STATUS_UNSUPPORTED] = "unsupported",
+ [ENCLOSURE_STATUS_OK] = "OK",
+ [ENCLOSURE_STATUS_CRITICAL] = "critical",
+ [ENCLOSURE_STATUS_NON_CRITICAL] = "non-critical",
+ [ENCLOSURE_STATUS_UNRECOVERABLE] = "unrecoverable",
+ [ENCLOSURE_STATUS_NOT_INSTALLED] = "not installed",
+ [ENCLOSURE_STATUS_UNKNOWN] = "unknown",
+ [ENCLOSURE_STATUS_UNAVAILABLE] = "unavailable",
+};
+
+static char *enclosure_type [] = {
+ [ENCLOSURE_COMPONENT_DEVICE] = "device",
+ [ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
+};
+
+static ssize_t get_component_fault(struct class_device *cdev, 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 snprintf(buf, 40, "%d\n", ecomp->fault);
+}
+
+static ssize_t set_component_fault(struct class_device *cdev, 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 class_device *cdev, char *buf)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+ if (edev->cb->get_status)
+ edev->cb->get_status(edev, ecomp);
+ return snprintf(buf, 40, "%s\n", enclosure_status[ecomp->status]);
+}
+
+static ssize_t set_component_status(struct class_device *cdev, const char *buf,
+ size_t count)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+ int i;
+
+ for (i = 0; enclosure_status[i]; i++) {
+ if (strncmp(buf, enclosure_status[i],
+ strlen(enclosure_status[i])) == 0 &&
+ buf[strlen(enclosure_status[i])] == '\n')
+ break;
+ }
+
+ if (enclosure_status[i] && edev->cb->set_status) {
+ edev->cb->set_status(edev, ecomp, i);
+ return count;
+ } else
+ return -EINVAL;
+}
+
+static ssize_t get_component_active(struct class_device *cdev, 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 snprintf(buf, 40, "%d\n", ecomp->active);
+}
+
+static ssize_t set_component_active(struct class_device *cdev, 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 class_device *cdev, 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 snprintf(buf, 40, "%d\n", ecomp->locate);
+}
+
+static ssize_t set_component_locate(struct class_device *cdev, 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_type(struct class_device *cdev, char *buf)
+{
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+ return snprintf(buf, 40, "%s\n", enclosure_type[ecomp->type]);
+}
+
+
+static struct class_device_attribute enclosure_component_attrs[] = {
+ __ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
+ set_component_fault),
+ __ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
+ set_component_status),
+ __ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
+ set_component_active),
+ __ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
+ set_component_locate),
+ __ATTR(type, S_IRUGO, get_component_type, NULL),
+ __ATTR_NULL
+};
+
+static struct class enclosure_component_class = {
+ .name = "enclosure_component",
+ .owner = THIS_MODULE,
+ .class_dev_attrs = enclosure_component_attrs,
+ .release = enclosure_component_release,
+};
+
+static int __init enclosure_init(void)
+{
+ int err;
+
+ err = class_register(&enclosure_class);
+ if (err)
+ return err;
+ err = class_register(&enclosure_component_class);
+ if (err)
+ goto err_out;
+
+ return 0;
+ err_out:
+ class_unregister(&enclosure_class);
+
+ return err;
+}
+
+static void __exit enclosure_exit(void)
+{
+ class_unregister(&enclosure_component_class);
+ class_unregister(&enclosure_class);
+}
+
+module_init(enclosure_init);
+module_exit(enclosure_exit);
+
+MODULE_AUTHOR("James Bottomley");
+MODULE_DESCRIPTION("Enclosure Services");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
new file mode 100644
index 0000000..622177a
--- /dev/null
+++ b/include/linux/enclosure.h
@@ -0,0 +1,120 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley <[email protected]>
+ *
+**-----------------------------------------------------------------------------
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-----------------------------------------------------------------------------
+*/
+#ifndef _LINUX_ENCLOSURE_H_
+#define _LINUX_ENCLOSURE_H_
+
+#include <linux/device.h>
+#include <linux/list.h>
+
+/* A few generic types ... taken from ses-2 */
+enum enclosure_component_type {
+ ENCLOSURE_COMPONENT_DEVICE = 0x01,
+ ENCLOSURE_COMPONENT_ARRAY_DEVICE = 0x17,
+};
+
+/* ses-2 common element status */
+enum enclosure_status {
+ ENCLOSURE_STATUS_UNSUPPORTED = 0,
+ ENCLOSURE_STATUS_OK,
+ ENCLOSURE_STATUS_CRITICAL,
+ ENCLOSURE_STATUS_NON_CRITICAL,
+ ENCLOSURE_STATUS_UNRECOVERABLE,
+ ENCLOSURE_STATUS_NOT_INSTALLED,
+ ENCLOSURE_STATUS_UNKNOWN,
+ ENCLOSURE_STATUS_UNAVAILABLE,
+};
+
+/* SFF-8485 activity light settings */
+enum enclosure_component_setting {
+ ENCLOSURE_SETTING_DISABLED = 0,
+ ENCLOSURE_SETTING_ENABLED = 1,
+ ENCLOSURE_SETTING_BLINK_A_ON_OFF = 2,
+ ENCLOSURE_SETTING_BLINK_A_OFF_ON = 3,
+ ENCLOSURE_SETTING_BLINK_B_ON_OFF = 6,
+ ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
+};
+
+struct enclosure_device;
+struct enclosure_component;
+struct enclosure_component_callbacks {
+ void (*get_status)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_status)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_status);
+ void (*get_fault)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_fault)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_component_setting);
+ void (*get_active)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_active)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_component_setting);
+ void (*get_locate)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_locate)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_component_setting);
+};
+
+
+struct enclosure_component {
+ void *scratch;
+ struct class_device cdev;
+ enum enclosure_component_type type;
+ int number;
+ int fault;
+ int active;
+ int locate;
+ enum enclosure_status status;
+};
+
+struct enclosure_device {
+ void *scratch;
+ struct list_head node;
+ struct class_device cdev;
+ struct enclosure_component_callbacks *cb;
+ int components;
+ struct enclosure_component component[0];
+};
+
+#define to_enclosure_device(x) container_of((x), struct enclosure_device, cdev)
+#define to_enclosure_component(x) container_of((x), struct enclosure_component, cdev)
+
+struct enclosure_device *
+enclosure_register(struct device *, const char *, int,
+ struct enclosure_component_callbacks *);
+void enclosure_unregister(struct enclosure_device *);
+struct enclosure_component *
+enclosure_component_register(struct enclosure_device *, unsigned int,
+ enum enclosure_component_type, const char *);
+int enclosure_add_device(struct enclosure_device *enclosure, int component,
+ struct device *dev);
+int enclosure_remove_device(struct enclosure_device *enclosure, int component);
+struct enclosure_device *enclosure_find(struct device *dev);
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data);
+
+#endif /* _LINUX_ENCLOSURE_H_ */
--
1.5.3.8
Hi James.
Nitpicking only.
Sam
> The enclosure misc device is really just a library providing sysfs
> support for physical enclosure devices and their components.
>
> Signed-off-by: James Bottomley <[email protected]>
> ---
>
> See the additional ses patch for SCSI enclosure services users of this.
>
> ---
> drivers/misc/Kconfig | 10 +
> drivers/misc/Makefile | 1 +
> drivers/misc/enclosure.c | 449 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/enclosure.h | 120 ++++++++++++
> 4 files changed, 580 insertions(+), 0 deletions(-)
> create mode 100644 drivers/misc/enclosure.c
> create mode 100644 include/linux/enclosure.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b5e67c0..c6e5c09 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -11,6 +11,7 @@ menuconfig MISC_DEVICES
>
> If you say N, all options in this submenu will be skipped and disabled.
>
> +
> if MISC_DEVICES
Unrelated change.
>
> config IBM_ASM
> @@ -232,4 +233,13 @@ config ATMEL_SSC
>
> If unsure, say N.
>
> +config ENCLOSURE_SERVICES
> + tristate "Enclosure Services"
> + default n
Not needed. n is default.
> + help
> + Provides support for intelligent enclosures (bays which
> + contain storage devices). You also need either a host
> + driver (SCSI/ATA) which supports enclosures
> + or a SCSI enclosure device (SES) to use these services.
> +
> endif # MISC_DEVICES
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 87f2685..de9f1f5 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o
> obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o
> obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
> obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
> +obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
> \ No newline at end of file
Can we get one..
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> new file mode 100644
> index 0000000..e4683cd
> --- /dev/null
> +++ b/drivers/misc/enclosure.c
> @@ -0,0 +1,449 @@
> +/*
> + * Enclosure Services
> + *
> + * Copyright (C) 2008 James Bottomley <[email protected]>
> + *
> +**-----------------------------------------------------------------------------
> +**
> +** This program is free software; you can redistribute it and/or
> +** modify it under the terms of the GNU General Public License
> +** version 2 as published by the Free Software Foundation.
> +**
> +** This program is distributed in the hope that it will be useful,
> +** but WITHOUT ANY WARRANTY; without even the implied warranty of
> +** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +** GNU General Public License for more details.
> +**
> +** You should have received a copy of the GNU General Public License
> +** along with this program; if not, write to the Free Software
> +** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +**
> +**-----------------------------------------------------------------------------
> +*/
> +#include <linux/device.h>
> +#include <linux/enclosure.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +static LIST_HEAD(container_list);
> +static DEFINE_MUTEX(container_list_lock);
> +static struct class enclosure_class;
> +static struct class enclosure_component_class;
> +
> +/**
> + * enclosure_find - find an enclosure given a device
> + * @dev: the device to find for
> + *
> + * Looks through the list of registered enclosures to see
> + * if it can find a match for a device. Returns NULL if no
> + * enclosure is found.
> + */
> +struct enclosure_device *enclosure_find(struct device *dev)
> +{
> + struct enclosure_device *edev = NULL;
> +
> + mutex_lock(&container_list_lock);
> + list_for_each_entry(edev, &container_list, node) {
> + if (edev->cdev.dev == dev) {
> + mutex_unlock(&container_list_lock);
> + return edev;
> + }
> + }
> + mutex_unlock(&container_list_lock);
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL(enclosure_find);
No GPL - but the other of your EXPORT's are GPL.
> +
> +/**
> + * enclosure_for_each_device - calls a function for each enclosure
> + * @fn: the function to call
> + * @data: the data to pass to each call
> + *
> + * Loops over all the enclosures calling the function.
> + *
> + * Note, this function uses a mutex which will be held across calls to
> + * @fn, so it must have user context, and @fn should not sleep or
> + * otherwise cause the mutex to be held for indefinite periods
> + */
> +int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
> + void *data)
> +{
> + int error = 0;
> + struct enclosure_device *edev;
> +
> + mutex_lock(&container_list_lock);
> + list_for_each_entry(edev, &container_list, node) {
> + error = fn(edev, data);
> + if (error)
> + break;
> + }
> + mutex_unlock(&container_list_lock);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(enclosure_for_each_device);
> +
> +/**
> + * enclosure_register - register device as an enclosure
> + *
> + * @dev: device containing the enclosure
> + * @components: number of components in the enclosure
> + *
> + * This sets up the device for being an enclosure. Note that @dev does
> + * not have to be a dedicated enclosure device. It may be some other type
> + * of device that additionally responds to enclosure services
> + */
> +struct enclosure_device *
> +enclosure_register(struct device *dev, const char *name, int components,
> + struct enclosure_component_callbacks *cb)
style purists would request you to put return type and function
name on the same line...
> +{
> + struct enclosure_device *edev =
> + kzalloc(sizeof(struct enclosure_device) +
> + sizeof(struct enclosure_component)*components,
> + GFP_KERNEL);
> + int err, i;
> +
> + if (!edev)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!cb)
> + return ERR_PTR(-EINVAL);
We leak memory here - we never free edev.
> +
> + edev->components = components;
> +
> + edev->cdev.class = &enclosure_class;
> + edev->cdev.dev = get_device(dev);
> + edev->cb = cb;
> + snprintf(edev->cdev.class_id, BUS_ID_SIZE, "%s", name);
> + err = class_device_register(&edev->cdev);
> + if (err)
> + goto err;
> +
> + for (i = 0; i < components; i++)
> + edev->component[i].number = -1;
> +
> + mutex_lock(&container_list_lock);
> + list_add_tail(&edev->node, &container_list);
> + mutex_unlock(&container_list_lock);
> +
> + return edev;
> +
> + err:
> + put_device(edev->cdev.dev);
> + kfree(edev);
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(enclosure_register);
> +
> +static struct enclosure_component_callbacks enclosure_null_callbacks;
> +
> +/**
> + * enclosure_unregister - remove an enclosure
> + *
> + * @edev: the registered enclosure to remove;
> + */
> +void enclosure_unregister(struct enclosure_device *edev)
> +{
> + int i;
> +
> + if (!edev)
> + return;
> +
> + mutex_lock(&container_list_lock);
> + list_del(&edev->node);
> + mutex_unlock(&container_list_lock);
> +
> + for (i = 0; i < edev->components; i++)
> + if (edev->component[i].number != -1)
> + class_device_unregister(&edev->component[i].cdev);
> +
> + /* prevent any callbacks into service user */
> + edev->cb = &enclosure_null_callbacks;
> + class_device_unregister(&edev->cdev);
> +}
> +EXPORT_SYMBOL_GPL(enclosure_unregister);
> +
> +static void enclosure_release(struct class_device *cdev)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev);
> +
> + put_device(cdev->dev);
> + kfree(edev);
> +}
> +
> +static void enclosure_component_release(struct class_device *cdev)
> +{
> + if (cdev->dev)
> + put_device(cdev->dev);
> + class_device_put(cdev->parent);
> +}
> +
> +struct enclosure_component *
> +enclosure_component_register(struct enclosure_device *edev,
> + unsigned int number,
> + enum enclosure_component_type type,
> + const char *name)
> +{
Missing kernel-doc for this and the following exports.
> + struct enclosure_component *ecomp;
> + struct class_device *cdev;
> + int err;
> +
> + if (!edev || number >= edev->components)
> + return ERR_PTR(-EINVAL);
> +
> + ecomp = &edev->component[number];
> +
> + if (ecomp->number != -1)
> + return ERR_PTR(-EINVAL);
> +
> + ecomp->type = type;
> + ecomp->number = number;
> + cdev = &ecomp->cdev;
> + cdev->parent = class_device_get(&edev->cdev);
> + cdev->class = &enclosure_component_class;
> + if (name)
> + snprintf(cdev->class_id, BUS_ID_SIZE, "%s", name);
> + else
> + snprintf(cdev->class_id, BUS_ID_SIZE, "%d", number);
> +
> + err = class_device_register(cdev);
> + if (err)
> + ERR_PTR(err);
> +
> + return ecomp;
> +}
> +EXPORT_SYMBOL_GPL(enclosure_component_register);
On Sun, 2008-02-03 at 23:03 +0100, Sam Ravnborg wrote:
> Hi James.
>
> Nitpicking only.
>
> Sam
Thanks for the review.
> > +
> > if MISC_DEVICES
>
> Unrelated change.
Yes, removed it.
> > +config ENCLOSURE_SERVICES
> > + tristate "Enclosure Services"
> > + default n
> Not needed. n is default.
It is now ... plus there are a few other entries in this file with
default n.
> > +obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
> > \ No newline at end of file
> Can we get one..
added
> > +EXPORT_SYMBOL(enclosure_find);
>
> No GPL - but the other of your EXPORT's are GPL.
Done ... Greg gets annoyed about these.
> > +struct enclosure_device *
> > +enclosure_register(struct device *dev, const char *name, int components,
> > + struct enclosure_component_callbacks *cb)
> style purists would request you to put return type and function
> name on the same line...
I tend to prefer the function name always at the beginning of the line.
But even style purists have to agree it's better than trying to futilely
squash all the arguments on separate lines because the return complex
type is quite long.
> + struct enclosure_device *edev =
> > + kzalloc(sizeof(struct enclosure_device) +
> > + sizeof(struct enclosure_component)*components,
> > + GFP_KERNEL);
> > + int err, i;
> > +
> > + if (!edev)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (!cb)
> > + return ERR_PTR(-EINVAL);
> We leak memory here - we never free edev.
We do indeed .. fixed.
> > +struct enclosure_component *
> > +enclosure_component_register(struct enclosure_device *edev,
> > + unsigned int number,
> > + enum enclosure_component_type type,
> > + const char *name)
> > +{
> Missing kernel-doc for this and the following exports.
Yes, added.
James
---
From: James Bottomley <[email protected]>
Date: Sun, 3 Feb 2008 15:40:56 -0600
Subject: [SCSI] enclosure: add support for enclosure services
The enclosure misc device is really just a library providing sysfs
support for physical enclosure devices and their components.
Signed-off-by: James Bottomley <[email protected]>
---
drivers/misc/Kconfig | 9 +
drivers/misc/Makefile | 1 +
drivers/misc/enclosure.c | 486 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/enclosure.h | 120 +++++++++++
4 files changed, 616 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/enclosure.c
create mode 100644 include/linux/enclosure.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b5e67c0..ca68480 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -232,4 +232,13 @@ config ATMEL_SSC
If unsure, say N.
+config ENCLOSURE_SERVICES
+ tristate "Enclosure Services"
+ default n
+ help
+ Provides support for intelligent enclosures (bays which
+ contain storage devices). You also need either a host
+ driver (SCSI/ATA) which supports enclosures
+ or a SCSI enclosure device (SES) to use these services.
+
endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 87f2685..639c755 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP) += sony-laptop.o
obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
+obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
new file mode 100644
index 0000000..42e6e43
--- /dev/null
+++ b/drivers/misc/enclosure.c
@@ -0,0 +1,486 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley <[email protected]>
+ *
+**-----------------------------------------------------------------------------
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-----------------------------------------------------------------------------
+*/
+#include <linux/device.h>
+#include <linux/enclosure.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+static LIST_HEAD(container_list);
+static DEFINE_MUTEX(container_list_lock);
+static struct class enclosure_class;
+static struct class enclosure_component_class;
+
+/**
+ * enclosure_find - find an enclosure given a device
+ * @dev: the device to find for
+ *
+ * Looks through the list of registered enclosures to see
+ * if it can find a match for a device. Returns NULL if no
+ * enclosure is found.
+ */
+struct enclosure_device *enclosure_find(struct device *dev)
+{
+ struct enclosure_device *edev = NULL;
+
+ mutex_lock(&container_list_lock);
+ list_for_each_entry(edev, &container_list, node) {
+ if (edev->cdev.dev == dev) {
+ mutex_unlock(&container_list_lock);
+ return edev;
+ }
+ }
+ mutex_unlock(&container_list_lock);
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(enclosure_find);
+
+/**
+ * enclosure_for_each_device - calls a function for each enclosure
+ * @fn: the function to call
+ * @data: the data to pass to each call
+ *
+ * Loops over all the enclosures calling the function.
+ *
+ * Note, this function uses a mutex which will be held across calls to
+ * @fn, so it must have user context, and @fn should not sleep or
+ * otherwise cause the mutex to be held for indefinite periods
+ */
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data)
+{
+ int error = 0;
+ struct enclosure_device *edev;
+
+ mutex_lock(&container_list_lock);
+ list_for_each_entry(edev, &container_list, node) {
+ error = fn(edev, data);
+ if (error)
+ break;
+ }
+ mutex_unlock(&container_list_lock);
+
+ return error;
+}
+EXPORT_SYMBOL_GPL(enclosure_for_each_device);
+
+/**
+ * enclosure_register - register device as an enclosure
+ *
+ * @dev: device containing the enclosure
+ * @components: number of components in the enclosure
+ *
+ * This sets up the device for being an enclosure. Note that @dev does
+ * not have to be a dedicated enclosure device. It may be some other type
+ * of device that additionally responds to enclosure services
+ */
+struct enclosure_device *
+enclosure_register(struct device *dev, const char *name, int components,
+ struct enclosure_component_callbacks *cb)
+{
+ struct enclosure_device *edev =
+ kzalloc(sizeof(struct enclosure_device) +
+ sizeof(struct enclosure_component)*components,
+ GFP_KERNEL);
+ int err, i;
+
+ if (!edev)
+ return ERR_PTR(-ENOMEM);
+
+ if (!cb) {
+ kfree(edev);
+ return ERR_PTR(-EINVAL);
+ }
+
+ edev->components = components;
+
+ edev->cdev.class = &enclosure_class;
+ edev->cdev.dev = get_device(dev);
+ edev->cb = cb;
+ snprintf(edev->cdev.class_id, BUS_ID_SIZE, "%s", name);
+ err = class_device_register(&edev->cdev);
+ if (err)
+ goto err;
+
+ for (i = 0; i < components; i++)
+ edev->component[i].number = -1;
+
+ mutex_lock(&container_list_lock);
+ list_add_tail(&edev->node, &container_list);
+ mutex_unlock(&container_list_lock);
+
+ return edev;
+
+ err:
+ put_device(edev->cdev.dev);
+ kfree(edev);
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(enclosure_register);
+
+static struct enclosure_component_callbacks enclosure_null_callbacks;
+
+/**
+ * enclosure_unregister - remove an enclosure
+ *
+ * @edev: the registered enclosure to remove;
+ */
+void enclosure_unregister(struct enclosure_device *edev)
+{
+ int i;
+
+ if (!edev)
+ return;
+
+ mutex_lock(&container_list_lock);
+ list_del(&edev->node);
+ mutex_unlock(&container_list_lock);
+
+ for (i = 0; i < edev->components; i++)
+ if (edev->component[i].number != -1)
+ class_device_unregister(&edev->component[i].cdev);
+
+ /* prevent any callbacks into service user */
+ edev->cb = &enclosure_null_callbacks;
+ class_device_unregister(&edev->cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_unregister);
+
+static void enclosure_release(struct class_device *cdev)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev);
+
+ put_device(cdev->dev);
+ kfree(edev);
+}
+
+static void enclosure_component_release(struct class_device *cdev)
+{
+ if (cdev->dev)
+ put_device(cdev->dev);
+ class_device_put(cdev->parent);
+}
+
+/**
+ * enclosure_component_register - add a particular component to an enclosure
+ * @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.
+ *
+ * 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)
+{
+ struct enclosure_component *ecomp;
+ struct class_device *cdev;
+ int err;
+
+ if (!edev || number >= edev->components)
+ return ERR_PTR(-EINVAL);
+
+ ecomp = &edev->component[number];
+
+ if (ecomp->number != -1)
+ return ERR_PTR(-EINVAL);
+
+ ecomp->type = type;
+ ecomp->number = number;
+ cdev = &ecomp->cdev;
+ cdev->parent = class_device_get(&edev->cdev);
+ cdev->class = &enclosure_component_class;
+ if (name)
+ snprintf(cdev->class_id, BUS_ID_SIZE, "%s", name);
+ else
+ snprintf(cdev->class_id, BUS_ID_SIZE, "%d", number);
+
+ err = class_device_register(cdev);
+ if (err)
+ ERR_PTR(err);
+
+ return ecomp;
+}
+EXPORT_SYMBOL_GPL(enclosure_component_register);
+
+/**
+ * enclosure_add_device - add a device as being part of an enclosure
+ * @edev: the enclosure device being added to.
+ * @num: the number of the component
+ * @dev: the device being added
+ *
+ * Declares a real device to reside in slot (or identifier) @num of an
+ * enclosure. This will cause the relevant sysfs links to appear.
+ * This function may also be used to change a device associated with
+ * an enclosure without having to call enclosure_remove_device() in
+ * between.
+ *
+ * Returns zero on success or an error.
+ */
+int enclosure_add_device(struct enclosure_device *edev, int component,
+ struct device *dev)
+{
+ struct class_device *cdev;
+
+ if (!edev || component >= edev->components)
+ return -EINVAL;
+
+ cdev = &edev->component[component].cdev;
+
+ class_device_del(cdev);
+ if (cdev->dev)
+ put_device(cdev->dev);
+ cdev->dev = get_device(dev);
+ return class_device_add(cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_add_device);
+
+/**
+ * enclosure_remove_device - remove a device from an enclosure
+ * @edev: the enclosure device
+ * @num: the number of the component to remove
+ *
+ * Returns zero on success or an error.
+ *
+ */
+int enclosure_remove_device(struct enclosure_device *edev, int component)
+{
+ struct class_device *cdev;
+
+ if (!edev || component >= edev->components)
+ return -EINVAL;
+
+ cdev = &edev->component[component].cdev;
+
+ class_device_del(cdev);
+ if (cdev->dev)
+ put_device(cdev->dev);
+ cdev->dev = NULL;
+ return class_device_add(cdev);
+}
+EXPORT_SYMBOL_GPL(enclosure_remove_device);
+
+/*
+ * sysfs pieces below
+ */
+
+static ssize_t enclosure_show_components(struct class_device *cdev, char *buf)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev);
+
+ return snprintf(buf, 40, "%d\n", edev->components);
+}
+
+static struct class_device_attribute enclosure_attrs[] = {
+ __ATTR(components, S_IRUGO, enclosure_show_components, NULL),
+ __ATTR_NULL
+};
+
+static struct class enclosure_class = {
+ .name = "enclosure",
+ .owner = THIS_MODULE,
+ .release = enclosure_release,
+ .class_dev_attrs = enclosure_attrs,
+};
+
+static char *enclosure_status [] = {
+ [ENCLOSURE_STATUS_UNSUPPORTED] = "unsupported",
+ [ENCLOSURE_STATUS_OK] = "OK",
+ [ENCLOSURE_STATUS_CRITICAL] = "critical",
+ [ENCLOSURE_STATUS_NON_CRITICAL] = "non-critical",
+ [ENCLOSURE_STATUS_UNRECOVERABLE] = "unrecoverable",
+ [ENCLOSURE_STATUS_NOT_INSTALLED] = "not installed",
+ [ENCLOSURE_STATUS_UNKNOWN] = "unknown",
+ [ENCLOSURE_STATUS_UNAVAILABLE] = "unavailable",
+};
+
+static char *enclosure_type [] = {
+ [ENCLOSURE_COMPONENT_DEVICE] = "device",
+ [ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
+};
+
+static ssize_t get_component_fault(struct class_device *cdev, 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 snprintf(buf, 40, "%d\n", ecomp->fault);
+}
+
+static ssize_t set_component_fault(struct class_device *cdev, 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 class_device *cdev, char *buf)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+ if (edev->cb->get_status)
+ edev->cb->get_status(edev, ecomp);
+ return snprintf(buf, 40, "%s\n", enclosure_status[ecomp->status]);
+}
+
+static ssize_t set_component_status(struct class_device *cdev, const char *buf,
+ size_t count)
+{
+ struct enclosure_device *edev = to_enclosure_device(cdev->parent);
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+ int i;
+
+ for (i = 0; enclosure_status[i]; i++) {
+ if (strncmp(buf, enclosure_status[i],
+ strlen(enclosure_status[i])) == 0 &&
+ buf[strlen(enclosure_status[i])] == '\n')
+ break;
+ }
+
+ if (enclosure_status[i] && edev->cb->set_status) {
+ edev->cb->set_status(edev, ecomp, i);
+ return count;
+ } else
+ return -EINVAL;
+}
+
+static ssize_t get_component_active(struct class_device *cdev, 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 snprintf(buf, 40, "%d\n", ecomp->active);
+}
+
+static ssize_t set_component_active(struct class_device *cdev, 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 class_device *cdev, 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 snprintf(buf, 40, "%d\n", ecomp->locate);
+}
+
+static ssize_t set_component_locate(struct class_device *cdev, 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_type(struct class_device *cdev, char *buf)
+{
+ struct enclosure_component *ecomp = to_enclosure_component(cdev);
+
+ return snprintf(buf, 40, "%s\n", enclosure_type[ecomp->type]);
+}
+
+
+static struct class_device_attribute enclosure_component_attrs[] = {
+ __ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
+ set_component_fault),
+ __ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
+ set_component_status),
+ __ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
+ set_component_active),
+ __ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
+ set_component_locate),
+ __ATTR(type, S_IRUGO, get_component_type, NULL),
+ __ATTR_NULL
+};
+
+static struct class enclosure_component_class = {
+ .name = "enclosure_component",
+ .owner = THIS_MODULE,
+ .class_dev_attrs = enclosure_component_attrs,
+ .release = enclosure_component_release,
+};
+
+static int __init enclosure_init(void)
+{
+ int err;
+
+ err = class_register(&enclosure_class);
+ if (err)
+ return err;
+ err = class_register(&enclosure_component_class);
+ if (err)
+ goto err_out;
+
+ return 0;
+ err_out:
+ class_unregister(&enclosure_class);
+
+ return err;
+}
+
+static void __exit enclosure_exit(void)
+{
+ class_unregister(&enclosure_component_class);
+ class_unregister(&enclosure_class);
+}
+
+module_init(enclosure_init);
+module_exit(enclosure_exit);
+
+MODULE_AUTHOR("James Bottomley");
+MODULE_DESCRIPTION("Enclosure Services");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
new file mode 100644
index 0000000..622177a
--- /dev/null
+++ b/include/linux/enclosure.h
@@ -0,0 +1,120 @@
+/*
+ * Enclosure Services
+ *
+ * Copyright (C) 2008 James Bottomley <[email protected]>
+ *
+**-----------------------------------------------------------------------------
+**
+** This program is free software; you can redistribute it and/or
+** modify it under the terms of the GNU General Public License
+** version 2 as published by the Free Software Foundation.
+**
+** This program is distributed in the hope that it will be useful,
+** but WITHOUT ANY WARRANTY; without even the implied warranty of
+** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+** GNU General Public License for more details.
+**
+** You should have received a copy of the GNU General Public License
+** along with this program; if not, write to the Free Software
+** Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+**
+**-----------------------------------------------------------------------------
+*/
+#ifndef _LINUX_ENCLOSURE_H_
+#define _LINUX_ENCLOSURE_H_
+
+#include <linux/device.h>
+#include <linux/list.h>
+
+/* A few generic types ... taken from ses-2 */
+enum enclosure_component_type {
+ ENCLOSURE_COMPONENT_DEVICE = 0x01,
+ ENCLOSURE_COMPONENT_ARRAY_DEVICE = 0x17,
+};
+
+/* ses-2 common element status */
+enum enclosure_status {
+ ENCLOSURE_STATUS_UNSUPPORTED = 0,
+ ENCLOSURE_STATUS_OK,
+ ENCLOSURE_STATUS_CRITICAL,
+ ENCLOSURE_STATUS_NON_CRITICAL,
+ ENCLOSURE_STATUS_UNRECOVERABLE,
+ ENCLOSURE_STATUS_NOT_INSTALLED,
+ ENCLOSURE_STATUS_UNKNOWN,
+ ENCLOSURE_STATUS_UNAVAILABLE,
+};
+
+/* SFF-8485 activity light settings */
+enum enclosure_component_setting {
+ ENCLOSURE_SETTING_DISABLED = 0,
+ ENCLOSURE_SETTING_ENABLED = 1,
+ ENCLOSURE_SETTING_BLINK_A_ON_OFF = 2,
+ ENCLOSURE_SETTING_BLINK_A_OFF_ON = 3,
+ ENCLOSURE_SETTING_BLINK_B_ON_OFF = 6,
+ ENCLOSURE_SETTING_BLINK_B_OFF_ON = 7,
+};
+
+struct enclosure_device;
+struct enclosure_component;
+struct enclosure_component_callbacks {
+ void (*get_status)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_status)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_status);
+ void (*get_fault)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_fault)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_component_setting);
+ void (*get_active)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_active)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_component_setting);
+ void (*get_locate)(struct enclosure_device *,
+ struct enclosure_component *);
+ int (*set_locate)(struct enclosure_device *,
+ struct enclosure_component *,
+ enum enclosure_component_setting);
+};
+
+
+struct enclosure_component {
+ void *scratch;
+ struct class_device cdev;
+ enum enclosure_component_type type;
+ int number;
+ int fault;
+ int active;
+ int locate;
+ enum enclosure_status status;
+};
+
+struct enclosure_device {
+ void *scratch;
+ struct list_head node;
+ struct class_device cdev;
+ struct enclosure_component_callbacks *cb;
+ int components;
+ struct enclosure_component component[0];
+};
+
+#define to_enclosure_device(x) container_of((x), struct enclosure_device, cdev)
+#define to_enclosure_component(x) container_of((x), struct enclosure_component, cdev)
+
+struct enclosure_device *
+enclosure_register(struct device *, const char *, int,
+ struct enclosure_component_callbacks *);
+void enclosure_unregister(struct enclosure_device *);
+struct enclosure_component *
+enclosure_component_register(struct enclosure_device *, unsigned int,
+ enum enclosure_component_type, const char *);
+int enclosure_add_device(struct enclosure_device *enclosure, int component,
+ struct device *dev);
+int enclosure_remove_device(struct enclosure_device *enclosure, int component);
+struct enclosure_device *enclosure_find(struct device *dev);
+int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
+ void *data);
+
+#endif /* _LINUX_ENCLOSURE_H_ */
--
1.5.3.8
--- On Sun, 2/3/08, James Bottomley <[email protected]> wrote:
> The enclosure misc device is really just a library providing
> sysfs
> support for physical enclosure devices and their
> components.
Who is the target audience/user of those facilities?
a) The kernel itself needing to read/write SES pages?
b) A user space application using sysfs to read/write
SES pages?
At the moment SES device management is done via
an application (user-space) and a user-space library
used by the application and /dev/sgX to send SCSI
commands to the SES device.
One could have a very good argument to not bloat
the kernel with this but leave it to a user-space
application and a library to do all this and
communicate with the SES device via the kernel's /dev/sgX.
Luben
On Mon, 2008-02-04 at 16:32 -0800, Luben Tuikov wrote:
> --- On Sun, 2/3/08, James Bottomley <[email protected]> wrote:
>
> > The enclosure misc device is really just a library providing
> > sysfs
> > support for physical enclosure devices and their
> > components.
>
> Who is the target audience/user of those facilities?
> a) The kernel itself needing to read/write SES pages?
That depends on the enclosure integration, but right at the moment, it
doesn't
> b) A user space application using sysfs to read/write
> SES pages?
Not an application so much as a user. The idea of sysfs is to allow
users to get and set the information in addition to applications.
> At the moment SES device management is done via
> an application (user-space) and a user-space library
> used by the application and /dev/sgX to send SCSI
> commands to the SES device.
I must have missed that when I was looking for implementations; what's
the URL?
But, if we have non-scsi enclosures to integrate, that makes it harder
for a user application because it has to know all the implementations.
A sysfs framework on the other hand is a universal known thing for the
user applications.
> One could have a very good argument to not bloat
> the kernel with this but leave it to a user-space
> application and a library to do all this and
> communicate with the SES device via the kernel's /dev/sgX.
The same thing goes for other esoteric SCSI infrastructure pieces like
cd changers. On the whole, given that ATA is asking for enclosure
management in kernel, it makes sense to consolidate the infrastructure
and a ses ULD is a very good test bed.
James
--- On Mon, 2/4/08, James Bottomley <[email protected]> wrote:
> > > The enclosure misc device is really just a
> library providing
> > > sysfs
> > > support for physical enclosure devices and their
> > > components.
> >
> > Who is the target audience/user of those facilities?
> > a) The kernel itself needing to read/write SES pages?
>
> That depends on the enclosure integration, but right at the
> moment, it
> doesn't
Yes, I didn't suspect so.
>
> > b) A user space application using sysfs to read/write
> > SES pages?
>
> Not an application so much as a user. The idea of sysfs is
> to allow
> users to get and set the information in addition to
> applications.
Exactly the same argument stands for a user-space
application with a user-space library.
This is the classical case of where it is better to
do this in user-space as opposed to the kernel.
The kernel provides capability to access the SES
device. The user space application and library
provide interpretation and control. Thus if the
enclosure were upgraded, one doesn't need to
upgrade their kernel in order to utilize the new
capabilities of the SES device. Plus upgrading
a user-space application is a lot easier than
the kernel (and no reboot necessary).
Consider another thing: vendors would really like
unprecedented access to the SES device in the enclosure
so as your ses/enclosure code keeps state it would
get out of sync when vendor user-space enclosure
applications access (and modify) the SES device's
pages.
You can test this yourself: submit a patch
that removes SES /dev/sgX support; advertise your
ses/class solution and watch the fun.
> > At the moment SES device management is done via
> > an application (user-space) and a user-space library
> > used by the application and /dev/sgX to send SCSI
> > commands to the SES device.
>
> I must have missed that when I was looking for
> implementations; what's
> the URL?
I'm not aware of any GPLed ones. That doesn't
necessarily mean that the best course of action is
to bloat the kernel. You can move your ses/enclosure
stuff to a user space application library
and thus start a GPLed one.
> But, if we have non-scsi enclosures to integrate, that
> makes it harder
> for a user application because it has to know all the
> implementations.
So does the kernel. And as I pointed out above, it
is a lot easier to upgrade a user-space application and
library than it is to upgrade a new kernel and having
to reboot the computer to run the new kernel.
> A sysfs framework on the other hand is a universal known
> thing for the
> user applications.
So would a user-space ses library, a la libses.so.
> > One could have a very good argument to not bloat
> > the kernel with this but leave it to a user-space
> > application and a library to do all this and
> > communicate with the SES device via the kernel's
> /dev/sgX.
>
> The same thing goes for other esoteric SCSI infrastructure
> pieces like
> cd changers. On the whole, given that ATA is asking for
> enclosure
> management in kernel, it makes sense to consolidate the
> infrastructure
> and a ses ULD is a very good test bed.
What is wrong with exporting the SES device as /dev/sgX
and having a user-space application and library to
do all this?
Luben
On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote:
> --- On Mon, 2/4/08, James Bottomley <[email protected]> wrote:
> > > > The enclosure misc device is really just a
> > library providing
> > > > sysfs
> > > > support for physical enclosure devices and their
> > > > components.
> > >
> > > Who is the target audience/user of those facilities?
> > > a) The kernel itself needing to read/write SES pages?
> >
> > That depends on the enclosure integration, but right at the
> > moment, it
> > doesn't
>
> Yes, I didn't suspect so.
>
> >
> > > b) A user space application using sysfs to read/write
> > > SES pages?
> >
> > Not an application so much as a user. The idea of sysfs is
> > to allow
> > users to get and set the information in addition to
> > applications.
>
> Exactly the same argument stands for a user-space
> application with a user-space library.
>
> This is the classical case of where it is better to
> do this in user-space as opposed to the kernel.
>
> The kernel provides capability to access the SES
> device. The user space application and library
> provide interpretation and control. Thus if the
> enclosure were upgraded, one doesn't need to
> upgrade their kernel in order to utilize the new
> capabilities of the SES device. Plus upgrading
> a user-space application is a lot easier than
> the kernel (and no reboot necessary).
The implementation is modular, so it's remove and insert ...
> Consider another thing: vendors would really like
> unprecedented access to the SES device in the enclosure
> so as your ses/enclosure code keeps state it would
> get out of sync when vendor user-space enclosure
> applications access (and modify) the SES device's
> pages.
The state model doesn't assume nothing else will alter the state.
> You can test this yourself: submit a patch
> that removes SES /dev/sgX support; advertise your
> ses/class solution and watch the fun.
>
> > > At the moment SES device management is done via
> > > an application (user-space) and a user-space library
> > > used by the application and /dev/sgX to send SCSI
> > > commands to the SES device.
> >
> > I must have missed that when I was looking for
> > implementations; what's
> > the URL?
>
> I'm not aware of any GPLed ones. That doesn't
> necessarily mean that the best course of action is
> to bloat the kernel. You can move your ses/enclosure
> stuff to a user space application library
> and thus start a GPLed one.
Certainly ... patches welcome.
> > But, if we have non-scsi enclosures to integrate, that
> > makes it harder
> > for a user application because it has to know all the
> > implementations.
>
> So does the kernel. And as I pointed out above, it
> is a lot easier to upgrade a user-space application and
> library than it is to upgrade a new kernel and having
> to reboot the computer to run the new kernel.
No, think again ... it's easy for SES based enclosures because they have
a SCSI transport. We have no transport for SGPIO based enclosures nor
for any of the other more esoteric ones.
That's not to say it can't be done, but it does mean that it can't be
completely userspace.
> > A sysfs framework on the other hand is a universal known
> > thing for the
> > user applications.
>
> So would a user-space ses library, a la libses.so.
>
> > > One could have a very good argument to not bloat
> > > the kernel with this but leave it to a user-space
> > > application and a library to do all this and
> > > communicate with the SES device via the kernel's
> > /dev/sgX.
> >
> > The same thing goes for other esoteric SCSI infrastructure
> > pieces like
> > cd changers. On the whole, given that ATA is asking for
> > enclosure
> > management in kernel, it makes sense to consolidate the
> > infrastructure
> > and a ses ULD is a very good test bed.
>
> What is wrong with exporting the SES device as /dev/sgX
> and having a user-space application and library to
> do all this?
How do you transport the enclosure commands over /dev/sgX? Only SES has
SCSI command encapsulation ... the rest won't even be SCSI targets ...
James
--- On Mon, 2/4/08, James Bottomley <[email protected]> wrote:
> On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote:
> > --- On Mon, 2/4/08, James Bottomley
> <[email protected]> wrote:
> > > > > The enclosure misc device is really
> just a
> > > library providing
> > > > > sysfs
> > > > > support for physical enclosure devices
> and their
> > > > > components.
> > > >
> > > > Who is the target audience/user of those
> facilities?
> > > > a) The kernel itself needing to read/write
> SES pages?
> > >
> > > That depends on the enclosure integration, but
> right at the
> > > moment, it
> > > doesn't
> >
> > Yes, I didn't suspect so.
> >
> > >
> > > > b) A user space application using sysfs to
> read/write
> > > > SES pages?
> > >
> > > Not an application so much as a user. The idea
> of sysfs is
> > > to allow
> > > users to get and set the information in addition
> to
> > > applications.
> >
> > Exactly the same argument stands for a user-space
> > application with a user-space library.
> >
> > This is the classical case of where it is better to
> > do this in user-space as opposed to the kernel.
> >
> > The kernel provides capability to access the SES
> > device. The user space application and library
> > provide interpretation and control. Thus if the
> > enclosure were upgraded, one doesn't need to
> > upgrade their kernel in order to utilize the new
> > capabilities of the SES device. Plus upgrading
> > a user-space application is a lot easier than
> > the kernel (and no reboot necessary).
>
> The implementation is modular, so it's remove and
> insert ...
I guess the same could be said for STGT and SCST, right?
LOL, no seriously, this is unnecessary kernel bloat,
or rather at the wrong place (see below).
>
> > Consider another thing: vendors would really like
> > unprecedented access to the SES device in the
> enclosure
> > so as your ses/enclosure code keeps state it would
> > get out of sync when vendor user-space enclosure
> > applications access (and modify) the SES device's
> > pages.
>
> The state model doesn't assume nothing else will alter
> the state.
But it would be trivial exercise to show that an
inconsistent state can be had by modifying pages
of the SES device directly from userspace bypassing
your implementation.
>
> > You can test this yourself: submit a patch
> > that removes SES /dev/sgX support; advertise your
> > ses/class solution and watch the fun.
> >
> > > > At the moment SES device management is done
> via
> > > > an application (user-space) and a user-space
> library
> > > > used by the application and /dev/sgX to send
> SCSI
> > > > commands to the SES device.
> > >
> > > I must have missed that when I was looking for
> > > implementations; what's
> > > the URL?
> >
> > I'm not aware of any GPLed ones. That doesn't
> > necessarily mean that the best course of action is
> > to bloat the kernel. You can move your ses/enclosure
> > stuff to a user space application library
> > and thus start a GPLed one.
>
> Certainly ... patches welcome.
I've non at the moment, plus I don't think you'd be
the point of contact for a user-space SES library.
Unless of course you've already started something up
on sourceforge.
Really, such an effort already exists: it is called
sg_ses(8).
>
> > > But, if we have non-scsi enclosures to integrate,
> that
> > > makes it harder
> > > for a user application because it has to know all
> the
> > > implementations.
> >
> > So does the kernel. And as I pointed out above, it
> > is a lot easier to upgrade a user-space application
> and
> > library than it is to upgrade a new kernel and having
> > to reboot the computer to run the new kernel.
>
> No, think again ... it's easy for SES based enclosures
> because they have
> a SCSI transport. We have no transport for SGPIO based
> enclosures nor
> for any of the other more esoteric ones.
Yes, for which the transport layer, implements the
scsi device node for the SES device. It doesn't really
matter if the SCSI commands sent to the SES device go
over SGPIO or FC or SAS or Bluetooth or I2C, etc, the
transport layer can implement that and present the
/dev/sgX node.
Case in point: the protocol FW running on the ASIC
provides this capability so really the LLDD would
only see a the pure SCSI SES or processor device and
register that with the kernel. At which point no new
kernel bloat is required.
Your code doesn't quite do that at the moment as it
actually goes further in to read and present SES pages.
Ideally it would simply provide capability for transport
layers to register a SCSI device of type SES, or processor.
Architecturally, the LLDD/transport layer would register
the SGPIO device on one end with the SGPIO layer and on
the other end as a SCSI SES/processpr device. After that
sg_ses(8) or sglib, fits the bill for user space applications.
> That's not to say it can't be done, but it does
> mean that it can't be
> completely userspace.
See previous paragraph.
>
> > > A sysfs framework on the other hand is a
> universal known
> > > thing for the
> > > user applications.
> >
> > So would a user-space ses library, a la libses.so.
> >
> > > > One could have a very good argument to not
> bloat
> > > > the kernel with this but leave it to a
> user-space
> > > > application and a library to do all this and
> > > > communicate with the SES device via the
> kernel's
> > > /dev/sgX.
> > >
> > > The same thing goes for other esoteric SCSI
> infrastructure
> > > pieces like
> > > cd changers. On the whole, given that ATA is
> asking for
> > > enclosure
> > > management in kernel, it makes sense to
> consolidate the
> > > infrastructure
> > > and a ses ULD is a very good test bed.
> >
> > What is wrong with exporting the SES device as
> /dev/sgX
> > and having a user-space application and library to
> > do all this?
>
> How do you transport the enclosure commands over /dev/sgX?
> Only SES has
> SCSI command encapsulation ... the rest won't even be
> SCSI targets ...
What is the protocol of those "rest" that you talk about?
At any rate, this capability lies in the kernel providing
a _device node_ -- not quite what your patch is doing.
Luben
On Mon, 2008-02-04 at 19:28 -0800, Luben Tuikov wrote:
> --- On Mon, 2/4/08, James Bottomley <[email protected]> wrote:
> > On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov wrote:
> > > --- On Mon, 2/4/08, James Bottomley
> > <[email protected]> wrote:
> > > > > > The enclosure misc device is really
> > just a
> > > > library providing
> > > > > > sysfs
> > > > > > support for physical enclosure devices
> > and their
> > > > > > components.
> > > > >
> > > > > Who is the target audience/user of those
> > facilities?
> > > > > a) The kernel itself needing to read/write
> > SES pages?
> > > >
> > > > That depends on the enclosure integration, but
> > right at the
> > > > moment, it
> > > > doesn't
> > >
> > > Yes, I didn't suspect so.
> > >
> > > >
> > > > > b) A user space application using sysfs to
> > read/write
> > > > > SES pages?
> > > >
> > > > Not an application so much as a user. The idea
> > of sysfs is
> > > > to allow
> > > > users to get and set the information in addition
> > to
> > > > applications.
> > >
> > > Exactly the same argument stands for a user-space
> > > application with a user-space library.
> > >
> > > This is the classical case of where it is better to
> > > do this in user-space as opposed to the kernel.
> > >
> > > The kernel provides capability to access the SES
> > > device. The user space application and library
> > > provide interpretation and control. Thus if the
> > > enclosure were upgraded, one doesn't need to
> > > upgrade their kernel in order to utilize the new
> > > capabilities of the SES device. Plus upgrading
> > > a user-space application is a lot easier than
> > > the kernel (and no reboot necessary).
> >
> > The implementation is modular, so it's remove and
> > insert ...
>
> I guess the same could be said for STGT and SCST, right?
You mean both of their kernel pieces are modular? That's correct.
> LOL, no seriously, this is unnecessary kernel bloat,
> or rather at the wrong place (see below).
>
> >
> > > Consider another thing: vendors would really like
> > > unprecedented access to the SES device in the
> > enclosure
> > > so as your ses/enclosure code keeps state it would
> > > get out of sync when vendor user-space enclosure
> > > applications access (and modify) the SES device's
> > > pages.
> >
> > The state model doesn't assume nothing else will alter
> > the state.
>
> But it would be trivial exercise to show that an
> inconsistent state can be had by modifying pages
> of the SES device directly from userspace bypassing
> your implementation.
I don't think so ... if you actually look at the code, you'll see it
doesn't really have persistent state for the enclosure.
> > > You can test this yourself: submit a patch
> > > that removes SES /dev/sgX support; advertise your
> > > ses/class solution and watch the fun.
> > >
> > > > > At the moment SES device management is done
> > via
> > > > > an application (user-space) and a user-space
> > library
> > > > > used by the application and /dev/sgX to send
> > SCSI
> > > > > commands to the SES device.
> > > >
> > > > I must have missed that when I was looking for
> > > > implementations; what's
> > > > the URL?
> > >
> > > I'm not aware of any GPLed ones. That doesn't
> > > necessarily mean that the best course of action is
> > > to bloat the kernel. You can move your ses/enclosure
> > > stuff to a user space application library
> > > and thus start a GPLed one.
> >
> > Certainly ... patches welcome.
>
> I've non at the moment, plus I don't think you'd be
> the point of contact for a user-space SES library.
> Unless of course you've already started something up
> on sourceforge.
>
> Really, such an effort already exists: it is called
> sg_ses(8).
>
> >
> > > > But, if we have non-scsi enclosures to integrate,
> > that
> > > > makes it harder
> > > > for a user application because it has to know all
> > the
> > > > implementations.
> > >
> > > So does the kernel. And as I pointed out above, it
> > > is a lot easier to upgrade a user-space application
> > and
> > > library than it is to upgrade a new kernel and having
> > > to reboot the computer to run the new kernel.
> >
> > No, think again ... it's easy for SES based enclosures
> > because they have
> > a SCSI transport. We have no transport for SGPIO based
> > enclosures nor
> > for any of the other more esoteric ones.
>
> Yes, for which the transport layer, implements the
> scsi device node for the SES device. It doesn't really
> matter if the SCSI commands sent to the SES device go
> over SGPIO or FC or SAS or Bluetooth or I2C, etc, the
> transport layer can implement that and present the
> /dev/sgX node.
But it does matter if the enclosure device doesn't speak SCSI. SGPIO
isn't a SCSI protocol ... it's a general purpose serial bus protocol.
It's pretty simple and register based, but it might (or might not) be
accessible via a SCSI bridge.
> Case in point: the protocol FW running on the ASIC
> provides this capability so really the LLDD would
> only see a the pure SCSI SES or processor device and
> register that with the kernel. At which point no new
> kernel bloat is required.
>
> Your code doesn't quite do that at the moment as it
> actually goes further in to read and present SES pages.
> Ideally it would simply provide capability for transport
> layers to register a SCSI device of type SES, or processor.
Yes, it provides a glue between the enclosure services and the SES
protocol.
> Architecturally, the LLDD/transport layer would register
> the SGPIO device on one end with the SGPIO layer and on
> the other end as a SCSI SES/processpr device. After that
> sg_ses(8) or sglib, fits the bill for user space applications.
That's possible, but none of these layers exist yet ... although I think
(assuming I can find a SGPIO enclosure) that SGPIO might be next.
> > That's not to say it can't be done, but it does
> > mean that it can't be
> > completely userspace.
>
> See previous paragraph.
>
> >
> > > > A sysfs framework on the other hand is a
> > universal known
> > > > thing for the
> > > > user applications.
> > >
> > > So would a user-space ses library, a la libses.so.
> > >
> > > > > One could have a very good argument to not
> > bloat
> > > > > the kernel with this but leave it to a
> > user-space
> > > > > application and a library to do all this and
> > > > > communicate with the SES device via the
> > kernel's
> > > > /dev/sgX.
> > > >
> > > > The same thing goes for other esoteric SCSI
> > infrastructure
> > > > pieces like
> > > > cd changers. On the whole, given that ATA is
> > asking for
> > > > enclosure
> > > > management in kernel, it makes sense to
> > consolidate the
> > > > infrastructure
> > > > and a ses ULD is a very good test bed.
> > >
> > > What is wrong with exporting the SES device as
> > /dev/sgX
> > > and having a user-space application and library to
> > > do all this?
> >
> > How do you transport the enclosure commands over /dev/sgX?
> > Only SES has
> > SCSI command encapsulation ... the rest won't even be
> > SCSI targets ...
>
> What is the protocol of those "rest" that you talk about?
At the moment it looks to be SES, SGPIO and AHCI.
> At any rate, this capability lies in the kernel providing
> a _device node_ -- not quite what your patch is doing.
So your idea is to provide a separate interface per enclosure in kernel?
Sure ... like I said patches welcome. I just did a common in-kernel
interface that abstracts common enclosure services.
James
--- On Mon, 2/4/08, James Bottomley <[email protected]> wrote:
> > --- On Mon, 2/4/08, James Bottomley
> <[email protected]> wrote:
> > > On Mon, 2008-02-04 at 18:01 -0800, Luben Tuikov
> wrote:
> > > > --- On Mon, 2/4/08, James Bottomley
> > > <[email protected]>
> wrote:
> > > > > > > The enclosure misc device is
> really
> > > just a
> > > > > library providing
> > > > > > > sysfs
> > > > > > > support for physical
> enclosure devices
> > > and their
> > > > > > > components.
> > > > > >
> > > > > > Who is the target audience/user of
> those
> > > facilities?
> > > > > > a) The kernel itself needing to
> read/write
> > > SES pages?
> > > > >
> > > > > That depends on the enclosure
> integration, but
> > > right at the
> > > > > moment, it
> > > > > doesn't
> > > >
> > > > Yes, I didn't suspect so.
> > > >
> > > > >
> > > > > > b) A user space application using
> sysfs to
> > > read/write
> > > > > > SES pages?
> > > > >
> > > > > Not an application so much as a user.
> The idea
> > > of sysfs is
> > > > > to allow
> > > > > users to get and set the information in
> addition
> > > to
> > > > > applications.
> > > >
> > > > Exactly the same argument stands for a
> user-space
> > > > application with a user-space library.
> > > >
> > > > This is the classical case of where it is
> better to
> > > > do this in user-space as opposed to the
> kernel.
> > > >
> > > > The kernel provides capability to access the
> SES
> > > > device. The user space application and
> library
> > > > provide interpretation and control. Thus if
> the
> > > > enclosure were upgraded, one doesn't
> need to
> > > > upgrade their kernel in order to utilize the
> new
> > > > capabilities of the SES device. Plus
> upgrading
> > > > a user-space application is a lot easier
> than
> > > > the kernel (and no reboot necessary).
> > >
> > > The implementation is modular, so it's remove
> and
> > > insert ...
> >
> > I guess the same could be said for STGT and SCST,
> right?
>
> You mean both of their kernel pieces are modular?
> That's correct.
No, you know very well what I mean.
By the same logic you're preaching to include your
solution part of the kernel, you can also apply to
SCST.
> > LOL, no seriously, this is unnecessary kernel bloat,
> > or rather at the wrong place (see below).
> >
> > >
> > > > Consider another thing: vendors would really
> like
> > > > unprecedented access to the SES device in
> the
> > > enclosure
> > > > so as your ses/enclosure code keeps state it
> would
> > > > get out of sync when vendor user-space
> enclosure
> > > > applications access (and modify) the SES
> device's
> > > > pages.
> > >
> > > The state model doesn't assume nothing else
> will alter
> > > the state.
> >
> > But it would be trivial exercise to show that an
> > inconsistent state can be had by modifying pages
> > of the SES device directly from userspace bypassing
> > your implementation.
>
> I don't think so ... if you actually look at the code,
> you'll see it
> doesn't really have persistent state for the enclosure.
>
> > > > You can test this yourself: submit a patch
> > > > that removes SES /dev/sgX support; advertise
> your
> > > > ses/class solution and watch the fun.
> > > >
> > > > > > At the moment SES device
> management is done
> > > via
> > > > > > an application (user-space) and a
> user-space
> > > library
> > > > > > used by the application and
> /dev/sgX to send
> > > SCSI
> > > > > > commands to the SES device.
> > > > >
> > > > > I must have missed that when I was
> looking for
> > > > > implementations; what's
> > > > > the URL?
> > > >
> > > > I'm not aware of any GPLed ones. That
> doesn't
> > > > necessarily mean that the best course of
> action is
> > > > to bloat the kernel. You can move your
> ses/enclosure
> > > > stuff to a user space application library
> > > > and thus start a GPLed one.
> > >
> > > Certainly ... patches welcome.
> >
> > I've non at the moment, plus I don't think
> you'd be
> > the point of contact for a user-space SES library.
> > Unless of course you've already started something
> up
> > on sourceforge.
> >
> > Really, such an effort already exists: it is called
> > sg_ses(8).
> >
> > >
> > > > > But, if we have non-scsi enclosures to
> integrate,
> > > that
> > > > > makes it harder
> > > > > for a user application because it has
> to know all
> > > the
> > > > > implementations.
> > > >
> > > > So does the kernel. And as I pointed out
> above, it
> > > > is a lot easier to upgrade a user-space
> application
> > > and
> > > > library than it is to upgrade a new kernel
> and having
> > > > to reboot the computer to run the new
> kernel.
> > >
> > > No, think again ... it's easy for SES based
> enclosures
> > > because they have
> > > a SCSI transport. We have no transport for SGPIO
> based
> > > enclosures nor
> > > for any of the other more esoteric ones.
> >
> > Yes, for which the transport layer, implements the
> > scsi device node for the SES device. It doesn't
> really
> > matter if the SCSI commands sent to the SES device go
> > over SGPIO or FC or SAS or Bluetooth or I2C, etc, the
> > transport layer can implement that and present the
> > /dev/sgX node.
>
> But it does matter if the enclosure device doesn't
> speak SCSI.
Enclosure management isn't as simple as you're
portraying it here. The enclosure management
device speaks either SES or SAF-TE. The transport
protocol to access it could be SGPIO or I2C or...
> SGPIO
> isn't a SCSI protocol ... it's a general purpose
> serial bus protocol.
> It's pretty simple and register based, but it might (or
> might not) be
> accessible via a SCSI bridge.
I see. You've just discovered SGPIO -- good for you.
At any rate, I told you already that what is needed
is not what you've provided but a _device node_
exported by the kernel, either a processor or
enclosure type.
> > Case in point: the protocol FW running on the ASIC
> > provides this capability so really the LLDD would
> > only see a the pure SCSI SES or processor device and
> > register that with the kernel. At which point no new
> > kernel bloat is required.
> >
> > Your code doesn't quite do that at the moment as
> it
> > actually goes further in to read and present SES
> pages.
> > Ideally it would simply provide capability for
> transport
> > layers to register a SCSI device of type SES, or
> processor.
>
> Yes, it provides a glue between the enclosure services and
> the SES
> protocol.
You're jumping over layers here.
And that's not what's needed. What is needed
is an instrumentation by the kernel to present the
enclosure device as a device node to user space
so that general utilities tools, like for example
sg_ses(8) can work with it.
Your solution seems to be interpreting SES in the
kernel and this is really unwieldy.
Just provide the device node to user space.
You shouldn't have to interpret SES in the kernel
as you're currently doing since the kernel itself
has no use of it.
> > Architecturally, the LLDD/transport layer would
> register
> > the SGPIO device on one end with the SGPIO layer and
> on
> > the other end as a SCSI SES/processpr device. After
> that
> > sg_ses(8) or sglib, fits the bill for user space
> applications.
>
> That's possible, but none of these layers exist yet ...
> although I think
> (assuming I can find a SGPIO enclosure) that SGPIO might be
> next.
Vendors prefer to abstract the transport protocol
in HW/FW so that to ULD the device is of
type processor or enclosure.
This makes it easy for the vendor (in terms of
support) and for the customer (in terms of
usability).
>
> > > That's not to say it can't be done, but
> it does
> > > mean that it can't be
> > > completely userspace.
> >
> > See previous paragraph.
> >
> > >
> > > > > A sysfs framework on the other hand is
> a
> > > universal known
> > > > > thing for the
> > > > > user applications.
> > > >
> > > > So would a user-space ses library, a la
> libses.so.
> > > >
> > > > > > One could have a very good
> argument to not
> > > bloat
> > > > > > the kernel with this but leave it
> to a
> > > user-space
> > > > > > application and a library to do
> all this and
> > > > > > communicate with the SES device
> via the
> > > kernel's
> > > > > /dev/sgX.
> > > > >
> > > > > The same thing goes for other esoteric
> SCSI
> > > infrastructure
> > > > > pieces like
> > > > > cd changers. On the whole, given that
> ATA is
> > > asking for
> > > > > enclosure
> > > > > management in kernel, it makes sense to
> > > consolidate the
> > > > > infrastructure
> > > > > and a ses ULD is a very good test bed.
> > > >
> > > > What is wrong with exporting the SES device
> as
> > > /dev/sgX
> > > > and having a user-space application and
> library to
> > > > do all this?
> > >
> > > How do you transport the enclosure commands over
> /dev/sgX?
> > > Only SES has
> > > SCSI command encapsulation ... the rest won't
> even be
> > > SCSI targets ...
> >
> > What is the protocol of those "rest" that
> you talk about?
>
> At the moment it looks to be SES, SGPIO and AHCI.
You should separate "what it talks" by "how it
gets there".
>
> > At any rate, this capability lies in the kernel
> providing
> > a _device node_ -- not quite what your patch is doing.
>
> So your idea is to provide a separate interface per
> enclosure in kernel?
No. My idea is to provide a device node of
type processor or enclosure for each enclosure.
Expanders already do this by providing a virtual
phy which is "connected" to the SES-2 device, so
a device of type SES is discovered and registered
with the kernel as any other device on the domain.
Then the enclosure can be controlled via sg_ses(8)
from user space.
Host adapters already do this in their FW, since
it is there where they do the transport protocol
abstraction as well.
You can help AHCI (if this really is needed of
course) to achieve the same thing by providing
code so that a LLDD/etc can register a
processor/enclosure device accessible via the
ASIC. At any rate the actual protocol to talk
to the enclosure device is abstracted away by
the HW solution (either in FW or HW).
At this moment I see all this effort in vain
for reasons I've described above.
> Sure ... like I said patches welcome. I just did a common
> in-kernel
> interface that abstracts common enclosure services.
I know -- this is why and where I responded to your
patches.
So I guess by what you just said, we're back
at square one? ... is this really needed, who is the
intended user/audience... why not just a device
node to be controlled via sg_ses(8) from user
space, as is already done for other enclosure
devices... etc.
Luben
On Mon, 2008-02-04 at 21:35 -0800, Luben Tuikov wrote:
> > > I guess the same could be said for STGT and SCST,
> > right?
> >
> > You mean both of their kernel pieces are modular?
> > That's correct.
>
> No, you know very well what I mean.
>
> By the same logic you're preaching to include your
> solution part of the kernel, you can also apply to
> SCST.
Ah, but it's not ... the current patch is merely exporting an interface.
The debate in STGT vs SCST is not whether to export an interface but
where to draw the line.
You could also argue in the same vein that sd is redundant because a
filesystem could talk directly to the device via /dev/sgX (in fact OSD
based filesystems already do this). The argument is true, but misses
the bigger picture that the interfaces exported by sd are more portable
(apply to non-SCSI block devices) and easier to use.
> > > Yes, for which the transport layer, implements the
> > > scsi device node for the SES device. It doesn't
> > really
> > > matter if the SCSI commands sent to the SES device go
> > > over SGPIO or FC or SAS or Bluetooth or I2C, etc, the
> > > transport layer can implement that and present the
> > > /dev/sgX node.
> >
> > But it does matter if the enclosure device doesn't
> > speak SCSI.
>
> Enclosure management isn't as simple as you're
> portraying it here. The enclosure management
> device speaks either SES or SAF-TE. The transport
> protocol to access it could be SGPIO or I2C or...
Look, just read the spec; SGPIO is a bus for driving enclosures ... it
doesn't require SES or SAF-TE or even any SCSI protocol.
> > SGPIO
> > isn't a SCSI protocol ... it's a general purpose
> > serial bus protocol.
> > It's pretty simple and register based, but it might (or
> > might not) be
> > accessible via a SCSI bridge.
>
> I see. You've just discovered SGPIO -- good for you.
>
> At any rate, I told you already that what is needed
> is not what you've provided but a _device node_
> exported by the kernel, either a processor or
> enclosure type.
Wrong ... we don't export non-SCSI devices as SCSI (with the single and
rather annoying exception of ATA via SAT).
James
--- On Tue, 2/5/08, James Bottomley <[email protected]> wrote:
> > > > I guess the same could be said for STGT and
> SCST,
> > > right?
> > >
> > > You mean both of their kernel pieces are modular?
>
> > > That's correct.
> >
> > No, you know very well what I mean.
> >
> > By the same logic you're preaching to include your
> > solution part of the kernel, you can also apply to
> > SCST.
>
> Ah, but it's not ... the current patch is merely
> exporting an interface.
> The debate in STGT vs SCST is not whether to export an
> interface but
> where to draw the line.
"draw the line" -- I see.
BTW, what is wrong with "exporting the interface"?
What is wrong if both implementations are in the kernel
and then let the users and distros decide which one
they like best and use more? It'll not be the fist time
this has happened in the kernel. Both are actively
maintained.
It seems highly arbitrary to say: "X is in the kernel, Y
is not. If you want Y, just forget about it and fix X."
Give people choice at config time.
This is off topic anyway.
> You could also argue in the same vein that sd is redundant
> because a
> filesystem could talk directly to the device via /dev/sgX
> (in fact OSD
> based filesystems already do this).
Yes, I've mentioned this thing before on this list. Oh, maybe 3
years ago. This is why I had wanted for transport protocols
to export ... (oh, let's not get this off topic).
(Apparently it takes 3 years...)
> The argument is true,
> but misses
> the bigger picture that the interfaces exported by sd are
> more portable
> (apply to non-SCSI block devices) and easier to use.
It isn't quite the same thing. It's like comparing
apples to oranges.
>
> > > > Yes, for which the transport layer,
> implements the
> > > > scsi device node for the SES device. It
> doesn't
> > > really
> > > > matter if the SCSI commands sent to the SES
> device go
> > > > over SGPIO or FC or SAS or Bluetooth or I2C,
> etc, the
> > > > transport layer can implement that and
> present the
> > > > /dev/sgX node.
> > >
> > > But it does matter if the enclosure device
> doesn't
> > > speak SCSI.
> >
> > Enclosure management isn't as simple as you're
> > portraying it here. The enclosure management
> > device speaks either SES or SAF-TE. The transport
> > protocol to access it could be SGPIO or I2C or...
>
> Look, just read the spec; SGPIO is a bus for driving
> enclosures ...
I thought Serial General Purpose Input Output
(SGPIO) was a method to serialize general purpose
IO signals.
> it
> doesn't require SES or SAF-TE or even any SCSI
> protocol.
That's true. And this is why I mentioned a couple
of emails ago to simply export a sgpio device node *IF*
this is what is needed. Of course devices that use SGPIO
abstract it away for their functional purpose, e.g.
enclosures, LED, etc, and provide a more general way to
control it -- highly hardware specific on one side.
Your abstraction currently deals with "SES" devices
and I'd rather leave that to user-space. Alternatively,
which I presume is what you're thinking, a HW specific
core would be using your "abstraction" to provide
some unified access to raw features, and that "unified
access" isn't defined anywhere, and would likely not
be. Alternatively that "unified access" is things
like SES and SAF-TE, which is what vendors prefer
to export, or they prefer to drive this directly
via other means.
That is, I fail to see the kernel bloat, for things
that aren't necessary in the kernel.
If you want your abstraction to fly, it first needs
a common usage model to abstract, and the latter is
missing _from the kernel_.
Unless I don't know the details and you've been
asked to implement this for a single vendor's HW solution.
> > > SGPIO
> > > isn't a SCSI protocol ... it's a general
> purpose
> > > serial bus protocol.
> > > It's pretty simple and register based, but it
> might (or
> > > might not) be
> > > accessible via a SCSI bridge.
> >
> > I see. You've just discovered SGPIO -- good for
> you.
> >
> > At any rate, I told you already that what is needed
> > is not what you've provided but a _device node_
> > exported by the kernel, either a processor or
> > enclosure type.
>
> Wrong ... we don't export non-SCSI devices as SCSI
> (with the single and
> rather annoying exception of ATA via SAT).
I didn't say you should do that. I had already
mentioned that vendors export such controls
as either enclosure or processor type devices,
and this is why I told you that that is what
needs to be exported, which incidentally is
a device node of that type.
Without a common usage model already in the kernel
to abstract (e.g. sd for block device, since you brought
that up) your abstraction seems redundant and arbitrary.
Your kernel code already uses READ DIAGNOSTIC, etc,
and I'd rather leave that to user-space.
Luben
On Tue, 2008-02-05 at 11:33 -0800, Luben Tuikov wrote:
> > Wrong ... we don't export non-SCSI devices as SCSI
> > (with the single and
> > rather annoying exception of ATA via SAT).
>
> I didn't say you should do that. I had already
> mentioned that vendors export such controls
> as either enclosure or processor type devices,
> and this is why I told you that that is what
> needs to be exported, which incidentally is
> a device node of that type.
>
> Without a common usage model already in the kernel
> to abstract (e.g. sd for block device, since you brought
> that up) your abstraction seems redundant and arbitrary.
Exactly, so the first patch in this series (a while ago now) was a
common usage model abstraction of enclosures, and the second was an
implementation in terms of SES. I will do one in terms of SGPIO as
well ... assuming I ever find a SGPIO enclosure ...
> Your kernel code already uses READ DIAGNOSTIC, etc,
> and I'd rather leave that to user-space.
You can do it in user space as well. It's just a bit difficult to get
information out of a SES enclosure without using it, and getting some of
the information is a requirement of the abstraction.
James
--- On Tue, 2/5/08, James Bottomley <[email protected]> wrote:
> > > Wrong ... we don't export non-SCSI devices as
> SCSI
> > > (with the single and
> > > rather annoying exception of ATA via SAT).
> >
> > I didn't say you should do that. I had already
> > mentioned that vendors export such controls
> > as either enclosure or processor type devices,
> > and this is why I told you that that is what
> > needs to be exported, which incidentally is
> > a device node of that type.
> >
> > Without a common usage model already in the kernel
> > to abstract (e.g. sd for block device, since you
> brought
> > that up) your abstraction seems redundant and
> arbitrary.
>
> Exactly, so the first patch in this series (a while ago
^^^^^^^^^^^
See last paragraph.
> now) was a
> common usage model abstraction of enclosures, and the
> second was an
> implementation in terms of SES. I will do one in terms of
> SGPIO as
> well ... assuming I ever find a SGPIO enclosure ...
The vendor would've abstracted that away most commonly
using SES.
>
> > Your kernel code already uses READ DIAGNOSTIC, etc,
> > and I'd rather leave that to user-space.
>
> You can do it in user space as well. It's just a bit
> difficult to get
> information out of a SES enclosure without using it, and
> getting some of
> the information is a requirement of the abstraction.
You missed my point. Your abstraction is redundant and
arbitrary -- it is not based on any known, in-practice,
usage model, already in place that needs a better, common
way of doing XYZ, and therefore needs an abstraction.
Luben
On Sun, 03 Feb 2008 18:16:51 -0600
James Bottomley <[email protected]> wrote:
>
> From: James Bottomley <[email protected]>
> Date: Sun, 3 Feb 2008 15:40:56 -0600
> Subject: [SCSI] enclosure: add support for enclosure services
>
> The enclosure misc device is really just a library providing sysfs
> support for physical enclosure devices and their components.
>
Thanks for sending it out for review.
> +struct enclosure_device *enclosure_find(struct device *dev)
> +{
> + struct enclosure_device *edev = NULL;
> +
> + mutex_lock(&container_list_lock);
> + list_for_each_entry(edev, &container_list, node) {
> + if (edev->cdev.dev == dev) {
> + mutex_unlock(&container_list_lock);
> + return edev;
> + }
> + }
> + mutex_unlock(&container_list_lock);
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(enclosure_find);
This looks a little odd. We don't take a ref on the object after looking
it up, so what prevents some other thread of control from freeing or
otherwise altering the returned object while the caller is playing with it?
> +/**
> + * enclosure_for_each_device - calls a function for each enclosure
> + * @fn: the function to call
> + * @data: the data to pass to each call
> + *
> + * Loops over all the enclosures calling the function.
> + *
> + * Note, this function uses a mutex which will be held across calls to
> + * @fn, so it must have user context, and @fn should not sleep or
Probably "non atomic context" would be more accurate.
fn() actually _can_ sleep.
> + * otherwise cause the mutex to be held for indefinite periods
> + */
> +int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
> + void *data)
> +{
> + int error = 0;
> + struct enclosure_device *edev;
> +
> + mutex_lock(&container_list_lock);
> + list_for_each_entry(edev, &container_list, node) {
> + error = fn(edev, data);
> + if (error)
> + break;
> + }
> + mutex_unlock(&container_list_lock);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(enclosure_for_each_device);
> +
> +/**
> + * enclosure_register - register device as an enclosure
> + *
> + * @dev: device containing the enclosure
> + * @components: number of components in the enclosure
> + *
> + * This sets up the device for being an enclosure. Note that @dev does
> + * not have to be a dedicated enclosure device. It may be some other type
> + * of device that additionally responds to enclosure services
> + */
> +struct enclosure_device *
> +enclosure_register(struct device *dev, const char *name, int components,
> + struct enclosure_component_callbacks *cb)
> +{
> + struct enclosure_device *edev =
> + kzalloc(sizeof(struct enclosure_device) +
> + sizeof(struct enclosure_component)*components,
> + GFP_KERNEL);
> + int err, i;
> +
> + if (!edev)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!cb) {
> + kfree(edev);
> + return ERR_PTR(-EINVAL);
> + }
It would be less fuss if this were to test cb before doing the kzalloc().
Can cb==NULL actually and legitimately happen?
> + edev->components = components;
> +
> + edev->cdev.class = &enclosure_class;
> + edev->cdev.dev = get_device(dev);
> + edev->cb = cb;
> + snprintf(edev->cdev.class_id, BUS_ID_SIZE, "%s", name);
> + err = class_device_register(&edev->cdev);
> + if (err)
> + goto err;
> +
> + for (i = 0; i < components; i++)
> + edev->component[i].number = -1;
> +
> + mutex_lock(&container_list_lock);
> + list_add_tail(&edev->node, &container_list);
> + mutex_unlock(&container_list_lock);
> +
> + return edev;
> +
> + err:
> + put_device(edev->cdev.dev);
> + kfree(edev);
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(enclosure_register);
> +
> +static struct enclosure_component_callbacks enclosure_null_callbacks;
> +
> +/**
> + * enclosure_unregister - remove an enclosure
> + *
> + * @edev: the registered enclosure to remove;
> + */
> +void enclosure_unregister(struct enclosure_device *edev)
> +{
> + int i;
> +
> + if (!edev)
> + return;
Is this legal?
> + mutex_lock(&container_list_lock);
> + list_del(&edev->node);
> + mutex_unlock(&container_list_lock);
See, right now, someone who found this enclosure_device via
enclosure_find() could still be playing with it?
> + for (i = 0; i < edev->components; i++)
> + if (edev->component[i].number != -1)
> + class_device_unregister(&edev->component[i].cdev);
> +
> + /* prevent any callbacks into service user */
> + edev->cb = &enclosure_null_callbacks;
> + class_device_unregister(&edev->cdev);
> +}
> +EXPORT_SYMBOL_GPL(enclosure_unregister);
> +
> +/**
> + * enclosure_component_register - add a particular component to an enclosure
> + * @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.
> + *
> + * 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)
> +{
> + struct enclosure_component *ecomp;
> + struct class_device *cdev;
> + int err;
> +
> + if (!edev || number >= edev->components)
> + return ERR_PTR(-EINVAL);
Is !edev possible and legitimate?
> + ecomp = &edev->component[number];
> +
> + if (ecomp->number != -1)
> + return ERR_PTR(-EINVAL);
> +
> + ecomp->type = type;
> + ecomp->number = number;
> + cdev = &ecomp->cdev;
> + cdev->parent = class_device_get(&edev->cdev);
> + cdev->class = &enclosure_component_class;
> + if (name)
> + snprintf(cdev->class_id, BUS_ID_SIZE, "%s", name);
> + else
> + snprintf(cdev->class_id, BUS_ID_SIZE, "%d", number);
%u :)
> +
> + err = class_device_register(cdev);
> + if (err)
> + ERR_PTR(err);
> +
> + return ecomp;
> +}
> +EXPORT_SYMBOL_GPL(enclosure_component_register);
> +
> +/**
> + * enclosure_add_device - add a device as being part of an enclosure
> + * @edev: the enclosure device being added to.
> + * @num: the number of the component
> + * @dev: the device being added
> + *
> + * Declares a real device to reside in slot (or identifier) @num of an
> + * enclosure. This will cause the relevant sysfs links to appear.
> + * This function may also be used to change a device associated with
> + * an enclosure without having to call enclosure_remove_device() in
> + * between.
> + *
> + * Returns zero on success or an error.
> + */
> +int enclosure_add_device(struct enclosure_device *edev, int component,
> + struct device *dev)
> +{
> + struct class_device *cdev;
> +
> + if (!edev || component >= edev->components)
> + return -EINVAL;
> +
> + cdev = &edev->component[component].cdev;
> +
> + class_device_del(cdev);
> + if (cdev->dev)
> + put_device(cdev->dev);
> + cdev->dev = get_device(dev);
> + return class_device_add(cdev);
> +}
> +EXPORT_SYMBOL_GPL(enclosure_add_device);
> +
> +/*
> + * sysfs pieces below
> + */
> +
> +static ssize_t enclosure_show_components(struct class_device *cdev, char *buf)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev);
> +
> + return snprintf(buf, 40, "%d\n", edev->components);
> +}
"40"?
> +static struct class_device_attribute enclosure_attrs[] = {
> + __ATTR(components, S_IRUGO, enclosure_show_components, NULL),
> + __ATTR_NULL
> +};
> +
> +static struct class enclosure_class = {
> + .name = "enclosure",
> + .owner = THIS_MODULE,
> + .release = enclosure_release,
> + .class_dev_attrs = enclosure_attrs,
> +};
> +
> +static char *enclosure_status [] = {
> + [ENCLOSURE_STATUS_UNSUPPORTED] = "unsupported",
> + [ENCLOSURE_STATUS_OK] = "OK",
> + [ENCLOSURE_STATUS_CRITICAL] = "critical",
> + [ENCLOSURE_STATUS_NON_CRITICAL] = "non-critical",
> + [ENCLOSURE_STATUS_UNRECOVERABLE] = "unrecoverable",
> + [ENCLOSURE_STATUS_NOT_INSTALLED] = "not installed",
> + [ENCLOSURE_STATUS_UNKNOWN] = "unknown",
> + [ENCLOSURE_STATUS_UNAVAILABLE] = "unavailable",
> +};
> +
> +static char *enclosure_type [] = {
> + [ENCLOSURE_COMPONENT_DEVICE] = "device",
> + [ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
> +};
One could play with const here, if sufficiently keen.
> +static ssize_t set_component_fault(struct class_device *cdev, 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);
hrm, we do this conversion about 1e99 times in the kernel and we have to go
and pass three args where only one was needed. katoi()?
> + if (edev->cb->set_fault)
> + edev->cb->set_fault(edev, ecomp, val);
> + return count;
> +}
> +
> +static ssize_t get_component_status(struct class_device *cdev, char *buf)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> + struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +
> + if (edev->cb->get_status)
> + edev->cb->get_status(edev, ecomp);
> + return snprintf(buf, 40, "%s\n", enclosure_status[ecomp->status]);
> +}
> +
> +static ssize_t set_component_status(struct class_device *cdev, const char *buf,
> + size_t count)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> + struct enclosure_component *ecomp = to_enclosure_component(cdev);
> + int i;
> +
> + for (i = 0; enclosure_status[i]; i++) {
> + if (strncmp(buf, enclosure_status[i],
> + strlen(enclosure_status[i])) == 0 &&
> + buf[strlen(enclosure_status[i])] == '\n')
> + break;
> + }
So if an application does
write(fd, "foo", 3)
it won't work? Thye have to do
write(fd, "foo\n", 4)
?
> + if (enclosure_status[i] && edev->cb->set_status) {
> + edev->cb->set_status(edev, ecomp, i);
> + return count;
> + } else
> + return -EINVAL;
> +}
> +
>
> ...
>
> +#define to_enclosure_device(x) container_of((x), struct enclosure_device, cdev)
> +#define to_enclosure_component(x) container_of((x), struct enclosure_component, cdev)
These could be C functions...
> +struct enclosure_device *
> +enclosure_register(struct device *, const char *, int,
> + struct enclosure_component_callbacks *);
> +void enclosure_unregister(struct enclosure_device *);
> +struct enclosure_component *
> +enclosure_component_register(struct enclosure_device *, unsigned int,
> + enum enclosure_component_type, const char *);
> +int enclosure_add_device(struct enclosure_device *enclosure, int component,
> + struct device *dev);
> +int enclosure_remove_device(struct enclosure_device *enclosure, int component);
> +struct enclosure_device *enclosure_find(struct device *dev);
> +int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
> + void *data);
> +
> +#endif /* _LINUX_ENCLOSURE_H_ */
Nice looking driver.
On Tue, 2008-02-05 at 16:12 -0800, Andrew Morton wrote:
> On Sun, 03 Feb 2008 18:16:51 -0600
> James Bottomley <[email protected]> wrote:
>
> >
> > From: James Bottomley <[email protected]>
> > Date: Sun, 3 Feb 2008 15:40:56 -0600
> > Subject: [SCSI] enclosure: add support for enclosure services
> >
> > The enclosure misc device is really just a library providing sysfs
> > support for physical enclosure devices and their components.
> >
>
> Thanks for sending it out for review.
>
> > +struct enclosure_device *enclosure_find(struct device *dev)
> > +{
> > + struct enclosure_device *edev = NULL;
> > +
> > + mutex_lock(&container_list_lock);
> > + list_for_each_entry(edev, &container_list, node) {
> > + if (edev->cdev.dev == dev) {
> > + mutex_unlock(&container_list_lock);
> > + return edev;
> > + }
> > + }
> > + mutex_unlock(&container_list_lock);
> > +
> > + return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(enclosure_find);
>
> This looks a little odd. We don't take a ref on the object after looking
> it up, so what prevents some other thread of control from freeing or
> otherwise altering the returned object while the caller is playing with it?
The use case is for enclosure destruction, so the free should never
happen, but I take the point; I've added a class_device_get().
> > +/**
> > + * enclosure_for_each_device - calls a function for each enclosure
> > + * @fn: the function to call
> > + * @data: the data to pass to each call
> > + *
> > + * Loops over all the enclosures calling the function.
> > + *
> > + * Note, this function uses a mutex which will be held across calls to
> > + * @fn, so it must have user context, and @fn should not sleep or
>
> Probably "non atomic context" would be more accurate.
>
> fn() actually _can_ sleep.
"should" to me means you don't have to do this but ought to. I'll add a
may (but should not).
> > + if (!cb) {
> > + kfree(edev);
> > + return ERR_PTR(-EINVAL);
> > + }
>
> It would be less fuss if this were to test cb before doing the kzalloc().
>
> Can cb==NULL actually and legitimately happen?
Not really ... I'll make it a BUG_ON.
> > +void enclosure_unregister(struct enclosure_device *edev)
> > +{
> > + int i;
> > +
> > + if (!edev)
> > + return;
>
> Is this legal?
No ... it'll oops on the null deref later ... I'll remove this.
> > + mutex_lock(&container_list_lock);
> > + list_del(&edev->node);
> > + mutex_unlock(&container_list_lock);
>
> See, right now, someone who found this enclosure_device via
> enclosure_find() could still be playing with it?
Yes, fixed.
> > + if (!edev || number >= edev->components)
> > + return ERR_PTR(-EINVAL);
>
> Is !edev possible and legitimate?
It shouldn't be, no ... I can remove it.
> > + snprintf(cdev->class_id, BUS_ID_SIZE, "%d", number);
>
> %u :)
Nitpicker!
> > + return snprintf(buf, 40, "%d\n", edev->components);
> > +}
>
> "40"?
I just followed precedence ;-P
There doesn't seem to be a define for this maximum length, so 40 is the
most commonly picked constant.
> > +static char *enclosure_type [] = {
> > + [ENCLOSURE_COMPONENT_DEVICE] = "device",
> > + [ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
> > +};
>
> One could play with const here, if sufficiently keen.
One will try to summon up the enthusiasm.
> > +static ssize_t set_component_fault(struct class_device *cdev, 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);
>
> hrm, we do this conversion about 1e99 times in the kernel and we have to go
> and pass three args where only one was needed. katoi()?
Yes ... I'll add it to the todo list.
> > + for (i = 0; enclosure_status[i]; i++) {
> > + if (strncmp(buf, enclosure_status[i],
> > + strlen(enclosure_status[i])) == 0 &&
> > + buf[strlen(enclosure_status[i])] == '\n')
> > + break;
> > + }
>
> So if an application does
>
> write(fd, "foo", 3)
>
> it won't work? Thye have to do
>
> write(fd, "foo\n", 4)
>
> ?
No ... it's designed for echo; however, I'll add a check for '\0' which
will catch the write case.
> > +#define to_enclosure_device(x) container_of((x), struct enclosure_device, cdev)
> > +#define to_enclosure_component(x) container_of((x), struct enclosure_component, cdev)
>
> These could be C functions...
OK ... I was just following precedence again, but I can make them
inlines.
> Nice looking driver.
Thanks,
James
---
Here's the incremental diff.
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 42e6e43..6fcb0e9 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -39,7 +39,8 @@ static struct class enclosure_component_class;
*
* Looks through the list of registered enclosures to see
* if it can find a match for a device. Returns NULL if no
- * enclosure is found.
+ * enclosure is found. Obtains a reference to the enclosure class
+ * device which must be released with class_device_put().
*/
struct enclosure_device *enclosure_find(struct device *dev)
{
@@ -48,6 +49,7 @@ struct enclosure_device *enclosure_find(struct device *dev)
mutex_lock(&container_list_lock);
list_for_each_entry(edev, &container_list, node) {
if (edev->cdev.dev == dev) {
+ class_device_get(&edev->cdev);
mutex_unlock(&container_list_lock);
return edev;
}
@@ -66,8 +68,9 @@ EXPORT_SYMBOL_GPL(enclosure_find);
* Loops over all the enclosures calling the function.
*
* Note, this function uses a mutex which will be held across calls to
- * @fn, so it must have user context, and @fn should not sleep or
- * otherwise cause the mutex to be held for indefinite periods
+ * @fn, so it must have non atomic context, and @fn may (although it
+ * should not) sleep or otherwise cause the mutex to be held for
+ * indefinite periods
*/
int enclosure_for_each_device(int (*fn)(struct enclosure_device *, void *),
void *data)
@@ -107,14 +110,11 @@ enclosure_register(struct device *dev, const char *name, int components,
GFP_KERNEL);
int err, i;
+ BUG_ON(!cb);
+
if (!edev)
return ERR_PTR(-ENOMEM);
- if (!cb) {
- kfree(edev);
- return ERR_PTR(-EINVAL);
- }
-
edev->components = components;
edev->cdev.class = &enclosure_class;
@@ -152,9 +152,6 @@ void enclosure_unregister(struct enclosure_device *edev)
{
int i;
- if (!edev)
- return;
-
mutex_lock(&container_list_lock);
list_del(&edev->node);
mutex_unlock(&container_list_lock);
@@ -207,7 +204,7 @@ enclosure_component_register(struct enclosure_device *edev,
struct class_device *cdev;
int err;
- if (!edev || number >= edev->components)
+ if (number >= edev->components)
return ERR_PTR(-EINVAL);
ecomp = &edev->component[number];
@@ -223,7 +220,7 @@ enclosure_component_register(struct enclosure_device *edev,
if (name)
snprintf(cdev->class_id, BUS_ID_SIZE, "%s", name);
else
- snprintf(cdev->class_id, BUS_ID_SIZE, "%d", number);
+ snprintf(cdev->class_id, BUS_ID_SIZE, "%u", number);
err = class_device_register(cdev);
if (err)
@@ -313,7 +310,7 @@ static struct class enclosure_class = {
.class_dev_attrs = enclosure_attrs,
};
-static char *enclosure_status [] = {
+static const char *const enclosure_status [] = {
[ENCLOSURE_STATUS_UNSUPPORTED] = "unsupported",
[ENCLOSURE_STATUS_OK] = "OK",
[ENCLOSURE_STATUS_CRITICAL] = "critical",
@@ -324,7 +321,7 @@ static char *enclosure_status [] = {
[ENCLOSURE_STATUS_UNAVAILABLE] = "unavailable",
};
-static char *enclosure_type [] = {
+static const char *const enclosure_type [] = {
[ENCLOSURE_COMPONENT_DEVICE] = "device",
[ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
};
@@ -371,7 +368,8 @@ static ssize_t set_component_status(struct class_device *cdev, const char *buf,
for (i = 0; enclosure_status[i]; i++) {
if (strncmp(buf, enclosure_status[i],
strlen(enclosure_status[i])) == 0 &&
- buf[strlen(enclosure_status[i])] == '\n')
+ (buf[strlen(enclosure_status[i])] == '\n' ||
+ buf[strlen(enclosure_status[i])] == '\0'))
break;
}
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 2565584..54afb80 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -420,9 +420,11 @@ static int ses_intf_add(struct class_device *cdev,
if (!scsi_device_enclosure(sdev)) {
/* not an enclosure, but might be in one */
- edev = enclosure_find(&sdev->host->shost_gendev);
- if (edev)
+ edev = enclosure_find(&sdev->host->shost_gendev);
+ if (edev) {
ses_match_to_enclosure(edev, sdev);
+ class_device_put(&edev->cdev);
+ }
return -ENODEV;
}
@@ -634,6 +636,7 @@ static void ses_intf_remove(struct class_device *cdev,
kfree(edev->component[0].scratch);
+ class_device_put(&edev->cdev);
enclosure_unregister(edev);
}
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 622177a..a5978f1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -100,8 +100,17 @@ struct enclosure_device {
struct enclosure_component component[0];
};
-#define to_enclosure_device(x) container_of((x), struct enclosure_device, cdev)
-#define to_enclosure_component(x) container_of((x), struct enclosure_component, cdev)
+static inline struct enclosure_device *
+to_enclosure_device(struct class_device *dev)
+{
+ return container_of(dev, struct enclosure_device, cdev);
+}
+
+static inline struct enclosure_component *
+to_enclosure_component(struct class_device *dev)
+{
+ return container_of(dev, struct enclosure_component, cdev);
+}
struct enclosure_device *
enclosure_register(struct device *, const char *, int,
On Mon, 4 Feb 2008 18:01:36 -0800 (PST)
Luben Tuikov <[email protected]> wrote:
> --- On Mon, 2/4/08, James Bottomley
> <[email protected]> wrote:
> > > > The enclosure misc device is really just a
> > library providing
> > > > sysfs
> > > > support for physical enclosure devices and their
> > > > components.
> > >
> > > Who is the target audience/user of those facilities?
> > > a) The kernel itself needing to read/write SES pages?
> >
> > That depends on the enclosure integration, but right at the
> > moment, it
> > doesn't
>
> Yes, I didn't suspect so.
>
> >
> > > b) A user space application using sysfs to read/write
> > > SES pages?
> >
> > Not an application so much as a user. The idea of sysfs is
> > to allow
> > users to get and set the information in addition to
> > applications.
>
> Exactly the same argument stands for a user-space
> application with a user-space library.
>
> This is the classical case of where it is better to
> do this in user-space as opposed to the kernel.
>
> The kernel provides capability to access the SES
> device. The user space application and library
> provide interpretation and control. Thus if the
> enclosure were upgraded, one doesn't need to
> upgrade their kernel in order to utilize the new
> capabilities of the SES device. Plus upgrading
> a user-space application is a lot easier than
> the kernel (and no reboot necessary).
>
> Consider another thing: vendors would really like
> unprecedented access to the SES device in the enclosure
> so as your ses/enclosure code keeps state it would
> get out of sync when vendor user-space enclosure
> applications access (and modify) the SES device's
> pages.
>
> You can test this yourself: submit a patch
> that removes SES /dev/sgX support; advertise your
> ses/class solution and watch the fun.
>
> > > At the moment SES device management is done via
> > > an application (user-space) and a user-space library
> > > used by the application and /dev/sgX to send SCSI
> > > commands to the SES device.
> >
> > I must have missed that when I was looking for
> > implementations; what's
> > the URL?
>
> I'm not aware of any GPLed ones. That doesn't
> necessarily mean that the best course of action is
> to bloat the kernel. You can move your ses/enclosure
> stuff to a user space application library
> and thus start a GPLed one.
>
> > But, if we have non-scsi enclosures to integrate, that
> > makes it harder
> > for a user application because it has to know all the
> > implementations.
>
> So does the kernel. And as I pointed out above, it
> is a lot easier to upgrade a user-space application and
> library than it is to upgrade a new kernel and having
> to reboot the computer to run the new kernel.
>
> > A sysfs framework on the other hand is a universal known
> > thing for the
> > user applications.
>
> So would a user-space ses library, a la libses.so.
>
> > > One could have a very good argument to not bloat
> > > the kernel with this but leave it to a user-space
> > > application and a library to do all this and
> > > communicate with the SES device via the kernel's
> > /dev/sgX.
> >
> > The same thing goes for other esoteric SCSI infrastructure
> > pieces like
> > cd changers. On the whole, given that ATA is asking for
> > enclosure
> > management in kernel, it makes sense to consolidate the
> > infrastructure
> > and a ses ULD is a very good test bed.
>
> What is wrong with exporting the SES device as /dev/sgX
> and having a user-space application and library to
> do all this?
>
> Luben
>
Hi,
I apologize for taking so long to review this patch. I obviously agree
wholeheartedly with Luben. The problem I ran into while trying to
design an enclosure management interface for the SATA devices is that
there is all this vendor defined stuff. For example, for the AHCI LED
protocol, the only "defined" LED is 'activity'. For LED2 and LED3 it
is up to hardware vendors to define these. For SGPIO there's all kinds
of ways for hw vendors to customize. I felt that it was going to be a
maintainance nightmare to have to keep track of various vendors
enclosure implementations in the ahci driver, and that it'd be better
to just have user space libraries take care of that. Plus, that way a
vendor doesn't have to get a patch into the kernel to get their new
spiffy wizzy bang blinky lights working (think of how long it takes
something to even get into a vendor kernel, which is what these guys
care about...). So I'm still not sold on having an enclosure
abstraction in the kernel - at least for the SATA controllers.
Kristen
On Tue, 2008-02-12 at 10:22 -0800, Kristen Carlson Accardi wrote:
> I apologize for taking so long to review this patch. I obviously agree
> wholeheartedly with Luben. The problem I ran into while trying to
> design an enclosure management interface for the SATA devices is that
> there is all this vendor defined stuff. For example, for the AHCI LED
> protocol, the only "defined" LED is 'activity'. For LED2 and LED3 it
> is up to hardware vendors to define these. For SGPIO there's all kinds
> of ways for hw vendors to customize. I felt that it was going to be a
> maintainance nightmare to have to keep track of various vendors
> enclosure implementations in the ahci driver, and that it'd be better
> to just have user space libraries take care of that. Plus, that way a
> vendor doesn't have to get a patch into the kernel to get their new
> spiffy wizzy bang blinky lights working (think of how long it takes
> something to even get into a vendor kernel, which is what these guys
> care about...). So I'm still not sold on having an enclosure
> abstraction in the kernel - at least for the SATA controllers.
Correct me if I'm wrong, but didn't the original AHCI enclosure patch
expose activity LEDs via sysfs?
I'm not saying there aren't a lot of non standard pieces that need to be
activated by direct commands or other user activated protocol. I am
saying there are a lot of standard pieces that we could do with showing
in a uniform manner.
The pieces I think are absolutely standard are
1. Actual enclosure presence (is this device in an enclosure)
2. Activity LED, this seems to be a feature of every enclosure.
I also think the following are reasonably standard (based on the fact
that most enclosure standards recommend but don't require this):
3. Locate LED (for locating the device). Even if you only have an
activity LED, this is usually done by flashing the activity LED in a
well defined pattern.
4. Fault. this is the least standardised of the lot, but does seem to
be present in about every enclosure implementation.
All I've done is standardise these four pieces ... the services actually
take into account that it might not be possible to do certain of these
(like fault).
James
On Tue, 12 Feb 2008 12:45:35 -0600
James Bottomley <[email protected]> wrote:
> On Tue, 2008-02-12 at 10:22 -0800, Kristen Carlson Accardi wrote:
> > I apologize for taking so long to review this patch. I obviously
> > agree wholeheartedly with Luben. The problem I ran into while
> > trying to design an enclosure management interface for the SATA
> > devices is that there is all this vendor defined stuff. For
> > example, for the AHCI LED protocol, the only "defined" LED is
> > 'activity'. For LED2 and LED3 it is up to hardware vendors to
> > define these. For SGPIO there's all kinds of ways for hw vendors
> > to customize. I felt that it was going to be a maintainance
> > nightmare to have to keep track of various vendors enclosure
> > implementations in the ahci driver, and that it'd be better to just
> > have user space libraries take care of that. Plus, that way a
> > vendor doesn't have to get a patch into the kernel to get their new
> > spiffy wizzy bang blinky lights working (think of how long it takes
> > something to even get into a vendor kernel, which is what these
> > guys care about...). So I'm still not sold on having an enclosure
> > abstraction in the kernel - at least for the SATA controllers.
>
> Correct me if I'm wrong, but didn't the original AHCI enclosure patch
> expose activity LEDs via sysfs?
You are sort of wrong. we exposed a sysfs entry to enable sofware
controlled activity LED, then the driver was responsible for turning it
on and off. (blech, I know, but some vendors want this feature).
>
> I'm not saying there aren't a lot of non standard pieces that need to
> be activated by direct commands or other user activated protocol. I
> am saying there are a lot of standard pieces that we could do with
> showing in a uniform manner.
>
> The pieces I think are absolutely standard are
>
> 1. Actual enclosure presence (is this device in an enclosure)
> 2. Activity LED, this seems to be a feature of every enclosure.
>
> I also think the following are reasonably standard (based on the fact
> that most enclosure standards recommend but don't require this):
>
> 3. Locate LED (for locating the device). Even if you only have an
> activity LED, this is usually done by flashing the activity LED in a
> well defined pattern.
> 4. Fault. this is the least standardised of the lot, but does seem to
> be present in about every enclosure implementation.
>
> All I've done is standardise these four pieces ... the services
> actually take into account that it might not be possible to do
> certain of these (like fault).
>
> James
>
>
I understand what you are trying to do - I guess I just doubt the value
you've added by doing this. I think that there's going to be so much
customization that system vendors will want to add, that they are going
to wind up adding a custom library regardless, so standardising those
few things won't buy us anything.
On Tue, 2008-02-12 at 11:07 -0800, Kristen Carlson Accardi wrote:
> I understand what you are trying to do - I guess I just doubt the value
> you've added by doing this. I think that there's going to be so much
> customization that system vendors will want to add, that they are going
> to wind up adding a custom library regardless, so standardising those
> few things won't buy us anything.
It depends ... if you actually have a use for the customisations, yes.
If you just want the basics of who (what's in the enclousure), what
(activity) and where (locate) then I think it solves your problem almost
entirely.
So, entirely as a straw horse, tell me what else your enclosures provide
that I haven't listed in the four points. The SES standards too provide
a huge range of things that no-one ever seems to implement (temperature,
power, fan speeds etc).
I think the users of enclosures fall int these categories
85% just want to know where their device actually is (i.e. that sdc is
in enclosure slot 5)
50% like watching the activity lights
30% want to be able to have a visual locate function
20% want a visual failure indication (the other 80% rely on some OS
notification instead)
When you add up the overlapping needs, you get about 90% of people happy
with the basics that the enclosure services provide. Could there be
more ... sure; should there be more ... I don't think so ... that's what
value add the user libraries can provide.
James
--- On Tue, 2/12/08, Kristen Carlson Accardi <[email protected]> wrote:
> Hi,
> I apologize for taking so long to review this patch. I
> obviously agree
> wholeheartedly with Luben. The problem I ran into while
> trying to
> design an enclosure management interface for the SATA
> devices is that
> there is all this vendor defined stuff. For example, for
> the AHCI LED
> protocol, the only "defined" LED is
> 'activity'. For LED2 and LED3 it
> is up to hardware vendors to define these. For SGPIO
> there's all kinds
> of ways for hw vendors to customize. I felt that it was
> going to be a
> maintainance nightmare to have to keep track of various
> vendors
> enclosure implementations in the ahci driver, and that
> it'd be better
> to just have user space libraries take care of that. Plus,
> that way a
> vendor doesn't have to get a patch into the kernel to
> get their new
> spiffy wizzy bang blinky lights working (think of how long
> it takes
> something to even get into a vendor kernel, which is what
> these guys
> care about...). So I'm still not sold on having an
> enclosure
> abstraction in the kernel - at least for the SATA
> controllers.
And I agree wholeheartedly with Kristen.
Luben
--- On Tue, 2/12/08, James Bottomley [email protected]> wrote:
> > I apologize for taking so long to review this patch.
> I obviously agree
> > wholeheartedly with Luben. The problem I ran into
> while trying to
> > design an enclosure management interface for the SATA
> devices is that
> > there is all this vendor defined stuff. For example,
> for the AHCI LED
> > protocol, the only "defined" LED is
> 'activity'. For LED2 and LED3 it
> > is up to hardware vendors to define these. For SGPIO
> there's all kinds
> > of ways for hw vendors to customize. I felt that it
> was going to be a
> > maintainance nightmare to have to keep track of
> various vendors
> > enclosure implementations in the ahci driver, and that
> it'd be better
> > to just have user space libraries take care of that.
> Plus, that way a
> > vendor doesn't have to get a patch into the kernel
> to get their new
> > spiffy wizzy bang blinky lights working (think of how
> long it takes
> > something to even get into a vendor kernel, which is
> what these guys
> > care about...). So I'm still not sold on having
> an enclosure
> > abstraction in the kernel - at least for the SATA
> controllers.
>
> Correct me if I'm wrong, but didn't the original
> AHCI enclosure patch
> expose activity LEDs via sysfs?
>
> I'm not saying there aren't a lot of non standard
> pieces that need to be
> activated by direct commands or other user activated
> protocol. I am
> saying there are a lot of standard pieces that we could do
> with showing
> in a uniform manner.
Which is already the case without the SES kernel bloat.
Case in point, the excellent user-space application
"lsscsi" would clearly show which device is SES.
And the excellent user-space application "sg_ses" could
control an SES device.
> The pieces I think are absolutely standard are
>
> 1. Actual enclosure presence (is this device in an
> enclosure)
"lsscsi"
> 2. Activity LED, this seems to be a feature of every
> enclosure.
So that means that it needs a kernel representation?
If this indeed were the case, for every "feature" of every
type of device (not only SCSI) then the kernel itself would
be insurmountably big.
> I also think the following are reasonably standard (based
> on the fact
> that most enclosure standards recommend but don't
> require this):
>
> 3. Locate LED (for locating the device). Even if you only
> have an
> activity LED, this is usually done by flashing the activity
> LED in a
> well defined pattern.
> 4. Fault. this is the least standardised of the lot, but
> does seem to
> be present in about every enclosure implementation.
>
> All I've done is standardise these four pieces ... the
> services actually
> take into account that it might not be possible to do
> certain of these
> (like fault).
And none of this means that it needs a kernel representation.
1. You're not "standardizing" any known, in-practice,
kernel representation, that is already in practice and
thusly needs a kernel representation.
2. The kernel itself is not using nor needing this
"representation" in order to function properly (the kernel).
Leaving control of SES devices to user-space makes both
the kernel and the vendors happy. All the kernel needs
to do is expose the SES device to user-space as it currently
does. It makes it so much easier both to vendors and to
the kernel to stay out of unnecessary representations.
Vendors may choose to distribute their own applications
to control their hardware, as long as the kernel exposes
an SES device and provides functionality, as opposed to
policy of any kind.
Luben
--- On Tue, 2/12/08, James Bottomley [email protected]> wrote:
> > I understand what you are trying to do - I guess I
> just doubt the value
> > you've added by doing this. I think that
> there's going to be so much
> > customization that system vendors will want to add,
> that they are going
> > to wind up adding a custom library regardless, so
> standardising those
> > few things won't buy us anything.
>
> It depends ... if you actually have a use for the
> customisations, yes.
> If you just want the basics of who (what's in the
> enclousure), what
> (activity) and where (locate) then I think it solves your
> problem almost
> entirely.
If the kernel doesn't solve it "entirely", then it is better
off without the kernel bloat. In fact vendors would distribute
their own user-space application(s) to provide a consistent and
complete solution(s) of their product(s) to their customer(s).
This could also be achieved via "sg_ses", which can also
control custom vendor pages if the encodings are known by the
customer (which they would be if they purchased the product).
> So, entirely as a straw horse, tell me what else your
> enclosures provide
> that I haven't listed in the four points.
You shouldn't insist on this. It should already be clear
to you this direction isn't the vendor's preference.
> The SES
> standards too provide
> a huge range of things that no-one ever seems to implement
> (temperature,
> power, fan speeds etc).
Vendors do use "temperature, power and fan speeds" and
in fact more features, some of them completely custom
for their product, since they end up writing the SES
device server of the enclosure themselves (for their product).
This kind of control and representation is better left to
user-space. The kernel neither needs to represent it
nor know about it, since it is itself not using nor
controlling it.
> I think the users of enclosures fall int these categories
This statement (above) really means that the numbers below
are but merely the speculation of one person.
>
> 85% just want to know where their device actually is (i.e.
> that sdc is
> in enclosure slot 5)
> 50% like watching the activity lights
> 30% want to be able to have a visual locate function
> 20% want a visual failure indication (the other 80% rely on
> some OS
> notification instead)
>
> When you add up the overlapping needs, you get about 90% of
> people happy
> with the basics that the enclosure services provide.
90% doesn't make it a necessary requirement for the kernel
to have this, as well as the fact that the kernel is neither
needing this to function properly, nor using it.
> Could there be more ... sure; should there be more ... I don't think
> so ... that's what value add the user libraries can provide.
"should there be more ... I don't think so"
This decision isn't yours to make. In fact such a decision is never
made by one person only. This decision is made by 1) the industry,
2) the customers and 3) vendors by the pricing of their product(s).
Luben
The keep-it-in-user-space arguments seem fairly compelling to me.
Especially as we've pushed whole i/o subsystems out to user space
(iscsi, stgt, talked about fcoe, a lot of dm control, etc).
The functionality seems to align with Doug's sg/lsscsi utility chain
as well. Granted, the new utility would have to be designed in such
as way that it can incorporate vendor "hardware handlers". But, if
James has a somewhat common implementation already for a kernel
implementation, I'm sure that can be the starting point for lsscsi.
So, the main question I believe is being asked is:
- Do we need to represent this via the object/sysfs tree or can an
outside utility be depended upon to show it ?
Note that I am not supporting:
"Vendors may choose to distribute their own applications".
For this to become truly useful, there needs to be a common tool/method
that presents common features in a common manner, regardless of whether
it is in kernel or not.
-- james s
Luben Tuikov wrote:
> Which is already the case without the SES kernel bloat.
> Case in point, the excellent user-space application
> "lsscsi" would clearly show which device is SES.
>
> And the excellent user-space application "sg_ses" could
> control an SES device.
>
>> The pieces I think are absolutely standard are
>>
>> 1. Actual enclosure presence (is this device in an
>> enclosure)
>
> "lsscsi"
>
>> 2. Activity LED, this seems to be a feature of every
>> enclosure.
>
> So that means that it needs a kernel representation?
> If this indeed were the case, for every "feature" of every
> type of device (not only SCSI) then the kernel itself would
> be insurmountably big.
>
>> I also think the following are reasonably standard (based
>> on the fact
>> that most enclosure standards recommend but don't
>> require this):
>>
>> 3. Locate LED (for locating the device). Even if you only
>> have an
>> activity LED, this is usually done by flashing the activity
>> LED in a
>> well defined pattern.
>> 4. Fault. this is the least standardised of the lot, but
>> does seem to
>> be present in about every enclosure implementation.
>>
>> All I've done is standardise these four pieces ... the
>> services actually
>> take into account that it might not be possible to do
>> certain of these
>> (like fault).
>
> And none of this means that it needs a kernel representation.
>
> 1. You're not "standardizing" any known, in-practice,
> kernel representation, that is already in practice and
> thusly needs a kernel representation.
>
> 2. The kernel itself is not using nor needing this
> "representation" in order to function properly (the kernel).
>
> Leaving control of SES devices to user-space makes both
> the kernel and the vendors happy. All the kernel needs
> to do is expose the SES device to user-space as it currently
> does. It makes it so much easier both to vendors and to
> the kernel to stay out of unnecessary representations.
>
> Vendors may choose to distribute their own applications
> to control their hardware, as long as the kernel exposes
> an SES device and provides functionality, as opposed to
> policy of any kind.
>
> Luben
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Wed, 2008-02-13 at 09:08 -0500, James Smart wrote:
> The keep-it-in-user-space arguments seem fairly compelling to me.
> Especially as we've pushed whole i/o subsystems out to user space
> (iscsi, stgt, talked about fcoe, a lot of dm control, etc).
And to me too.
> The functionality seems to align with Doug's sg/lsscsi utility chain
> as well. Granted, the new utility would have to be designed in such
> as way that it can incorporate vendor "hardware handlers". But, if
> James has a somewhat common implementation already for a kernel
> implementation, I'm sure that can be the starting point for lsscsi.
>
> So, the main question I believe is being asked is:
> - Do we need to represent this via the object/sysfs tree or can an
> outside utility be depended upon to show it ?
>
> Note that I am not supporting:
> "Vendors may choose to distribute their own applications".
> For this to become truly useful, there needs to be a common tool/method
> that presents common features in a common manner, regardless of whether
> it is in kernel or not.
I don't disagree with that, but the fact is that there isn't such a
tool. It's also a fact that the enterprise is reasonably unhappy with
the lack of an enclosure management infrastructure, since it's something
they got on all the other unix systems.
I think a minimal infrastructure in-kernel does just about everything
the enterprise wants ... and since it's stateless, they can always use
direct connect tools in addition.
However, I'm happy to be proven wrong ... anyone on this thread is
welcome to come up with a userland enclosure infrastructure. Once it
does everything the in-kernel one does (which is really about the
minimal possible set), I'll be glad to erase the in-kernel one.
James
James Bottomley wrote:
> I don't disagree with that, but the fact is that there isn't such a
> tool. It's also a fact that the enterprise is reasonably unhappy with
> the lack of an enclosure management infrastructure, since it's something
> they got on all the other unix systems.
I don't disagree.
> I think a minimal infrastructure in-kernel does just about everything
> the enterprise wants ... and since it's stateless, they can always use
> direct connect tools in addition.
>
> However, I'm happy to be proven wrong ... anyone on this thread is
> welcome to come up with a userland enclosure infrastructure. Once it
> does everything the in-kernel one does (which is really about the
> minimal possible set), I'll be glad to erase the in-kernel one.
yeah, but... putting something new in, only to pull it later, is a bad
paradigm for adding new mgmt interfaces. Believe me, I've felt users pain in
the reverse flow : driver-specific stuff that then has to migrate to upstream
interfaces, complicated by different pull points by different distros. You can
migrate a management interface, but can you really remove/pull one out ?
Isn't it better to let the lack of an interface give motivation to create
the "right" interface, once the "right way" is determined - which is what I
thought we were discussing ? or is this simply that there is no motivation
until something exists, that people don't like, thus they become motivated ?
-- james s
On Wed, 2008-02-13 at 11:22 -0500, James Smart wrote:
> James Bottomley wrote:
> > I don't disagree with that, but the fact is that there isn't such a
> > tool. It's also a fact that the enterprise is reasonably unhappy with
> > the lack of an enclosure management infrastructure, since it's something
> > they got on all the other unix systems.
>
> I don't disagree.
>
> > I think a minimal infrastructure in-kernel does just about everything
> > the enterprise wants ... and since it's stateless, they can always use
> > direct connect tools in addition.
> >
> > However, I'm happy to be proven wrong ... anyone on this thread is
> > welcome to come up with a userland enclosure infrastructure. Once it
> > does everything the in-kernel one does (which is really about the
> > minimal possible set), I'll be glad to erase the in-kernel one.
>
> yeah, but... putting something new in, only to pull it later, is a bad
> paradigm for adding new mgmt interfaces. Believe me, I've felt users pain in
> the reverse flow : driver-specific stuff that then has to migrate to upstream
> interfaces, complicated by different pull points by different distros. You can
> migrate a management interface, but can you really remove/pull one out ?
That depends on the result. I agree that migration will be a pain, so I
suppose I set the bar a bit low; the user tool needs to be a bit more
compelling; plus I'll manage the interface transition ... if there is
one; there's no such tool yet.
> Isn't it better to let the lack of an interface give motivation to create
> the "right" interface, once the "right way" is determined - which is what I
> thought we were discussing ? or is this simply that there is no motivation
> until something exists, that people don't like, thus they become motivated ?
Well ... I did learn the latter from Andrew, so I thought I'd try it.
It's certainly true that the enclosure problem has been an issue for
over a decade, so there doesn't seem to be anything motivating anyone to
solve it. I wouldn't have bothered except that I could see ad-hoc
in-kernel sysfs solutions beginning to appear. At least this way they
can all present a unified interface.
James
James Bottomley wrote:
> ... I wouldn't have bothered except that I could see ad-hoc
> in-kernel sysfs solutions beginning to appear.
If this is true, and if no one quickly volunteers to do the utility, then
I agree with what you are doing.
-- james s
On Tue, 12 Feb 2008 13:28:15 -0600
James Bottomley <[email protected]> wrote:
> On Tue, 2008-02-12 at 11:07 -0800, Kristen Carlson Accardi wrote:
> > I understand what you are trying to do - I guess I just doubt the
> > value you've added by doing this. I think that there's going to be
> > so much customization that system vendors will want to add, that
> > they are going to wind up adding a custom library regardless, so
> > standardising those few things won't buy us anything.
>
> It depends ... if you actually have a use for the customisations, yes.
> If you just want the basics of who (what's in the enclousure), what
> (activity) and where (locate) then I think it solves your problem
> almost entirely.
>
> So, entirely as a straw horse, tell me what else your enclosures
> provide that I haven't listed in the four points. The SES standards
> too provide a huge range of things that no-one ever seems to
> implement (temperature, power, fan speeds etc).
>
> I think the users of enclosures fall int these categories
>
> 85% just want to know where their device actually is (i.e. that sdc is
> in enclosure slot 5)
> 50% like watching the activity lights
> 30% want to be able to have a visual locate function
> 20% want a visual failure indication (the other 80% rely on some OS
> notification instead)
>
> When you add up the overlapping needs, you get about 90% of people
> happy with the basics that the enclosure services provide. Could
> there be more ... sure; should there be more ... I don't think so ...
> that's what value add the user libraries can provide.
>
> James
>
>
I don't think I'm arguing whether or not your solution may work, what I
am arguing is really a more philosophical point. Not "can we do it
this way", but "should we do it way". I am of the opinion that
management belongs in userspace. I also am of the opinion that if you
can successfully accomplish something in user space, you should. I
also believe that even if you provide this basic interface, all system
vendors are going to provide libraries on top of that to customize it,
so you've not added much value to just a simple message passing
interface.
So, I'm happy to defer to Jeff's judgement call here - I just want to
do what's right for our customers and get an enclosure management
interface for SATA exposed, preferrably in time for the 2.6.26 merge
window. If he prefers your design, I'll disagree, but commit to his
decision and try to get this to work for SATA. If he'd rather see
something along the lines of what I proposed, then since it is 100% self
contained in the SATA subsystem, it shouldn't impact whatever you
want to do in the SCSI subsystem.
Jeff?
On Wed, 2008-02-13 at 09:45 -0800, Kristen Carlson Accardi wrote:
> I don't think I'm arguing whether or not your solution may work, what I
> am arguing is really a more philosophical point. Not "can we do it
> this way", but "should we do it way". I am of the opinion that
> management belongs in userspace. I also am of the opinion that if you
> can successfully accomplish something in user space, you should. I
> also believe that even if you provide this basic interface, all system
> vendors are going to provide libraries on top of that to customize it,
> so you've not added much value to just a simple message passing
> interface.
I'm not necessarily arguing against that. However, what you're
providing is slightly more than just a userspace tap into the enclosure.
You're adding a file to display and control the enclosure state
(sw_activity). This constitutes an ad-hoc sysfs interface. I'm not
telling you not to do it, but I am pleading that if we have to have all
these sysfs interfaces, lets at least do it in a uniform way.
Enclosures are such nasty beasts, that even the job of getting a tap
into them is problematic, so if we have a different tap infrastructure
for every different enclosure type and connection it's still going to be
pretty unmanageable to a userspace interface.
> So, I'm happy to defer to Jeff's judgement call here - I just want to
> do what's right for our customers and get an enclosure management
> interface for SATA exposed, preferrably in time for the 2.6.26 merge
> window. If he prefers your design, I'll disagree, but commit to his
> decision and try to get this to work for SATA. If he'd rather see
> something along the lines of what I proposed, then since it is 100% self
> contained in the SATA subsystem, it shouldn't impact whatever you
> want to do in the SCSI subsystem.
James
On Wed 2008-02-13 09:45:02, Kristen Carlson Accardi wrote:
> On Tue, 12 Feb 2008 13:28:15 -0600
> James Bottomley <[email protected]> wrote:
>
> > On Tue, 2008-02-12 at 11:07 -0800, Kristen Carlson Accardi wrote:
> > > I understand what you are trying to do - I guess I just doubt the
> > > value you've added by doing this. I think that there's going to be
> > > so much customization that system vendors will want to add, that
> > > they are going to wind up adding a custom library regardless, so
> > > standardising those few things won't buy us anything.
> >
> > It depends ... if you actually have a use for the customisations, yes.
> > If you just want the basics of who (what's in the enclousure), what
> > (activity) and where (locate) then I think it solves your problem
> > almost entirely.
> >
> > So, entirely as a straw horse, tell me what else your enclosures
> > provide that I haven't listed in the four points. The SES standards
> > too provide a huge range of things that no-one ever seems to
> > implement (temperature, power, fan speeds etc).
> >
> > I think the users of enclosures fall int these categories
> >
> > 85% just want to know where their device actually is (i.e. that sdc is
> > in enclosure slot 5)
> > 50% like watching the activity lights
> > 30% want to be able to have a visual locate function
> > 20% want a visual failure indication (the other 80% rely on some OS
> > notification instead)
> >
> > When you add up the overlapping needs, you get about 90% of people
> > happy with the basics that the enclosure services provide. Could
> > there be more ... sure; should there be more ... I don't think so ...
> > that's what value add the user libraries can provide.
> >
> > James
> >
> >
>
> I don't think I'm arguing whether or not your solution may work, what I
> am arguing is really a more philosophical point. Not "can we do it
> this way", but "should we do it way". I am of the opinion that
Hw abstraction is still kernel's job. That's why we have leds exported
in sysfs... let vendors have their libraries, but lets put the
'everyone does these' stuff in kernel.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html