linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH #upstream-fixes] libata: Add transport class for libata
@ 2008-08-19  4:31 Gwendal Grignou
  2008-08-19 11:34 ` Stefan Richter
  2008-08-20  8:17 ` Tejun Heo
  0 siblings, 2 replies; 9+ messages in thread
From: Gwendal Grignou @ 2008-08-19  4:31 UTC (permalink / raw)
  To: IDE/ATA development list; +Cc: linux-scsi

>From df84b33306569f8247f331fd67b0a7e7dfb47936 Mon Sep 17 00:00:00 2001
From: Gwendal Grignou <gwendal@google.com>
Date: Mon, 18 Aug 2008 14:02:09 -0700
Subject: Add transport class for libata.

This patch adds objects for accessing libata objects from user space:
- ata_port class: one per ATA port
- ata_link class: one per ATA port or 15 for SATA Port Multiplier
- ata_device class: up to 2 for PATA link, usually one for SATA.

Each object exports information from the corresponding data strucure.
For instance, ata_device exports id, the cached identify output if
the device is a disk, gscr register output if it is a PM.

More fields can be added later, like error counters and ATA specific
statistics.
---
 drivers/ata/Makefile           |    2 +-
 drivers/ata/libata-core.c      |   24 ++-
 drivers/ata/libata-pmp.c       |   14 +-
 drivers/ata/libata-scsi.c      |   29 +-
 drivers/ata/libata-transport.c |  720 ++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-transport.h |   18 +
 drivers/ata/libata.h           |    4 +
 include/linux/libata.h         |   12 +-
 8 files changed, 797 insertions(+), 26 deletions(-)
 create mode 100644 drivers/ata/libata-transport.c
 create mode 100644 drivers/ata/libata-transport.h

diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 674965f..17746a9 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -79,7 +79,7 @@ obj-$(CONFIG_ATA_GENERIC)	+= ata_generic.o
 # Should be last libata driver
 obj-$(CONFIG_PATA_LEGACY)	+= pata_legacy.o

-libata-objs	:= libata-core.o libata-scsi.o libata-eh.o
+libata-objs	:= libata-core.o libata-scsi.o libata-eh.o libata-transport.o
 libata-$(CONFIG_ATA_SFF)	+= libata-sff.o
 libata-$(CONFIG_SATA_PMP)	+= libata-pmp.o
 libata-$(CONFIG_ATA_ACPI)	+= libata-acpi.o
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5ba96c5..9a21e66 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -64,7 +64,7 @@
 #include <linux/cdrom.h>

 #include "libata.h"
-
+#include "libata-transport.h"

 /* debounce timing parameters in msecs { interval, duration, timeout } */
 const unsigned long sata_deb_timing_normal[]		= {   5,  100, 2000 };
@@ -853,7 +853,7 @@ const char *ata_mode_string(unsigned long xfer_mask)
 	return "<n/a>";
 }

-static const char *sata_spd_string(unsigned int spd)
+const char *sata_spd_string(unsigned int spd)
 {
 	static const char * const spd_str[] = {
 		"1.5 Gbps",
@@ -5156,7 +5156,8 @@ void ata_link_init(struct ata_port *ap, struct
ata_link *link, int pmp)
 	int i;

 	/* clear everything except for devices */
-	memset(link, 0, offsetof(struct ata_link, device[0]));
+	memset((void *)link + ATA_LINK_CLEAR_OFFSET, 0,
+	       offsetof(struct ata_link, device[0]) - ATA_LINK_CLEAR_OFFSET);

 	link->ap = ap;
 	link->pmp = pmp;
@@ -5802,6 +5803,13 @@ static void ata_port_detach(struct ata_port *ap)
 	cancel_rearming_delayed_work(&ap->hotplug_task);

  skip_eh:
+	if (ap->pmp_link) {
+		int i;
+		for (i = 0; i < SATA_PMP_MAX_PORTS; i++)
+			ata_tlink_delete(&ap->pmp_link[i]);
+	}
+	ata_tport_delete(ap);
+
 	/* remove the associated SCSI host */
 	scsi_remove_host(ap->scsi_host);
 }
@@ -6124,9 +6132,17 @@ static int __init ata_init(void)
 	if (!ata_aux_wq)
 		goto free_wq;

+	libata_transport_init();
+	ata_scsi_transport_template = ata_attach_transport();
+	if (ata_scsi_transport_template == NULL) {
+		goto free_aux_wq;
+	}	
+
 	printk(KERN_DEBUG "libata version " DRV_VERSION " loaded.\n");
 	return 0;

+free_aux_wq:
+	destroy_workqueue(ata_aux_wq);
 free_wq:
 	destroy_workqueue(ata_wq);
 free_force_tbl:
@@ -6136,6 +6152,8 @@ free_force_tbl:

 static void __exit ata_exit(void)
 {
+	ata_release_transport(ata_scsi_transport_template);
+	libata_transport_exit();	
 	kfree(ata_force_tbl);
 	destroy_workqueue(ata_wq);
 	destroy_workqueue(ata_aux_wq);
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index b65db30..9b76b67 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/libata.h>
 #include "libata.h"
+#include "libata-transport.h"

 const struct ata_port_operations sata_pmp_port_ops = {
 	.inherits		= &sata_port_ops,
@@ -286,7 +287,7 @@ static int sata_pmp_configure(struct ata_device
*dev, int print_info)
 static int sata_pmp_init_links(struct ata_port *ap, int nr_ports)
 {
 	struct ata_link *pmp_link = ap->pmp_link;
-	int i;
+	int i, err;

 	if (!pmp_link) {
 		pmp_link = kzalloc(sizeof(pmp_link[0]) * SATA_PMP_MAX_PORTS,
@@ -298,6 +299,13 @@ static int sata_pmp_init_links(struct ata_port
*ap, int nr_ports)
 			ata_link_init(ap, &pmp_link[i], i);

 		ap->pmp_link = pmp_link;
+
+		for (i = 0; i < SATA_PMP_MAX_PORTS; i++) {
+			err = ata_tlink_add(&pmp_link[i]);
+			if (err) {
+				goto err_tlink;
+			}
+		}
 	}

 	for (i = 0; i < nr_ports; i++) {
@@ -310,6 +318,10 @@ static int sata_pmp_init_links(struct ata_port
*ap, int nr_ports)
 	}

 	return 0;
+  err_tlink:
+	while (--i >= 0)
+		ata_tlink_delete(&pmp_link[i]);
+	return err;
 }

 static void sata_pmp_quirks(struct ata_port *ap)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9d3ba4..43eb14a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -48,6 +48,7 @@
 #include <linux/uaccess.h>

 #include "libata.h"
+#include "libata-transport.h"

 #define SECTOR_SIZE		512
 #define ATA_SCSI_RBUF_SIZE	4096
@@ -61,9 +62,6 @@ static struct ata_device *__ata_scsi_find_dev(struct
ata_port *ap,
 					const struct scsi_device *scsidev);
 static struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
 					    const struct scsi_device *scsidev);
-static int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
-			      unsigned int id, unsigned int lun);
-

 #define RW_RECOVERY_MPAGE 0x1
 #define RW_RECOVERY_MPAGE_LEN 12
@@ -103,17 +101,6 @@ static const u8 def_control_mpage[CONTROL_MPAGE_LEN] = {
 	0, 30	/* extended self test time, see 05-359r1 */
 };

-/*
- * libata transport template.  libata doesn't do real transport stuff.
- * It just needs the eh_timed_out hook.
- */
-static struct scsi_transport_template ata_scsi_transport_template = {
-	.eh_strategy_handler	= ata_scsi_error,
-	.eh_timed_out		= ata_scsi_timed_out,
-	.user_scan		= ata_scsi_user_scan,
-};
-
-
 static const struct {
 	enum link_pm	value;
 	const char	*name;
@@ -3072,7 +3059,7 @@ int ata_scsi_add_hosts(struct ata_host *host,
struct scsi_host_template *sht)
 		*(struct ata_port **)&shost->hostdata[0] = ap;
 		ap->scsi_host = shost;

-		shost->transportt = &ata_scsi_transport_template;
+		shost->transportt = ata_scsi_transport_template;
 		shost->unique_id = ap->print_id;
 		shost->max_id = 16;
 		shost->max_lun = 1;
@@ -3089,6 +3076,10 @@ int ata_scsi_add_hosts(struct ata_host *host,
struct scsi_host_template *sht)
 		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
 		if (rc)
 			goto err_add;
+		rc = ata_tport_add(host->dev,ap);
+		if (rc) {
+			goto err_add;
+		}
 	}

 	return 0;
@@ -3097,8 +3088,10 @@ int ata_scsi_add_hosts(struct ata_host *host,
struct scsi_host_template *sht)
 	scsi_host_put(host->ports[i]->scsi_host);
  err_alloc:
 	while (--i >= 0) {
-		struct Scsi_Host *shost = host->ports[i]->scsi_host;
+		struct ata_port *ap = host->ports[i];
+		struct Scsi_Host *shost = ap->scsi_host;

+		ata_tport_delete(ap);
 		scsi_remove_host(shost);
 		scsi_host_put(shost);
 	}
@@ -3355,8 +3348,8 @@ void ata_scsi_hotplug(struct work_struct *work)
  *	RETURNS:
  *	Zero.
  */
-static int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
-			      unsigned int id, unsigned int lun)
+int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
+		       unsigned int id, unsigned int lun)
 {
 	struct ata_port *ap = ata_shost_to_port(shost);
 	unsigned long flags;
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
new file mode 100644
index 0000000..6f0347c
--- /dev/null
+++ b/drivers/ata/libata-transport.c
@@ -0,0 +1,720 @@
+/*
+ * Copyright (C) 2005-2006 Dell Inc.
+ *	Released under GPL v2.
+ *
+ * Libata transport class.
+ *
+ * The ATA transport class contains common code to deal with ATA HBAs,
+ * an aproximated representation of ATA topologies in the driver model,
+ * and various sysfs attributes to expose these topologies and managment
+ * interfaces to userspace.
+ *
+ * There are 3 objects defined in in this class:
+ * - ata_port
+ * - ata_link
+ * - ata_device
+ * Each port has a link object. Each link can have up to two devices for PATA
+ * and generally one for SATA.
+ * If there is SATA port multiplier [PMP], 15 additional ata_link object are
+ * created.
+ *
+ * These objects are created when the ata host is initalized and when a PMP is
+ * found. They are removed only when the HBA is removed, cleaned before the
+ * error handler runs.
+ */
+
+
+#include <linux/kernel.h>
+#include <linux/blkdev.h>
+#include <linux/spinlock.h>
+#include <scsi/scsi_transport.h>
+#include <linux/libata.h>
+#include <linux/hdreg.h>
+#include <linux/uaccess.h>
+
+#include "libata.h"
+#include "libata-transport.h"
+
+#define ATA_PORT_ATTRS		2
+#define ATA_LINK_ATTRS		3
+#define ATA_DEV_ATTRS		8
+
+struct scsi_transport_template;
+struct scsi_transport_template *ata_scsi_transport_template;
+
+struct ata_internal {
+	struct scsi_transport_template t;
+
+	struct device_attribute private_port_attrs[ATA_PORT_ATTRS];
+	struct device_attribute private_link_attrs[ATA_LINK_ATTRS];
+	struct device_attribute private_dev_attrs[ATA_DEV_ATTRS];
+
+	struct transport_container link_attr_cont;
+	struct transport_container dev_attr_cont;
+
+	/*
+	 * The array of null terminated pointers to attributes
+	 * needed by scsi_sysfs.c
+	 */
+	struct device_attribute *link_attrs[ATA_LINK_ATTRS + 1];
+	struct device_attribute *port_attrs[ATA_PORT_ATTRS + 1];
+	struct device_attribute *dev_attrs[ATA_DEV_ATTRS + 1];
+};
+#define to_ata_internal(tmpl)	container_of(tmpl, struct ata_internal, t)
+
+
+#define tdev_to_device(d)					\
+	container_of((d), struct ata_device, tdev)
+#define transport_class_to_dev(dev)				\
+	tdev_to_device((dev)->parent)
+
+#define tdev_to_link(d)						\
+	container_of((d), struct ata_link, tdev)
+#define transport_class_to_link(dev)				\
+	tdev_to_link((dev)->parent)
+
+#define tdev_to_port(d)						\
+	container_of((d), struct ata_port, tdev)
+#define transport_class_to_port(dev)				\
+	tdev_to_port((dev)->parent)
+
+
+/* Device objects are always created whit link objects */
+static int ata_tdev_add(struct ata_device *dev);
+static void ata_tdev_delete(struct ata_device *dev);
+
+
+/*
+ * Hack to allow attributes of the same name in different objects.
+ */
+#define ATA_DEVICE_ATTR(_prefix,_name,_mode,_show,_store) \
+	struct device_attribute device_attr_##_prefix##_##_name = \
+	__ATTR(_name,_mode,_show,_store)
+
+#define ata_bitfield_name_match(title, table)			\
+static ssize_t							\
+get_ata_##title##_names(u32 table_key, char *buf)		\
+{								\
+	char *prefix = "";					\
+	ssize_t len = 0;					\
+	int i;							\
+								\
+	for (i = 0; i < ARRAY_SIZE(table); i++) {		\
+		if (table[i].value & table_key) {		\
+			len += sprintf(buf + len, "%s%s",	\
+				prefix, table[i].name);		\
+			prefix = ", ";				\
+		}						\
+	}							\
+	len += sprintf(buf + len, "\n");			\
+	return len;						\
+}
+
+#define ata_bitfield_name_search(title, table)			\
+static ssize_t							\
+get_ata_##title##_names(u32 table_key, char *buf)		\
+{								\
+	ssize_t len = 0;					\
+	int i;							\
+								\
+	for (i = 0; i < ARRAY_SIZE(table); i++) {		\
+		if (table[i].value == table_key) {		\
+			len += sprintf(buf + len, "%s",		\
+				table[i].name);			\
+			break;					\
+		}						\
+	}							\
+	len += sprintf(buf + len, "\n");			\
+	return len;						\
+}
+
+static struct {
+	u32		value;
+	char		*name;
+} ata_class_names[] = {
+	{ ATA_DEV_UNKNOWN,		"unknown" },
+	{ ATA_DEV_ATA,			"ata" },
+	{ ATA_DEV_ATA_UNSUP,		"ata" },
+	{ ATA_DEV_ATAPI,		"atapi" },
+	{ ATA_DEV_ATAPI_UNSUP,		"atapi" },
+	{ ATA_DEV_PMP,			"pmp" },
+	{ ATA_DEV_PMP_UNSUP,		"pmp" },
+	{ ATA_DEV_SEMB,			"semb" },
+	{ ATA_DEV_SEMB_UNSUP,		"semb" },
+	{ ATA_DEV_NONE,			"none" }
+};
+ata_bitfield_name_search(class, ata_class_names)
+
+static struct {
+	u32		value;
+	char		*name;
+} ata_xfer_names[] = {
+	{ XFER_UDMA_7,			"XFER_UDMA_7" },
+	{ XFER_UDMA_6,			"XFER_UDMA_6" },
+	{ XFER_UDMA_5,			"XFER_UDMA_5" },
+	{ XFER_UDMA_4,			"XFER_UDMA_4" },
+	{ XFER_UDMA_3,			"XFER_UDMA_3" },
+	{ XFER_UDMA_2,			"XFER_UDMA_2" },
+	{ XFER_UDMA_1,			"XFER_UDMA_1" },
+	{ XFER_UDMA_0,			"XFER_UDMA_0" },
+	{ XFER_MW_DMA_4,		"XFER_MW_DMA_4" },
+	{ XFER_MW_DMA_3,		"XFER_MW_DMA_3" },
+	{ XFER_MW_DMA_2,		"XFER_MW_DMA_2" },
+	{ XFER_MW_DMA_1,		"XFER_MW_DMA_1" },
+	{ XFER_MW_DMA_0,		"XFER_MW_DMA_0" },
+	{ XFER_SW_DMA_2,		"XFER_SW_DMA_2" },
+	{ XFER_SW_DMA_1,		"XFER_SW_DMA_1" },
+	{ XFER_SW_DMA_0,		"XFER_SW_DMA_0" },
+	{ XFER_PIO_6,			"XFER_PIO_6" },
+	{ XFER_PIO_5,			"XFER_PIO_5" },
+	{ XFER_PIO_4,			"XFER_PIO_4" },
+	{ XFER_PIO_3,			"XFER_PIO_3" },
+	{ XFER_PIO_2,			"XFER_PIO_2" },
+	{ XFER_PIO_1,			"XFER_PIO_1" },
+	{ XFER_PIO_0,			"XFER_PIO_0" },
+	{ XFER_PIO_SLOW,		"XFER_PIO_SLOW" }
+};
+ata_bitfield_name_match(xfer,ata_xfer_names)
+
+/*
+ * ATA Port attributes
+ */
+#define ata_port_show_simple(field, name, format_string, cast)		\
+static ssize_t								\
+show_ata_port_##name(struct device *dev,				\
+		     struct device_attribute *attr, char *buf)		\
+{									\
+	struct ata_port *ap = transport_class_to_port(dev);		\
+									\
+	return snprintf(buf, 20, format_string, cast ap->field);	\
+}
+
+#define ata_port_simple_attr(field, name, format_string, type)		\
+	ata_port_show_simple(field, name, format_string, (type))	\
+static DEVICE_ATTR(name, S_IRUGO, show_ata_port_##name, NULL)
+
+ata_port_simple_attr(nr_pmp_links, nr_pmp_links, "%d\n", int);
+ata_port_simple_attr(stats.idle_irq, idle_irq, "%ld\n", unsigned long);
+
+static DECLARE_TRANSPORT_CLASS(ata_port_class,
+			       "ata_port", NULL, NULL, NULL);
+
+static void ata_tport_release(struct device *dev)
+{
+	put_device(dev->parent);
+}
+
+/**
+ * ata_is_port --  check if a struct device represents a ATA port
+ * @dev:	device to check
+ *
+ * Returns:
+ *	%1 if the device represents a ATA Port, %0 else
+ */
+int ata_is_port(const struct device *dev)
+{
+	return dev->release == ata_tport_release;
+}
+
+static int ata_tport_match(struct attribute_container *cont,
+			   struct device *dev)
+{
+	if (!ata_is_port(dev))
+		return 0;
+	return &ata_scsi_transport_template->host_attrs.ac == cont;
+}
+
+/**
+ * ata_tport_delete  --  remove ATA PORT
+ * @port:	ATA PORT to remove
+ *
+ * Removes the specified ATA PORT.  Remove the associated link as well.
+ */
+void ata_tport_delete(struct ata_port *ap)
+{
+	struct device *dev = &ap->tdev;
+
+	ata_tlink_delete(&ap->link);
+
+	transport_remove_device(dev);
+	device_del(dev);
+	transport_destroy_device(dev);
+	put_device(dev);
+}
+
+/** ata_tport_add - initialize a transport ATA port structure
+ *
+ * @parent:	parent device
+ * @ap:		existing ata_port structure
+ *
+ * Initialize a ATA port structure for sysfs.  It will be added to the device
+ * tree below the device specified by @parent which could be a PCI device.
+ *
+ * Returns %0 on success
+ */
+int ata_tport_add(struct device *parent, 	
+		  struct ata_port *ap)
+{
+	int error;
+	struct device *dev = &ap->tdev;
+
+	device_initialize(dev);
+
+	dev->parent = get_device(parent);
+	dev->release = ata_tport_release;
+	sprintf(dev->bus_id, "ata%d", ap->print_id);
+	transport_setup_device(dev);
+	error = device_add(dev);
+	if (error) {
+		goto tport_err;
+	}
+
+	transport_add_device(dev);
+	transport_configure_device(dev);
+
+	error = ata_tlink_add(&ap->link);
+	if (error) {
+		goto tport_link_err;
+	}
+	return 0;
+
+ tport_link_err:
+	transport_remove_device(dev);
+	device_del(dev);
+
+ tport_err:
+	transport_destroy_device(dev);
+	put_device(dev);
+	return error;
+}
+
+
+/*
+ * ATA link attributes
+ */
+
+
+#define ata_link_show_linkspeed(field)					\
+static ssize_t								\
+show_ata_link_##field(struct device *dev,				\
+		      struct device_attribute *attr, char *buf)		\
+{									\
+	struct ata_link *link = transport_class_to_link(dev);		\
+									\
+	return sprintf(buf,"%s\n", sata_spd_string(fls(link->field)));	\
+}
+
+#define ata_link_linkspeed_attr(field)					\
+	ata_link_show_linkspeed(field)					\
+static DEVICE_ATTR(field, S_IRUGO, show_ata_link_##field, NULL)
+
+ata_link_linkspeed_attr(hw_sata_spd_limit);
+ata_link_linkspeed_attr(sata_spd_limit);
+ata_link_linkspeed_attr(sata_spd);
+
+
+static DECLARE_TRANSPORT_CLASS(ata_link_class,
+		"ata_link", NULL, NULL, NULL);
+
+static void ata_tlink_release(struct device *dev)
+{
+	put_device(dev->parent);
+}
+
+/**
+ * ata_is_link --  check if a struct device represents a ATA link
+ * @dev:	device to check
+ *
+ * Returns:
+ *	%1 if the device represents a ATA link, %0 else
+ */
+int ata_is_link(const struct device *dev)
+{
+	return dev->release == ata_tlink_release;
+}
+
+static int ata_tlink_match(struct attribute_container *cont,
+			   struct device *dev)
+{
+	struct ata_internal* i = to_ata_internal(ata_scsi_transport_template);
+	if (!ata_is_link(dev))
+		return 0;
+	return &i->link_attr_cont.ac == cont;
+}
+
+/**
+ * ata_tlink_delete  --  remove ATA LINK
+ * @port:	ATA LINK to remove
+ *
+ * Removes the specified ATA LINK.  remove associated ATA device(s) as well.
+ */
+void ata_tlink_delete(struct ata_link *link)
+{
+	struct device *dev = &link->tdev;
+	struct ata_device *ata_dev;
+
+	ata_link_for_each_dev(ata_dev, link) {
+		ata_tdev_delete(ata_dev);
+	}
+
+	transport_remove_device(dev);
+	device_del(dev);
+	transport_destroy_device(dev);
+	put_device(dev);
+}
+
+/**
+ * ata_tlink_add  --  initialize a transport ATA link structure
+ * @link:	allocated ata_link structure.
+ *
+ * Initlialize an ATA LINK structure for sysfs.  It will be added in the
+ * device tree below the ATA PORT itbelongs to.
+ *
+ * Returns %0 on success
+ */
+int ata_tlink_add(struct ata_link *link)
+{
+	struct device *dev = &link->tdev;
+	struct ata_port *ap = link->ap;
+	struct ata_device *ata_dev;
+	int error;
+
+	device_initialize(dev);
+	dev->parent = get_device(&ap->tdev);
+	dev->release = ata_tlink_release;
+	if (ata_is_host_link(link))
+		sprintf(dev->bus_id, "link%d", ap->print_id);
+        else
+		sprintf(dev->bus_id, "link%d.%d", ap->print_id, link->pmp);
+
+	transport_setup_device(dev);
+
+	error = device_add(dev);
+	if (error) {
+		goto tlink_err;
+	}
+
+	transport_add_device(dev);
+	transport_configure_device(dev);
+	
+	ata_link_for_each_dev(ata_dev, link) {
+		error = ata_tdev_add(ata_dev);
+		if (error) {
+			goto tlink_dev_err;
+		}
+	}
+	return 0;
+  tlink_dev_err:
+	while (--ata_dev >= link->device) {
+		ata_tdev_delete(ata_dev);
+	}
+	transport_remove_device(dev);
+	device_del(dev);
+  tlink_err:
+	transport_destroy_device(dev);
+	put_device(dev);
+	return error;
+}
+
+/*
+ * ATA device attributes
+ */
+
+#define ata_dev_show_class(title, field)				\
+static ssize_t								\
+show_ata_dev_##field(struct device *dev,				\
+		     struct device_attribute *attr, char *buf)		\
+{									\
+	struct ata_device *ata_dev = transport_class_to_dev(dev);	\
+									\
+	return get_ata_##title##_names(ata_dev->field, buf);		\
+}
+
+#define ata_dev_attr(title, field)					\
+	ata_dev_show_class(title, field)				\
+static DEVICE_ATTR(field, S_IRUGO, show_ata_dev_##field, NULL)
+
+ata_dev_attr(class, class);
+ata_dev_attr(xfer, pio_mode);
+ata_dev_attr(xfer, dma_mode);
+ata_dev_attr(xfer, xfer_mode);
+
+
+#define ata_dev_show_simple(field, format_string, cast)		\
+static ssize_t								\
+show_ata_dev_##field(struct device *dev,				\
+		     struct device_attribute *attr, char *buf)		\
+{									\
+	struct ata_device *ata_dev = transport_class_to_dev(dev);	\
+									\
+	return snprintf(buf, 20, format_string, cast ata_dev->field);	\
+}
+
+#define ata_dev_simple_attr(field, format_string, type)	\
+	ata_dev_show_simple(field, format_string, (type))	\
+static DEVICE_ATTR(field, S_IRUGO, 			\
+		   show_ata_dev_##field, NULL)
+
+ata_dev_simple_attr(spdn_cnt, "%d\n", int);
+
+static ssize_t
+show_ata_dev_id(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ata_device *ata_dev = transport_class_to_dev(dev);
+	int written = 0, i = 0;
+
+	if (ata_dev->class == ATA_DEV_PMP)
+		return 0;
+	for(i=0;i<ATA_ID_WORDS;i++)  {
+		written += snprintf(buf+written, 20, "%04x%c",
+				    ata_dev->id[i],
+				    ((i+1) & 7) ? ' ' : '\n');
+	}
+	return written;
+}
+
+static DEVICE_ATTR(id, S_IRUGO, show_ata_dev_id, NULL);
+
+static ssize_t
+show_ata_dev_gscr(struct device *dev,
+		  struct device_attribute *attr, char *buf)
+{
+	struct ata_device *ata_dev = transport_class_to_dev(dev);
+	int written = 0, i = 0;
+
+	if (ata_dev->class != ATA_DEV_PMP)
+		return 0;
+	for(i=0;i<SATA_PMP_GSCR_DWORDS;i++)  {
+		written += snprintf(buf+written, 20, "%08x%c",
+				    ata_dev->gscr[i],
+				    ((i+1) & 3) ? ' ' : '\n');
+	}
+	return written;
+}
+
+static DEVICE_ATTR(gscr, S_IRUGO, show_ata_dev_gscr, NULL);
+
+static DECLARE_TRANSPORT_CLASS(ata_dev_class,
+			       "ata_device", NULL, NULL, NULL);
+
+static void ata_tdev_release(struct device *dev)
+{
+	put_device(dev->parent);
+}
+
+/**
+ * ata_is_ata_dev  --  check if a struct device represents a ATA device
+ * @dev:	device to check
+ *
+ * Returns:
+ *	%1 if the device represents a ATA device, %0 else
+ */
+int ata_is_ata_dev(const struct device *dev)
+{
+	return dev->release == ata_tdev_release;
+}
+
+static int ata_tdev_match(struct attribute_container *cont,
+			  struct device *dev)
+{
+	struct ata_internal* i = to_ata_internal(ata_scsi_transport_template);
+	if (!ata_is_ata_dev(dev))
+		return 0;
+	return &i->dev_attr_cont.ac == cont;
+}
+
+/**
+ * ata_tdev_free  --  free a ATA LINK
+ * @dev:	ATA PHY to free
+ *
+ * Frees the specified ATA PHY.
+ *
+ * Note:
+ *   This function must only be called on a PHY that has not
+ *   sucessfully been added using ata_tdev_add().
+ */
+static void ata_tdev_free(struct ata_device *dev)
+{
+	transport_destroy_device(&dev->tdev);
+	put_device(&dev->tdev);
+}
+
+/**
+ * ata_tdev_delete  --  remove ATA device
+ * @port:	ATA PORT to remove
+ *
+ * Removes the specified ATA device.
+ */
+static void ata_tdev_delete(struct ata_device *ata_dev)
+{
+	struct device *dev = &ata_dev->tdev;
+
+	transport_remove_device(dev);
+	device_del(dev);
+	ata_tdev_free(ata_dev);
+}
+
+
+/**
+ * ata_tdev_add  --  initialize a transport ATA device structure.
+ * @ata_dev:	ata_dev structure.
+ *
+ * Initialize an ATA device structure for sysfs.  It will be added in the
+ * device tree below the ATA LINK device it belongs to.
+ *
+ * Returns %0 on success
+ */
+static int ata_tdev_add(struct ata_device *ata_dev)
+{
+	struct device *dev = &ata_dev->tdev;
+	struct ata_link *link = ata_dev->link;
+	struct ata_port *ap = link->ap;
+	int error;
+
+	device_initialize(dev);
+	dev->parent = get_device(&link->tdev);
+	dev->release = ata_tdev_release;
+	if (ata_is_host_link(link))
+		sprintf(dev->bus_id, "dev%d.%d",
+			ap->print_id,ata_dev->devno);
+        else
+		sprintf(dev->bus_id, "dev%d.%d.0",
+			ap->print_id, link->pmp);
+
+	transport_setup_device(dev);
+	error = device_add(dev);
+	if (error) {
+		ata_tdev_free(ata_dev);
+		return error;
+	}
+
+	transport_add_device(dev);
+	transport_configure_device(dev);
+	return 0;
+}
+
+
+/*
+ * Setup / Teardown code
+ */
+
+#define SETUP_TEMPLATE(attrb, field, perm, test)			\
+	i->private_##attrb[count] = dev_attr_##field;		       	\
+	i->private_##attrb[count].attr.mode = perm;			\
+	i->attrb[count] = &i->private_##attrb[count];			\
+	if (test)							\
+		count++
+
+#define SETUP_LINK_ATTRIBUTE(field)					\
+	SETUP_TEMPLATE(link_attrs, field, S_IRUGO, 1)
+
+#define SETUP_PORT_ATTRIBUTE(field)					\
+	SETUP_TEMPLATE(port_attrs, field, S_IRUGO, 1)
+
+#define SETUP_DEV_ATTRIBUTE(field)					\
+	SETUP_TEMPLATE(dev_attrs, field, S_IRUGO, 1)
+
+/**
+ * ata_attach_transport  --  instantiate ATA transport template
+ */
+struct scsi_transport_template *ata_attach_transport(void)
+{
+	struct ata_internal *i;
+	int count;
+
+	i = kzalloc(sizeof(struct ata_internal), GFP_KERNEL);
+	if (!i)
+		return NULL;
+
+	i->t.eh_strategy_handler	= ata_scsi_error;
+	i->t.eh_timed_out		= ata_scsi_timed_out;
+	i->t.user_scan			= ata_scsi_user_scan;
+
+	i->t.host_attrs.ac.attrs = &i->port_attrs[0];
+	i->t.host_attrs.ac.class = &ata_port_class.class;
+	i->t.host_attrs.ac.match = ata_tport_match;
+	transport_container_register(&i->t.host_attrs);
+
+	i->link_attr_cont.ac.class = &ata_link_class.class;
+	i->link_attr_cont.ac.attrs = &i->link_attrs[0];
+	i->link_attr_cont.ac.match = ata_tlink_match;
+	transport_container_register(&i->link_attr_cont);
+
+	i->dev_attr_cont.ac.class = &ata_dev_class.class;
+	i->dev_attr_cont.ac.attrs = &i->dev_attrs[0];
+	i->dev_attr_cont.ac.match = ata_tdev_match;
+	transport_container_register(&i->dev_attr_cont);
+
+	count = 0;
+	SETUP_PORT_ATTRIBUTE(nr_pmp_links);
+	SETUP_PORT_ATTRIBUTE(idle_irq);
+	BUG_ON(count > ATA_PORT_ATTRS);
+	i->port_attrs[count] = NULL;
+
+	count = 0;
+	SETUP_LINK_ATTRIBUTE(hw_sata_spd_limit);
+	SETUP_LINK_ATTRIBUTE(sata_spd_limit);
+	SETUP_LINK_ATTRIBUTE(sata_spd);
+	BUG_ON(count > ATA_LINK_ATTRS);
+	i->link_attrs[count] = NULL;
+
+	count = 0;
+	SETUP_DEV_ATTRIBUTE(class);
+	SETUP_DEV_ATTRIBUTE(pio_mode);
+	SETUP_DEV_ATTRIBUTE(dma_mode);
+	SETUP_DEV_ATTRIBUTE(xfer_mode);
+	SETUP_DEV_ATTRIBUTE(spdn_cnt);
+	SETUP_DEV_ATTRIBUTE(id);
+	SETUP_DEV_ATTRIBUTE(gscr);
+	BUG_ON(count > ATA_DEV_ATTRS);
+	i->dev_attrs[count] = NULL;
+
+	return &i->t;
+}
+
+/**
+ * ata_release_transport  --  release ATA transport template instance
+ * @t:		transport template instance
+ */
+void ata_release_transport(struct scsi_transport_template *t)
+{
+	struct ata_internal *i = to_ata_internal(t);
+
+	transport_container_unregister(&i->t.host_attrs);
+	transport_container_unregister(&i->link_attr_cont);
+	transport_container_unregister(&i->dev_attr_cont);
+
+	kfree(i);
+}
+
+__init int libata_transport_init(void)
+{
+	int error;
+
+	error = transport_class_register(&ata_link_class);
+	if (error)
+		goto out_unregister_transport;
+	error = transport_class_register(&ata_port_class);
+	if (error)
+		goto out_unregister_link;
+	error = transport_class_register(&ata_dev_class);
+	if (error)
+		goto out_unregister_port;
+	return 0;
+
+ out_unregister_port:
+	transport_class_unregister(&ata_port_class);
+ out_unregister_link:
+	transport_class_unregister(&ata_link_class);
+ out_unregister_transport:
+	return error;
+
+}
+
+void __exit libata_transport_exit(void)
+{
+	transport_class_unregister(&ata_link_class);
+	transport_class_unregister(&ata_port_class);
+	transport_class_unregister(&ata_dev_class);
+}
diff --git a/drivers/ata/libata-transport.h b/drivers/ata/libata-transport.h
new file mode 100644
index 0000000..2820cf8
--- /dev/null
+++ b/drivers/ata/libata-transport.h
@@ -0,0 +1,18 @@
+#ifndef _LIBATA_TRANSPORT_H
+#define _LIBATA_TRANSPORT_H
+
+
+extern struct scsi_transport_template *ata_scsi_transport_template;
+
+int ata_tlink_add(struct ata_link *link);
+void ata_tlink_delete(struct ata_link *link);
+
+int ata_tport_add(struct device *parent, struct ata_port *ap);
+void ata_tport_delete(struct ata_port *ap);
+
+struct scsi_transport_template *ata_attach_transport(void);
+void ata_release_transport(struct scsi_transport_template *t);
+
+__init int libata_transport_init(void);
+void __exit libata_transport_exit(void);
+#endif
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index ade5c75..d262d87 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -115,6 +115,7 @@ extern int ata_cmd_ioctl(struct scsi_device
*scsidev, void __user *arg);
 extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern void ata_dev_enable_pm(struct ata_device *dev, enum link_pm policy);
 extern void ata_lpm_schedule(struct ata_port *ap, enum link_pm);
+extern const char *sata_spd_string(unsigned int spd);

 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
@@ -148,6 +149,9 @@ extern void ata_scsi_hotplug(struct work_struct *work);
 extern void ata_schedule_scsi_eh(struct Scsi_Host *shost);
 extern void ata_scsi_dev_rescan(struct work_struct *work);
 extern int ata_bus_probe(struct ata_port *ap);
+extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
+			      unsigned int id, unsigned int lun);
+

 /* libata-eh.c */
 extern unsigned long ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 06b8033..e117d5a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -556,13 +556,16 @@ struct ata_device {
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	unsigned int		horkage;	/* List of broken features */
 	struct scsi_device	*sdev;		/* attached SCSI device */
+	struct device		tdev;
+
+	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
+	u64			n_sectors;	/* size of device, if ATA */
+	unsigned int		class;		/* ATA_DEV_xxx */
+
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
 	union acpi_object	*gtf_cache;
 #endif
-	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
-	u64			n_sectors;	/* size of device, if ATA */
-	unsigned int		class;		/* ATA_DEV_xxx */

 	u8			pio_mode;
 	u8			dma_mode;
@@ -641,6 +644,7 @@ struct ata_link {
 	struct ata_port		*ap;
 	int			pmp;		/* port multiplier port # */

+	struct device		tdev;
 	unsigned int		active_tag;	/* active tag on this link */
 	u32			sactive;	/* active NCQ commands */

@@ -657,6 +661,7 @@ struct ata_link {

 	struct ata_device	device[ATA_MAX_DEVICES];
 };
+#define ATA_LINK_CLEAR_OFFSET		offsetof(struct ata_link, active_tag)

 struct ata_port {
 	struct Scsi_Host	*scsi_host; /* our co-allocated scsi host */
@@ -695,6 +700,7 @@ struct ata_port {
 	struct ata_port_stats	stats;
 	struct ata_host		*host;
 	struct device 		*dev;
+	struct device		tdev;

 	void			*port_task_data;
 	struct delayed_work	port_task;
-- 
1.5.4.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19  4:31 [PATCH #upstream-fixes] libata: Add transport class for libata Gwendal Grignou
@ 2008-08-19 11:34 ` Stefan Richter
  2008-08-19 12:27   ` Matthew Wilcox
  2008-08-20  8:17 ` Tejun Heo
  1 sibling, 1 reply; 9+ messages in thread
From: Stefan Richter @ 2008-08-19 11:34 UTC (permalink / raw)
  To: Gwendal Grignou; +Cc: IDE/ATA development list, linux-scsi

Gwendal Grignou wrote:
> This patch adds objects for accessing libata objects from user space:
> - ata_port class: one per ATA port
> - ata_link class: one per ATA port or 15 for SATA Port Multiplier
> - ata_device class: up to 2 for PATA link, usually one for SATA.

Two outsider questions:

You are adding a userspace interface.  What are the stability commitments?

Could you make it configurable, so that people who don't need this
interface can build the kernel without it?
-- 
Stefan Richter
-=====-==--- =--- =--==
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19 11:34 ` Stefan Richter
@ 2008-08-19 12:27   ` Matthew Wilcox
  2008-08-19 13:04     ` Stefan Richter
  2008-08-19 13:16     ` Stefan Richter
  0 siblings, 2 replies; 9+ messages in thread
From: Matthew Wilcox @ 2008-08-19 12:27 UTC (permalink / raw)
  To: Stefan Richter; +Cc: Gwendal Grignou, IDE/ATA development list, linux-scsi

On Tue, Aug 19, 2008 at 01:34:04PM +0200, Stefan Richter wrote:
> Gwendal Grignou wrote:
> > This patch adds objects for accessing libata objects from user space:
> > - ata_port class: one per ATA port
> > - ata_link class: one per ATA port or 15 for SATA Port Multiplier
> > - ata_device class: up to 2 for PATA link, usually one for SATA.
> 
> Two outsider questions:
> 
> You are adding a userspace interface.  What are the stability commitments?

To channel GregKH, you need to document them in Documentation/ABI/

> Could you make it configurable, so that people who don't need this
> interface can build the kernel without it?

Please, no.  Don't we already have enough CONFIG options?

I think a more important question is ... this is coming from a Google
address, but has a Copyright Dell on it.  Where's the sign-off chain for
this?  Who's the author really?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19 12:27   ` Matthew Wilcox
@ 2008-08-19 13:04     ` Stefan Richter
  2008-08-19 13:16     ` Stefan Richter
  1 sibling, 0 replies; 9+ messages in thread
From: Stefan Richter @ 2008-08-19 13:04 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Gwendal Grignou, IDE/ATA development list, linux-scsi

Matthew Wilcox wrote:
> On Tue, Aug 19, 2008 at 01:34:04PM +0200, Stefan Richter wrote:
>> Could you make it configurable, so that people who don't need this
>> interface can build the kernel without it?
> 
> Please, no.  Don't we already have enough CONFIG options?

I agree that CONFIGurability should be minimal only as far as essential
functionality is concerned.  What is proposed here doesn't look
essential to me.  (Essential in order to support a broad range of
hardware or of usage scenarios, that is.)  Hence it should be off by
default, if it is added at all.
-- 
Stefan Richter
-=====-==--- =--- =--==
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19 12:27   ` Matthew Wilcox
  2008-08-19 13:04     ` Stefan Richter
@ 2008-08-19 13:16     ` Stefan Richter
  2008-08-19 13:41       ` Stefan Richter
  2008-08-19 18:14       ` Gwendal Grignou
  1 sibling, 2 replies; 9+ messages in thread
From: Stefan Richter @ 2008-08-19 13:16 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Gwendal Grignou, IDE/ATA development list, linux-scsi

Matthew Wilcox wrote:
> I think a more important question is ... this is coming from a Google
> address, but has a Copyright Dell on it.  Where's the sign-off chain for
> this?  Who's the author really?

Indeed, this needs to be clarified.

>> Gwendal Grignou wrote:
>>> +#define SETUP_TEMPLATE(attrb, field, perm, test)			\
>>> +	i->private_##attrb[count] = dev_attr_##field;		       	\
>>> +	i->private_##attrb[count].attr.mode = perm;			\
>>> +	i->attrb[count] = &i->private_##attrb[count];			\
>>> +	if (test)							\
>>> +		count++
...
>>> +#define SETUP_PORT_ATTRIBUTE(field)					\
>>> +	SETUP_TEMPLATE(port_attrs, field, S_IRUGO, 1)
...
>>> +	count = 0;
>>> +	SETUP_PORT_ATTRIBUTE(nr_pmp_links);
>>> +	SETUP_PORT_ATTRIBUTE(idle_irq);
>>> +	BUG_ON(count > ATA_PORT_ATTRS);
>>> +	i->port_attrs[count] = NULL;

I understand that such preprocessor games are hard to avoid in code like
sysfs attribute setup.  I have nothing better to suggest, but they are
ugly nevertheless, and may amount to bloat.

Anyway; the BUG_ON there should probably be a BUILD_BUG_ON.
-- 
Stefan Richter
-=====-==--- =--- =--==
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19 13:16     ` Stefan Richter
@ 2008-08-19 13:41       ` Stefan Richter
  2008-08-19 18:14       ` Gwendal Grignou
  1 sibling, 0 replies; 9+ messages in thread
From: Stefan Richter @ 2008-08-19 13:41 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Gwendal Grignou, IDE/ATA development list, linux-scsi

I wrote:
>>> Gwendal Grignou wrote:
>>>> +	count = 0;
>>>> +	SETUP_PORT_ATTRIBUTE(nr_pmp_links);
>>>> +	SETUP_PORT_ATTRIBUTE(idle_irq);
>>>> +	BUG_ON(count > ATA_PORT_ATTRS);
>>>> +	i->port_attrs[count] = NULL;
> 
> I understand that such preprocessor games are hard to avoid in code like
> sysfs attribute setup.  I have nothing better to suggest, but they are
> ugly nevertheless, and may amount to bloat.
> 
> Anyway; the BUG_ON there should probably be a BUILD_BUG_ON.

...but alas gcc isn't clever enough:  It won't report if ATA_PORT_ATTRS 
was too small.
-- 
Stefan Richter
-=====-==--- =--- =--==
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19 13:16     ` Stefan Richter
  2008-08-19 13:41       ` Stefan Richter
@ 2008-08-19 18:14       ` Gwendal Grignou
  1 sibling, 0 replies; 9+ messages in thread
From: Gwendal Grignou @ 2008-08-19 18:14 UTC (permalink / raw)
  To: Stefan Richter; +Cc: Matthew Wilcox, IDE/ATA development list, linux-scsi

Hi Stefan,

My mistake. I started from scsi_transport_sas.c code already present
in the kernel; I forgot to remove the copyright notice. I changed the
copyright notice.

About Documentation/ABI, I did not found any information about
existing transport classes [sas,fc,spi,iscsi,...]. I added on for
libata transport class. I send a new patch shortly.

Thanks,

Gwendal.

On Tue, Aug 19, 2008 at 6:16 AM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Matthew Wilcox wrote:
>> I think a more important question is ... this is coming from a Google
>> address, but has a Copyright Dell on it.  Where's the sign-off chain for
>> this?  Who's the author really?
>
> Indeed, this needs to be clarified.
>
>>> Gwendal Grignou wrote:
>>>> +#define SETUP_TEMPLATE(attrb, field, perm, test)                   \
>>>> +   i->private_##attrb[count] = dev_attr_##field;                   \
>>>> +   i->private_##attrb[count].attr.mode = perm;                     \
>>>> +   i->attrb[count] = &i->private_##attrb[count];                   \
>>>> +   if (test)                                                       \
>>>> +           count++
> ...
>>>> +#define SETUP_PORT_ATTRIBUTE(field)                                        \
>>>> +   SETUP_TEMPLATE(port_attrs, field, S_IRUGO, 1)
> ...
>>>> +   count = 0;
>>>> +   SETUP_PORT_ATTRIBUTE(nr_pmp_links);
>>>> +   SETUP_PORT_ATTRIBUTE(idle_irq);
>>>> +   BUG_ON(count > ATA_PORT_ATTRS);
>>>> +   i->port_attrs[count] = NULL;
>
> I understand that such preprocessor games are hard to avoid in code like
> sysfs attribute setup.  I have nothing better to suggest, but they are
> ugly nevertheless, and may amount to bloat.
>
> Anyway; the BUG_ON there should probably be a BUILD_BUG_ON.
> --
> Stefan Richter
> -=====-==--- =--- =--==
> http://arcgraph.de/sr/
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-19  4:31 [PATCH #upstream-fixes] libata: Add transport class for libata Gwendal Grignou
  2008-08-19 11:34 ` Stefan Richter
@ 2008-08-20  8:17 ` Tejun Heo
  2008-08-20 18:55   ` Gwendal Grignou
  1 sibling, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2008-08-20  8:17 UTC (permalink / raw)
  To: Gwendal Grignou; +Cc: IDE/ATA development list, linux-scsi

Gwendal Grignou wrote:
>>From df84b33306569f8247f331fd67b0a7e7dfb47936 Mon Sep 17 00:00:00 2001
> From: Gwendal Grignou <gwendal@google.com>
> Date: Mon, 18 Aug 2008 14:02:09 -0700
> Subject: Add transport class for libata.
> 
> This patch adds objects for accessing libata objects from user space:
> - ata_port class: one per ATA port
> - ata_link class: one per ATA port or 15 for SATA Port Multiplier
> - ata_device class: up to 2 for PATA link, usually one for SATA.
> 
> Each object exports information from the corresponding data strucure.
> For instance, ata_device exports id, the cached identify output if
> the device is a disk, gscr register output if it is a PM.
> 
> More fields can be added later, like error counters and ATA specific
> statistics.

Ah... I'm not so sure about the class transport approach although I do
agree that sysfs hierarchy for libata is way overdue.  Hmmm... Can you
please explain the planned hierarchy and how you're gonna match lifetime
rules?  Because libata uses very different object lifetime rules from
the generic driver layer, it can get ugly at times and from a quick
glance I don't really think the problem has been addressed but I could
have just missed it.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH #upstream-fixes] libata: Add transport class for libata
  2008-08-20  8:17 ` Tejun Heo
@ 2008-08-20 18:55   ` Gwendal Grignou
  0 siblings, 0 replies; 9+ messages in thread
From: Gwendal Grignou @ 2008-08-20 18:55 UTC (permalink / raw)
  To: Tejun Heo; +Cc: IDE/ATA development list, linux-scsi

Hi Tejun,

1- the hierarchy

In the case CONFIG_SYSFS_DEPRECATED is not set, here is the view of
ATA topology in sysfs: [I can describe when CONFIG_SYSFS_DEPRECATED is
set upon request]

The parent of the hierarchy is the device object given when the ata
host object is created. In the following example of a ATA controller
on a PCI bus, the current directory is the pci directory of the
controller - "/sys/devices/pci0000:00/0000:00:1f.2".
If there is no port multiplier are connected connected to a port, we would find:

ata1            <== 1 is the port_id of the port
ata1/ata_port
ata1/ata_port/ata1 <== directory where sysfs attributes are
ata1/link1
ata1/link1/ata_link
ata1/link1/ata_link/link1  <== directory where sysfs attributes are
ata1/link1/dev1.0
ata1/link1/dev1.0/ata_device
ata1/link1/dev1.0/ata_device/dev1.0  <== directory where sysfs attributes are
ata1/link1/dev1.1           <== If controller can support 2 devices
per link, a second device created even if there is not slave disk
attached
ata1/link1/dev1.1/ata_device
ata1/link1/dev1.1/ata_device/dev1.1

If there is a port multiplier, 15 links are created in addition:
ata5
ata5/ata_port
ata5/ata_port/ata5
ata5/link5            <====== host link
ata5/link5/ata_link
ata5/link5/ata_link/link5
ata5/link5/dev5.0   <==== device behind the host link [port multiplier]
ata5/link5/dev5.0/ata_device
ata5/link5/dev5.0/ata_device/dev5.0
ata5/link5.0  <=== first link behind the PM, port 0
ata5/link5.0/ata_link
ata5/link5.0/ata_link/link5.0
ata5/link5.0/dev5.0.0   <=== device object for sata device connected
in port 0 of the PM [if any]
ata5/link5.0/dev5.0.0/ata_device
ata5/link5.0/dev5.0.0/ata_device/dev5.0.0
ata5/link5.1
ata5/link5.1/ata_link
ata5/link5.1/ata_link/link5.1
ata5/link5.1/dev5.1.0
ata5/link5.1/dev5.1.0/ata_device
ata5/link5.1/dev5.1.0/ata_device/dev5.1.0
ata5/link5.2
ata5/link5.2/ata_link
ata5/link5.2/ata_link/link5.2
ata5/link5.2/dev5.2.0
ata5/link5.2/dev5.2.0/ata_device
ata5/link5.2/dev5.2.0/ata_device/dev5.2.0
...
ata5/link5.14
ata5/link5.14/ata_link
ata5/link5.14/ata_link/link5.14
ata5/link5.14/dev5.14.0 <=== device object for sata device connected
in port 14 of the PM [if any]
ata5/link5.14/dev5.14.0/ata_device
ata5/link5.14/dev5.14.0/ata_device/dev5.14.0


I describe the current information you can find in these object in
Documentation/ABI/testing/sysfs-ata in the latest version of the
patch.

2- sysfs object lifecycle

I try to sync sysfs object lifetime with libata object lifetime rules.
Given libata creates objects statically, I decided to create related
sysfs objects at almost the same time, even if they are not populated:

For port object, I have to wait for for print_id to be set. In the
latest version of the patch [v4], I create the sysfs port object
before calling ata_scsi_add_host().
The sysfs link object for the host link is created when the port
object is created. Related ata_link has been initialized before.

When a SATA PMP is found, all sysfs link objects  are created in
sata_pmp_init_links(), if the data structure to hold the links behind
the PMP did not exist already.

Given sysfs link objects are always created after ata_link_init() and
ata_dev_init() are called, sysfs ata_device object are implicitly
created when the links are created.

Given during error handling link and device ata objects are wiped out
instead of being freed, I alter memset() for link and devices to not
include fields holding sysfs information.

For cleanup:
All sysfs related objects are deleted in ata_port_detach, called when
.remove entry point of a ATA driver is called.

I was tempted to add objects only when they are useful [for instance,
creating only the required number of links object behind the PMP], but
I balked away; as you point out, the lifecycle is quite different from
scsi_device for instance, and I did not want to change the logic of
libata.

Also, I tried to create a link between scsi_device and ata_device
object when appropriate, but I was not successful: scsi_device sysfs
objects are created after slave_configure is called, so I did not find
an easy hook in libata to create the link.

Thanks,

Gwendal.


On Wed, Aug 20, 2008 at 1:17 AM, Tejun Heo <tj@kernel.org> wrote:
> Gwendal Grignou wrote:
>>>From df84b33306569f8247f331fd67b0a7e7dfb47936 Mon Sep 17 00:00:00 2001
>> From: Gwendal Grignou <gwendal@google.com>
>> Date: Mon, 18 Aug 2008 14:02:09 -0700
>> Subject: Add transport class for libata.
>>
>> This patch adds objects for accessing libata objects from user space:
>> - ata_port class: one per ATA port
>> - ata_link class: one per ATA port or 15 for SATA Port Multiplier
>> - ata_device class: up to 2 for PATA link, usually one for SATA.
>>
>> Each object exports information from the corresponding data strucure.
>> For instance, ata_device exports id, the cached identify output if
>> the device is a disk, gscr register output if it is a PM.
>>
>> More fields can be added later, like error counters and ATA specific
>> statistics.
>
> Ah... I'm not so sure about the class transport approach although I do
> agree that sysfs hierarchy for libata is way overdue.  Hmmm... Can you
> please explain the planned hierarchy and how you're gonna match lifetime
> rules?  Because libata uses very different object lifetime rules from
> the generic driver layer, it can get ugly at times and from a quick
> glance I don't really think the problem has been addressed but I could
> have just missed it.
>
> Thanks.
>
> --
> tejun
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-08-20 18:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-19  4:31 [PATCH #upstream-fixes] libata: Add transport class for libata Gwendal Grignou
2008-08-19 11:34 ` Stefan Richter
2008-08-19 12:27   ` Matthew Wilcox
2008-08-19 13:04     ` Stefan Richter
2008-08-19 13:16     ` Stefan Richter
2008-08-19 13:41       ` Stefan Richter
2008-08-19 18:14       ` Gwendal Grignou
2008-08-20  8:17 ` Tejun Heo
2008-08-20 18:55   ` Gwendal Grignou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).