libata: sysfs naming

This patch adds a new dummy bus 'ata', which collects all ata device objects like ata_port, ata_link, and ata_dev, and adds an 'ata' prefix to the message log.
To be consistent with the other libata objects the 'ata_port' object name has been changed from 'ata' to 'port'.

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/ata/libata-transport.c | 21 +++++++++++--
  include/linux/libata.h         | 54 ++++++++++------------------------
  2 files changed, 34 insertions(+), 41 deletions(-)

drivers/ata/libata-transport.c +12 -3

index ca129854a88c..555fe6e2293d 100644
@@ -81,10 +81,13 @@ struct ata_internal {
  	tdev_to_port((dev)->parent)
 
 
-/* Device objects are always created whit link objects */
+/* Device objects are always created with link objects */
  static int ata_tdev_add(struct ata_device *dev);
  static void ata_tdev_delete(struct ata_device *dev);
 
+struct bus_type ata_bus_type = {
+        .name		= "ata",
+};
 
  /*
   * Hack to allow attributes of the same name in different objects.

@@ -288,7 +291,9 @@ int ata_tport_add(struct device *parent,
  	dev->parent = parent;
  	ata_host_get(ap->host);
  	dev->release = ata_tport_release;
-	dev_set_name(dev, "ata%d", ap->print_id);
+	dev->bus = &ata_bus_type;
+	dev_set_name(dev, "port%d", ap->print_id);
+
  	transport_setup_device(dev);
  	ata_acpi_bind_port(ap);
  	error = device_add(dev);

@@ -444,6 +449,8 @@ int ata_tlink_add(struct ata_link *link)
  	device_initialize(dev);
  	dev->parent = &ap->tdev;
  	dev->release = ata_tlink_release;
+	dev->bus = &ata_bus_type;
+
  	if (ata_is_host_link(link))
  		dev_set_name(dev, "link%d", ap->print_id);
  	else

@@ -695,8 +702,10 @@ static int ata_tdev_add(struct ata_device *ata_dev)
  	device_initialize(dev);
  	dev->parent = &link->tdev;
  	dev->release = ata_tdev_release;
+	dev->bus = &ata_bus_type;
+
  	if (ata_is_host_link(link))
-		dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
+		dev_set_name(dev, "dev%d.%d", ap->print_id, ata_dev->devno);
  	else
  		dev_set_name(dev, "dev%d.%d.0", ap->print_id, link->pmp);
 

drivers/ata/libata-transport.c +3 -0

  	return 0;
 
+ out_unregister_bus:
+	bus_unregister(&ata_bus_type);
   out_unregister_port:
  	transport_class_unregister(&ata_port_class);
   out_unregister_link:

@@ -835,6 +849,7 @@ __init int libata_transport_init(void)
 
  void __exit libata_transport_exit(void)
  {
+	bus_unregister(&ata_bus_type);
  	transport_class_unregister(&ata_link_class);
  	transport_class_unregister(&ata_port_class);
  	transport_class_unregister(&ata_dev_class);

include/linux/libata.h +0 -0

index 0619ae462ecd..b17683b00c90 100644

include/linux/libata.h +15 -38

  	struct mutex		scsi_scan_mutex;
  	struct delayed_work	hotplug_task;

@@ -1458,61 +1459,38 @@ static inline int sata_srst_pmp(struct ata_link *link)
  	return link->pmp;
  }
 
-#define ata_port_printk(level, ap, fmt, ...)			\
-	pr_ ## level ("ata%u: " fmt, (ap)->print_id, ##__VA_ARGS__)
-
  #define ata_port_err(ap, fmt, ...)				\
-	ata_port_printk(err, ap, fmt, ##__VA_ARGS__)
+	dev_err(&ap->tdev, fmt, ##__VA_ARGS__)
  #define ata_port_warn(ap, fmt, ...)				\
-	ata_port_printk(warn, ap, fmt, ##__VA_ARGS__)
+	dev_warn(&ap->tdev, fmt, ##__VA_ARGS__)
  #define ata_port_notice(ap, fmt, ...)				\
-	ata_port_printk(notice, ap, fmt, ##__VA_ARGS__)
+	dev_notice(&ap->tdev, fmt, ##__VA_ARGS__)
  #define ata_port_info(ap, fmt, ...)				\
-	ata_port_printk(info, ap, fmt, ##__VA_ARGS__)
+	dev_info(&ap->tdev, fmt, ##__VA_ARGS__)
  #define ata_port_dbg(ap, fmt, ...)				\
-	ata_port_printk(debug, ap, fmt, ##__VA_ARGS__)
-
-#define ata_link_printk(level, link, fmt, ...)			\
-do {								\
-	if (sata_pmp_attached((link)->ap) ||			\
-	    (link)->ap->slave_link)				\
-		pr_ ## level ("ata%u.%02u: " fmt,		\
-			      (link)->ap->print_id,		\
-			      (link)->pmp,			\
-			      ##__VA_ARGS__);			\
-        else							\
-		pr_ ## level ("ata%u: " fmt,			\
-			      (link)->ap->print_id,		\
-			      ##__VA_ARGS__);			\
-} while (0)
+	dev_dbg(&ap->tdev, fmt, ##__VA_ARGS__)
 
  #define ata_link_err(link, fmt, ...)				\
-	ata_link_printk(err, link, fmt, ##__VA_ARGS__)
+	dev_err(&link->tdev, fmt, ##__VA_ARGS__)
  #define ata_link_warn(link, fmt, ...)				\
-	ata_link_printk(warn, link, fmt, ##__VA_ARGS__)
+	dev_warn(&link->tdev, fmt, ##__VA_ARGS__)
  #define ata_link_notice(link, fmt, ...)				\
-	ata_link_printk(notice, link, fmt, ##__VA_ARGS__)
+	dev_notice(&link->tdev, fmt, ##__VA_ARGS__)
  #define ata_link_info(link, fmt, ...)				\
-	ata_link_printk(info, link, fmt, ##__VA_ARGS__)
+	dev_info(&link->tdev, fmt, ##__VA_ARGS__)
  #define ata_link_dbg(link, fmt, ...)				\
-	ata_link_printk(debug, link, fmt, ##__VA_ARGS__)
-
-#define ata_dev_printk(level, dev, fmt, ...)			\
-        pr_ ## level("ata%u.%02u: " fmt,			\
-               (dev)->link->ap->print_id,			\
-	       (dev)->link->pmp + (dev)->devno,			\
-	       ##__VA_ARGS__)
+	dev_dbg(&link->tdev, fmt, ##__VA_ARGS__)
 
  #define ata_dev_err(dev, fmt, ...)				\
-	ata_dev_printk(err, dev, fmt, ##__VA_ARGS__)
+	dev_err(&dev->tdev, fmt, ##__VA_ARGS__)
  #define ata_dev_warn(dev, fmt, ...)				\
-	ata_dev_printk(warn, dev, fmt, ##__VA_ARGS__)
+	dev_warn(&dev->tdev, fmt, ##__VA_ARGS__)
  #define ata_dev_notice(dev, fmt, ...)				\
-	ata_dev_printk(notice, dev, fmt, ##__VA_ARGS__)
+	dev_notice(&dev->tdev, fmt, ##__VA_ARGS__)
  #define ata_dev_info(dev, fmt, ...)				\
-	ata_dev_printk(info, dev, fmt, ##__VA_ARGS__)
+	dev_info(&dev->tdev, fmt, ##__VA_ARGS__)
  #define ata_dev_dbg(dev, fmt, ...)				\
-	ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__)
+	dev_dbg(&dev->tdev, fmt, ##__VA_ARGS__)
 
  void ata_print_version(const struct device *dev, const char *version);
 

Quoted 59 lines · Hannes Reinecke

This patch adds a new dummy bus 'ata', which collects all ata device objects like ata_port, ata_link, and ata_dev, and adds an 'ata' prefix to the message log.
To be consistent with the other libata objects the 'ata_port' object name has been changed from 'ata' to 'port'.

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/ata/libata-transport.c | 21 +++++++++++-- include/linux/libata.h         | 54 ++++++++++------------------------

  2 files changed, 34 insertions(+), 41 deletions(-)

drivers/ata/libata-transport.c +9 -2

index ca129854a88c..555fe6e2293d 100644
@@ -81,10 +81,13 @@ struct ata_internal {
       tdev_to_port((dev)->parent)
-/* Device objects are always created whit link objects */
+/* Device objects are always created with link objects */
   static int ata_tdev_add(struct ata_device *dev);
   static void ata_tdev_delete(struct ata_device *dev);
+struct bus_type ata_bus_type = {
+        .name        = "ata",
+};
   /*
    * Hack to allow attributes of the same name in different objects.

@@ -288,7 +291,9 @@ int ata_tport_add(struct device *parent,
       dev->parent = parent;
       ata_host_get(ap->host);
       dev->release = ata_tport_release;
-    dev_set_name(dev, "ata%d", ap->print_id);
+    dev->bus = &ata_bus_type;
+    dev_set_name(dev, "port%d", ap->print_id);
+
       transport_setup_device(dev);
       ata_acpi_bind_port(ap);
       error = device_add(dev);

@@ -444,6 +449,8 @@ int ata_tlink_add(struct ata_link *link)
       device_initialize(dev);
       dev->parent = &ap->tdev;
       dev->release = ata_tlink_release;
+    dev->bus = &ata_bus_type;
+
       if (ata_is_host_link(link))
           dev_set_name(dev, "link%d", ap->print_id);

      else

drivers/ata/libata-transport.c +3 -1

@@ -695,8 +702,10 @@ static int ata_tdev_add(struct ata_device *ata_dev)
       device_initialize(dev);
       dev->parent = &link->tdev;
       dev->release = ata_tdev_release;
+    dev->bus = &ata_bus_type;
+
       if (ata_is_host_link(link))
-        dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
+        dev_set_name(dev, "dev%d.%d", ap->print_id, ata_dev->devno);

      else
          dev_set_name(dev, "dev%d.%d.0", ap->print_id, link->pmp);

drivers/ata/libata-transport.c +0 -0

@@ -822,8 +831,13 @@ __init int libata_transport_init(void)

drivers/ata/libata-transport.c __init int libata_transport_init(void) · line 836

Quoted 6 lines

drivers/ata/libata-transport.c +3 -0

       error = transport_class_register(&ata_dev_class);
       if (error)
           goto out_unregister_port;
+    error = bus_register(&ata_bus_type);
+    if (error)
+        goto out_unregister_bus;

Why is it needed to call bus_unregister() if bus_register() fails ? Shouldn't this be a "goto out_unregister_dev" which does a "transport_class_unregister(&ata_dev_class)" ?

Quoted 16 lines

drivers/ata/libata-transport.c +2 -0

       return 0;
+ out_unregister_bus:
+    bus_unregister(&ata_bus_type);

   out_unregister_port:
      transport_class_unregister(&ata_port_class); out_unregister_link:

drivers/ata/libata-transport.c +1 -0

@@ -835,6 +849,7 @@ __init int libata_transport_init(void)
   void __exit libata_transport_exit(void)
   {
+    bus_unregister(&ata_bus_type);
       transport_class_unregister(&ata_link_class);
       transport_class_unregister(&ata_port_class);
       transport_class_unregister(&ata_dev_class);

include/linux/libata.h +0 -1

index 0619ae462ecd..b17683b00c90 100644
--- a/include/linux/libata.h

include/linux/libata.h struct ata_port { · line 838

Quoted 6 lines

+++ b/include/linux/libata.h

include/linux/libata.h +1 -0

@@ -835,6 +835,7 @@ struct ata_port {
       struct ata_host        *host;
       struct device         *dev;
       struct device        tdev;
+    struct device        cdev;

This one is not used...

Add a config option 'ATA_SYSFS_COMPAT' to create a compability

'ata' symlink in the PCI device sysfs directory.

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/ata/Kconfig            | 10 ++++++++++
  drivers/ata/libata-transport.c | 20 ++++++++++++++++++++
  2 files changed, 30 insertions(+)

drivers/ata/Kconfig +10 -0

index e5641e6c52ee..f27b12ba2ce7 100644
@@ -51,6 +51,16 @@ config ATA_VERBOSE_ERROR
 
  	  If unsure, say Y.
 
+config ATA_SYSFS_COMPAT
+	bool "Keep original sysfs layout"
+	default y
+	help
+	  This option retains the original sysfs layout by adding an
+	  additional ata_port object with the name of 'ataX' in
+	  to the ATA objects like 'ata_port', 'ata_link', and 'ata_device'.
+
+	  If unsure, say Y.
+
  config ATA_FORCE
  	bool "\"libata.force=\" kernel parameter support" if EXPERT
  	default y

drivers/ata/libata-transport.c +20 -0

index 555fe6e2293d..a66c3480bdcf 100644
@@ -260,7 +260,13 @@ static int ata_tport_match(struct attribute_container *cont,
  void ata_tport_delete(struct ata_port *ap)
  {
  	struct device *dev = &ap->tdev;
+#ifdef CONFIG_ATA_SYSFS_COMPAT
+	struct device *parent = dev->parent;
+	char compat_name[64];
 
+	sprintf(compat_name, "ata%d", ap->print_id);
+	sysfs_remove_link(&parent->kobj, compat_name);
+#endif
  	ata_tlink_delete(&ap->link);
 
  	transport_remove_device(dev);

@@ -284,6 +290,9 @@ int ata_tport_add(struct device *parent,
  {
  	int error;
  	struct device *dev = &ap->tdev;
+#ifdef CONFIG_ATA_SYSFS_COMPAT
+	char compat_name[64];
+#endif
 
  	device_initialize(dev);
  	dev->type = &ata_port_type;

@@ -313,8 +322,19 @@ int ata_tport_add(struct device *parent,
  	if (error) {
  		goto tport_link_err;
  	}
+
+#ifdef CONFIG_ATA_SYSFS_COMPAT
+	sprintf(compat_name, "ata%d", ap->print_id);
+	error = sysfs_create_link(&parent->kobj, &dev->kobj, compat_name);
+	if (error)
+		goto compat_link_err;
+#endif
  	return 0;
 
+#ifdef CONFIG_ATA_SYSFS_COMPAT
+ compat_link_err:
+	ata_tlink_delete(&ap->link);
+#endif
   tport_link_err:
  	transport_remove_device(dev);
  	device_del(dev);

Quoted 1 line · Hannes Reinecke

Add a config option 'ATA_SYSFS_COMPAT' to create a compability

s/compability/compatibility

Quoted 1 line

'ata' symlink in the PCI device sysfs directory.

I am not yet convinced if this new config option is really necessary... We could create the symlink unconditionally, no ?

In any case, I would like to at least reduce the number of #ifdef. So what about something like this on top of your patch:

Quoted 10 lines · Damien Le Moal

Quoted 1 line · Hannes Reinecke

Add a config option 'ATA_SYSFS_COMPAT' to create a compability

s/compability/compatibility

Quoted 1 line

'ata' symlink in the PCI device sysfs directory.

I am not yet convinced if this new config option is really necessary... We could create the symlink unconditionally, no ?

We could, but why?
The sole point of the compat symlink is to preserve compability with previous releases. But we don't really know if this compatility really is required; I haven't seen any difference in behaviour with or without the symlinks.
By having a config option we make it clear that the symlinks will eventually vanish.

Quoted 4 lines

In any case, I would like to at least reduce the number of #ifdef. So what about something like this on top of your patch:

Sure. Will be doing so in the next round.

Cheers,

Hannes