[PATCHv3 0/2] libata: sysfs naming

[PATCHv3 0/2] libata: sysfs naming

[Hannes Reinecke]

Hi all,

here's my second attempt the align the libata object naming with sysfs.
Key point is to introduce an 'ata' bus, which serves to collect all
libata object (ata_port, ata_link, and ata_device).

To facilitate that the name of the 'ata_port' object changes from 'ata'
to 'port'. To provide backwards compability I've added config option
CONFIG_ATA_SYSFS_COMPAT to create a compat symlink in the PCI sysfs device
directory with the name of 'ata'.

As usual, comments and reviews are welcome.

Changes to v2:
- Dropped patch to change PMP naming
- created compability symlink instead of full sysfs objects

Hannes Reinecke (2):
  libata: rework sysfs naming
  libata: CONFIG_ATA_SYSFS_COMPAT

 drivers/ata/Kconfig            | 10 +++++++
 drivers/ata/libata-transport.c | 41 ++++++++++++++++++++++++--
 include/linux/libata.h         | 54 ++++++++++------------------------
 3 files changed, 64 insertions(+), 41 deletions(-)

-- 
2.29.2

[PATCH 1/2] libata: rework sysfs naming

[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 <hare@suse.de>
---
 drivers/ata/libata-transport.c | 21 +++++++++++--
 include/linux/libata.h         | 54 ++++++++++------------------------
 2 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index ca129854a88c..555fe6e2293d 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -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);
 
@@ -822,8 +831,13 @@ __init int libata_transport_init(void)
 	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;
 	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);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0619ae462ecd..b17683b00c90 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -835,6 +835,7 @@ struct ata_port {
 	struct ata_host		*host;
 	struct device 		*dev;
 	struct device		tdev;
+	struct device		cdev;
 
 	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);
 
-- 
2.29.2

[PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Hannes Reinecke]

Add a config option 'ATA_SYSFS_COMPAT' to create a compability
'ata' symlink in the PCI device sysfs directory.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/Kconfig            | 10 ++++++++++
 drivers/ata/libata-transport.c | 20 ++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e5641e6c52ee..f27b12ba2ce7 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -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
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 555fe6e2293d..a66c3480bdcf 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -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);
-- 
2.29.2

Re: [PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Damien Le Moal]

On 3/24/22 21:32, Hannes Reinecke wrote:
> Add a config option 'ATA_SYSFS_COMPAT' to create a compability

s/compability/compatibility

> '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:

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index a66c3480bdcf..fa249638bfb6 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -251,6 +251,40 @@ static int ata_tport_match(struct 
attribute_container *cont,
  	return &ata_scsi_transport_template->host_attrs.ac == cont;
  }

+#ifdef CONFIG_ATA_SYSFS_COMPAT
+
+static int ata_tport_compat_link_add(struct ata_port *ap)
+{
+	struct device *dev = &ap->tdev;
+	struct device *parent = dev->parent;
+	char compat_name[64];
+
+	sprintf(compat_name, "ata%d", ap->print_id);
+
+	return sysfs_create_link(&parent->kobj, &dev->kobj, compat_name);
+}
+
+static void ata_tport_compat_link_delete(struct ata_port *ap)
+{
+	struct device *dev = &ap->tdev;
+	struct device *parent = dev->parent;
+	char compat_name[64];
+
+	sprintf(compat_name, "ata%d", ap->print_id);
+	sysfs_remove_link(&parent->kobj, compat_name);
+}
+
+#else
+
+static inline int ata_tport_compat_link_add(struct ata_port *ap)
+{
+	return 0;
+}
+
+static inline void ata_tport_compat_link_delete(struct ata_port *ap) {}
+
+#endif
+
  /**
   * ata_tport_delete  --  remove ATA PORT
   * @ap:	ATA PORT to remove
@@ -260,13 +294,8 @@ 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_tport_compat_link_delete(ap);
  	ata_tlink_delete(&ap->link);

  	transport_remove_device(dev);
@@ -290,9 +319,6 @@ 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;
@@ -323,18 +349,14 @@ int ata_tport_add(struct device *parent,
  		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);
+	error = ata_tport_compat_link_add(ap);
  	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);



> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/ata/Kconfig            | 10 ++++++++++
>   drivers/ata/libata-transport.c | 20 ++++++++++++++++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e5641e6c52ee..f27b12ba2ce7 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -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
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index 555fe6e2293d..a66c3480bdcf 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -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);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] libata: rework sysfs naming

[Damien Le Moal]

On 3/24/22 21:32, Hannes Reinecke wrote:
> 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 <hare@suse.de>
> ---
>   drivers/ata/libata-transport.c | 21 +++++++++++--
>   include/linux/libata.h         | 54 ++++++++++------------------------
>   2 files changed, 34 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index ca129854a88c..555fe6e2293d 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -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);
>   
> @@ -822,8 +831,13 @@ __init int libata_transport_init(void)
>   	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)" ?

>   	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);
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 0619ae462ecd..b17683b00c90 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -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...

>   
>   	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);
>   


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] libata: rework sysfs naming

[Hannes Reinecke]

On 3/25/22 04:05, Damien Le Moal wrote:
> On 3/24/22 21:32, Hannes Reinecke wrote:
>> 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 <hare@suse.de>
>> ---
>>   drivers/ata/libata-transport.c | 21 +++++++++++--
>>   include/linux/libata.h         | 54 ++++++++++------------------------
>>   2 files changed, 34 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/ata/libata-transport.c 
>> b/drivers/ata/libata-transport.c
>> index ca129854a88c..555fe6e2293d 100644
>> --- a/drivers/ata/libata-transport.c
>> +++ b/drivers/ata/libata-transport.c
>> @@ -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);
>> @@ -822,8 +831,13 @@ __init int libata_transport_init(void)
>>       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)" ?
> 
Ah yes. You are right.

>>       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);
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 0619ae462ecd..b17683b00c90 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -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...
> 

Yeah, left-over from previous iteration.
I'll be resending.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Hannes Reinecke]

On 3/25/22 04:01, Damien Le Moal wrote:
> On 3/24/22 21:32, Hannes Reinecke wrote:
>> Add a config option 'ATA_SYSFS_COMPAT' to create a compability
> 
> s/compability/compatibility
> 
>> '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.

> 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
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Damien Le Moal]

On 2022/03/25 16:12, Hannes Reinecke wrote:
> On 3/25/22 04:01, Damien Le Moal wrote:
>> On 3/24/22 21:32, Hannes Reinecke wrote:
>>> Add a config option 'ATA_SYSFS_COMPAT' to create a compability
>>
>> s/compability/compatibility
>>
>>> '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.

OK. The default is "y" for now anyway, so it is safe.

> 
>> 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


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Hannes Reinecke]

On 3/25/22 17:22, Sergey Shtylyov wrote:
> Hello!
> 
> On 3/25/22 3:56 PM, Hannes Reinecke wrote:
> 
>> Add a config option 'ATA_SYSFS_COMPAT' to create a compatibility
>> 'ata' symlink in the PCI device sysfs directory.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> [...]
>> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
>> index e5ed5046b299..29dec89ccc2d 100644
>> --- a/drivers/ata/libata-transport.c
>> +++ b/drivers/ata/libata-transport.c
>> @@ -251,6 +251,39 @@ static int ata_tport_match(struct attribute_container *cont,
>>   	return &ata_scsi_transport_template->host_attrs.ac == cont;
>>   }
>>   
>> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>> +static int ata_tport_compat_link_add(struct ata_port *ap)
>> +{
>> +        struct device *dev = &ap->tdev;
> 
>     Indent with a tab, please.
> 

Of course.

>> +	struct device *parent = dev->parent;
>> +        char compat_name[64];
> 
>     Same here. The buffer seems oversized too...
> 
Hard to tell how many ATA devices there are in the system.
More than hundred?
But yeah, I can reduce it.

>> +
>> +	sprintf(compat_name, "ata%d", ap->print_id);
> 
>     snprintf(), perhaps?
> 
OK.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Sergey Shtylyov]

Hello!

On 3/25/22 3:56 PM, Hannes Reinecke wrote:

> Add a config option 'ATA_SYSFS_COMPAT' to create a compatibility
> 'ata' symlink in the PCI device sysfs directory.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
[...]
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index e5ed5046b299..29dec89ccc2d 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -251,6 +251,39 @@ static int ata_tport_match(struct attribute_container *cont,
>  	return &ata_scsi_transport_template->host_attrs.ac == cont;
>  }
>  
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
> +static int ata_tport_compat_link_add(struct ata_port *ap)
> +{
> +        struct device *dev = &ap->tdev;

   Indent with a tab, please.

> +	struct device *parent = dev->parent;
> +        char compat_name[64];

   Same here. The buffer seems oversized too...

> +
> +	sprintf(compat_name, "ata%d", ap->print_id);

   snprintf(), perhaps?

> +
> +	return sysfs_create_link(&parent->kobj, &dev->kobj, compat_name);
> +}
> +
> +static void ata_tport_compat_link_delete(struct ata_port *ap)
> +{
> +	struct device *dev = &ap->tdev;
> +	struct device *parent = dev->parent;
> +	char compat_name[64];
> +
> +	sprintf(compat_name, "ata%d", ap->print_id);

   snprintf()?

[...]

MBR, Sergey

[PATCH 2/2] libata: CONFIG_ATA_SYSFS_COMPAT

[Hannes Reinecke]

Add a config option 'ATA_SYSFS_COMPAT' to create a compatibility
'ata' symlink in the PCI device sysfs directory.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/Kconfig            | 10 +++++++++
 drivers/ata/libata-transport.c | 41 ++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e5641e6c52ee..f27b12ba2ce7 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -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
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index e5ed5046b299..29dec89ccc2d 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -251,6 +251,39 @@ static int ata_tport_match(struct attribute_container *cont,
 	return &ata_scsi_transport_template->host_attrs.ac == cont;
 }
 
+#ifdef CONFIG_ATA_SYSFS_COMPAT
+static int ata_tport_compat_link_add(struct ata_port *ap)
+{
+        struct device *dev = &ap->tdev;
+	struct device *parent = dev->parent;
+        char compat_name[64];
+
+	sprintf(compat_name, "ata%d", ap->print_id);
+
+	return sysfs_create_link(&parent->kobj, &dev->kobj, compat_name);
+}
+
+static void ata_tport_compat_link_delete(struct ata_port *ap)
+{
+	struct device *dev = &ap->tdev;
+	struct device *parent = dev->parent;
+	char compat_name[64];
+
+	sprintf(compat_name, "ata%d", ap->print_id);
+	sysfs_remove_link(&parent->kobj, compat_name);
+}
+
+#else
+
+static inline int ata_tport_compat_link_add(struct ata_port *ap)
+{
+	return 0;
+}
+
+static inline void ata_tport_compat_link_delete(struct ata_port *ap) {}
+
+#endif
+
 /**
  * ata_tport_delete  --  remove ATA PORT
  * @ap:	ATA PORT to remove
@@ -261,6 +294,7 @@ void ata_tport_delete(struct ata_port *ap)
 {
 	struct device *dev = &ap->tdev;
 
+	ata_tport_compat_link_delete(ap);
 	ata_tlink_delete(&ap->link);
 
 	transport_remove_device(dev);
@@ -313,8 +347,15 @@ int ata_tport_add(struct device *parent,
 	if (error) {
 		goto tport_link_err;
 	}
+
+	error = ata_tport_compat_link_add(ap);
+	if (error)
+		goto compat_link_err;
+
 	return 0;
 
+ compat_link_err:
+	ata_tlink_delete(&ap->link);
  tport_link_err:
 	transport_remove_device(dev);
 	device_del(dev);
-- 
2.29.2