[RFC PATCH] libata: sysfs naming option

[RFC PATCH] libata: sysfs naming option

[Hannes Reinecke]

This patch adds a config option ATA_SYSFS_NAMING to align the libata
device names in sysfs with those in the kernel message log.
It 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.
For consistency the 'ata_dev' name has been changed from 'ata' to 'dev';
as this induces a sysfs change the config option is disabled per default.

Patch is relative to the 'for-5.17-logging' branch from Damien.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/Kconfig            | 17 ++++++++++++++++
 drivers/ata/libata-transport.c | 24 +++++++++++++++++++++--
 include/linux/libata.h         | 36 ++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index f6e943c74001..9ebaa3c288dd 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -51,6 +51,23 @@ config ATA_VERBOSE_ERROR
 
 	  If unsure, say Y.
 
+config ATA_SYSFS_NAMING
+	bool "Align sysfs device names with kernel messages"
+	default n
+	help
+	  This option modifies the device naming to use a common
+	  format for all ATA objects (port, link, and devices) and use
+	  these names for both the sysfs directories and the kernel message
+	  log to align with the linux driver model.
+	  The names have the format ata<port>-<link>.<device>, such that an
+	  ATA port will have the name 'ata<port>', an ATA link the name
+	  'ata<port>-<link>', and an ATA device the name
+	  'ata<port>-<link>.<devno>'.
+	  This option changes the sysfs names, so userland tools might be
+	  impacted.
+
+	  If unsure, say N.
+
 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 ca129854a88c..b537eadc6501 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -81,10 +81,14 @@ 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 +292,12 @@ int ata_tport_add(struct device *parent,
 	dev->parent = parent;
 	ata_host_get(ap->host);
 	dev->release = ata_tport_release;
+#ifdef CONFIG_ATA_SYSFS_NAMING
+	dev->bus = &ata_bus_type;
+	dev_set_name(dev, "port%d", ap->print_id);
+#else
 	dev_set_name(dev, "ata%d", ap->print_id);
+#endif
 	transport_setup_device(dev);
 	ata_acpi_bind_port(ap);
 	error = device_add(dev);
@@ -444,6 +453,9 @@ int ata_tlink_add(struct ata_link *link)
 	device_initialize(dev);
 	dev->parent = &ap->tdev;
 	dev->release = ata_tlink_release;
+#ifdef CONFIG_ATA_SYSFS_NAMING
+	dev->bus = &ata_bus_type;
+#endif
 	if (ata_is_host_link(link))
 		dev_set_name(dev, "link%d", ap->print_id);
 	else
@@ -695,8 +707,11 @@ static int ata_tdev_add(struct ata_device *ata_dev)
 	device_initialize(dev);
 	dev->parent = &link->tdev;
 	dev->release = ata_tdev_release;
+#ifdef CONFIG_ATA_SYSFS_NAMING
+	dev->bus = &ata_bus_type;
+#endif
 	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 +837,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:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c258f69106f4..ab2d404cde08 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1440,6 +1440,41 @@ static inline int sata_srst_pmp(struct ata_link *link)
 	return link->pmp;
 }
 
+#ifdef CONFIG_ATA_SYSFS_NAMING
+#define ata_port_err(ap, fmt, ...)				\
+	dev_err(&ap->tdev, fmt, ##__VA_ARGS__)
+#define ata_port_warn(ap, fmt, ...)				\
+	dev_warn(&ap->tdev, fmt, ##__VA_ARGS__)
+#define ata_port_notice(ap, fmt, ...)				\
+	dev_notice(&ap->tdev, fmt, ##__VA_ARGS__)
+#define ata_port_info(ap, fmt, ...)				\
+	dev_info(&ap->tdev, fmt, ##__VA_ARGS__)
+#define ata_port_dbg(ap, fmt, ...)				\
+	dev_dbg(&ap->tdev, fmt, ##__VA_ARGS__)
+
+#define ata_link_err(link, fmt, ...)				\
+	dev_err(&link->tdev, fmt, ##__VA_ARGS__)
+#define ata_link_warn(link, fmt, ...)				\
+	dev_warn(&link->tdev, fmt, ##__VA_ARGS__)
+#define ata_link_notice(link, fmt, ...)				\
+	dev_notice(&link->tdev, fmt, ##__VA_ARGS__)
+#define ata_link_info(link, fmt, ...)				\
+	dev_info(&link->tdev, fmt, ##__VA_ARGS__)
+#define ata_link_dbg(link, fmt, ...)				\
+	dev_dbg(&link->tdev, fmt, ##__VA_ARGS__)
+
+#define ata_dev_err(dev, fmt, ...)				\
+	dev_err(&dev->tdev, fmt, ##__VA_ARGS__)
+#define ata_dev_warn(dev, fmt, ...)				\
+	dev_warn(&dev->tdev, fmt, ##__VA_ARGS__)
+#define ata_dev_notice(dev, fmt, ...)				\
+	dev_notice(&dev->tdev, fmt, ##__VA_ARGS__)
+#define ata_dev_info(dev, fmt, ...)				\
+	dev_info(&dev->tdev, fmt, ##__VA_ARGS__)
+#define ata_dev_dbg(dev, fmt, ...)				\
+	dev_dbg(&dev->tdev, fmt, ##__VA_ARGS__)
+
+#else
 #define ata_port_printk(level, ap, fmt, ...)			\
 	pr_ ## level ("ata%u: " fmt, (ap)->print_id, ##__VA_ARGS__)
 
@@ -1495,6 +1530,7 @@ do {								\
 	ata_dev_printk(info, dev, fmt, ##__VA_ARGS__)
 #define ata_dev_dbg(dev, fmt, ...)				\
 	ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__)
+#endif
 
 void ata_print_version(const struct device *dev, const char *version);
 
-- 
2.29.2

Re: [RFC PATCH] libata: sysfs naming option

[Damien Le Moal]

On 1/3/22 18:09, Hannes Reinecke wrote:
> This patch adds a config option ATA_SYSFS_NAMING to align the libata
> device names in sysfs with those in the kernel message log.
> It 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.
> For consistency the 'ata_dev' name has been changed from 'ata' to 'dev';
> as this induces a sysfs change the config option is disabled per default.
> 
> Patch is relative to the 'for-5.17-logging' branch from Damien.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/Kconfig            | 17 ++++++++++++++++
>  drivers/ata/libata-transport.c | 24 +++++++++++++++++++++--
>  include/linux/libata.h         | 36 ++++++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index f6e943c74001..9ebaa3c288dd 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -51,6 +51,23 @@ config ATA_VERBOSE_ERROR
>  
>  	  If unsure, say Y.
>  
> +config ATA_SYSFS_NAMING
> +	bool "Align sysfs device names with kernel messages"
> +	default n
> +	help
> +	  This option modifies the device naming to use a common
> +	  format for all ATA objects (port, link, and devices) and use
> +	  these names for both the sysfs directories and the kernel message
> +	  log to align with the linux driver model.
> +	  The names have the format ata<port>-<link>.<device>, such that an
> +	  ATA port will have the name 'ata<port>', an ATA link the name
> +	  'ata<port>-<link>', and an ATA device the name
> +	  'ata<port>-<link>.<devno>'.
> +	  This option changes the sysfs names, so userland tools might be
> +	  impacted.
> +
> +	  If unsure, say N.
> +
>  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 ca129854a88c..b537eadc6501 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -81,10 +81,14 @@ 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 +292,12 @@ int ata_tport_add(struct device *parent,
>  	dev->parent = parent;
>  	ata_host_get(ap->host);
>  	dev->release = ata_tport_release;
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +	dev->bus = &ata_bus_type;
> +	dev_set_name(dev, "port%d", ap->print_id);
> +#else
>  	dev_set_name(dev, "ata%d", ap->print_id);
> +#endif
>  	transport_setup_device(dev);
>  	ata_acpi_bind_port(ap);
>  	error = device_add(dev);
> @@ -444,6 +453,9 @@ int ata_tlink_add(struct ata_link *link)
>  	device_initialize(dev);
>  	dev->parent = &ap->tdev;
>  	dev->release = ata_tlink_release;
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +	dev->bus = &ata_bus_type;
> +#endif
>  	if (ata_is_host_link(link))
>  		dev_set_name(dev, "link%d", ap->print_id);
>  	else
> @@ -695,8 +707,11 @@ static int ata_tdev_add(struct ata_device *ata_dev)
>  	device_initialize(dev);
>  	dev->parent = &link->tdev;
>  	dev->release = ata_tdev_release;
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +	dev->bus = &ata_bus_type;
> +#endif
>  	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 +837,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:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index c258f69106f4..ab2d404cde08 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1440,6 +1440,41 @@ static inline int sata_srst_pmp(struct ata_link *link)
>  	return link->pmp;
>  }
>  
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +#define ata_port_err(ap, fmt, ...)				\
> +	dev_err(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_warn(ap, fmt, ...)				\
> +	dev_warn(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_notice(ap, fmt, ...)				\
> +	dev_notice(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_info(ap, fmt, ...)				\
> +	dev_info(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_dbg(ap, fmt, ...)				\
> +	dev_dbg(&ap->tdev, fmt, ##__VA_ARGS__)
> +
> +#define ata_link_err(link, fmt, ...)				\
> +	dev_err(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_warn(link, fmt, ...)				\
> +	dev_warn(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_notice(link, fmt, ...)				\
> +	dev_notice(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_info(link, fmt, ...)				\
> +	dev_info(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_dbg(link, fmt, ...)				\
> +	dev_dbg(&link->tdev, fmt, ##__VA_ARGS__)
> +
> +#define ata_dev_err(dev, fmt, ...)				\
> +	dev_err(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_warn(dev, fmt, ...)				\
> +	dev_warn(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_notice(dev, fmt, ...)				\
> +	dev_notice(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_info(dev, fmt, ...)				\
> +	dev_info(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_dbg(dev, fmt, ...)				\
> +	dev_dbg(&dev->tdev, fmt, ##__VA_ARGS__)
> +
> +#else
>  #define ata_port_printk(level, ap, fmt, ...)			\
>  	pr_ ## level ("ata%u: " fmt, (ap)->print_id, ##__VA_ARGS__)
>  
> @@ -1495,6 +1530,7 @@ do {								\
>  	ata_dev_printk(info, dev, fmt, ##__VA_ARGS__)
>  #define ata_dev_dbg(dev, fmt, ...)				\
>  	ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__)
> +#endif
>  
>  void ata_print_version(const struct device *dev, const char *version);
>  

Before the patch, in sysfs:

# tree /sys/class/ata_device/
/sys/class/ata_device/
...
|-- dev8.0 ->
../../devices/pci0000:00/0000:00:17.0/ata8/link8/dev8.0/ata_device/dev8.0
...

# tree /sys/class/ata_port/
/sys/class/ata_port/
...
|-- ata8 -> ../../devices/pci0000:00/0000:00:17.0/ata8/ata_port/ata8
...

# tree /sys/class/ata_link/
/sys/class/ata_link/
...
|-- link8 -> ../../devices/pci0000:00/0000:00:17.0/ata8/link8/ata_link/link8
...

And the dmesg:

[   20.713229] ata8: SATA max UDMA/133 abar m524288@0x9d200000 port
0x9d200380 irq 309
[   21.024579] ata8: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[   21.025763] ata8.00: ATA-9: WDC  WUH721414ALE604, LDGNW240, max UDMA/133
[   21.035186] ata8.00: 27344764928 sectors, multi 16: LBA48 NCQ (depth
32), AA
[   21.377184] ata8.00: Features: NCQ-sndrcv NCQ-prio
[   21.432131] ata8.00: configured for UDMA/133

After the patch, in sysfs:

# tree ata_device/
ata_device/
...
|-- dev8.0 ->
../../devices/pci0000:00/0000:00:17.0/port8/link8/dev8.0/ata_device/dev8.0
...

# tree ata_port/
ata_port/
...
|-- port8 -> ../../devices/pci0000:00/0000:00:17.0/port8/ata_port/port8
...

# tree ata_link/
ata_link/
...
|-- link8 ->
../../devices/pci0000:00/0000:00:17.0/port8/link8/ata_link/link8
...

And the dmesg:

[   20.973002] ata port8: SATA max UDMA/133 abar m524288@0x9d200000 port
0x9d200380 irq 308
[   21.355104] ata link8: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[   21.412804] ata dev8.0: ATA-9: WDC  WUH721414ALE604, LDGNW240, max
UDMA/133
[   22.646935] ata dev8.0: 27344764928 sectors, multi 16: LBA48 NCQ
(depth 32), AA
[   22.680872] ata dev8.0: Features: NCQ-sndrcv NCQ-prio
[   22.748586] ata dev8.0: configured for UDMA/133

So for the dmesg changes, this looks all good to me. As readable as
before, no real change. If anything, we now clearly see if the message
if from ata_dev_xxx(), ata_port_xxx() or ata_link_XXX() functions.

For sysfs changes, the real change is "ataX" becoming "portX" and the
addition of the new ata bus:

# tree /sys/bus/ata
...
|   |-- dev8.0 ->
../../../devices/pci0000:00/0000:00:17.0/port8/link8/dev8.0
...
|   |-- link8 -> ../../../devices/pci0000:00/0000:00:17.0/port8/link8
...
|   |-- port8 -> ../../../devices/pci0000:00/0000:00:17.0/port8
...

All good.

And I wonder if we could not add a symbolic link named "ataX" pointing
to "portX" (the new name) to avoid breaking userspace. That would allow
dropping the Kconfig option. Hmm ?

-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH] libata: sysfs naming option

[Damien Le Moal]

On 1/3/22 18:09, Hannes Reinecke wrote:
> This patch adds a config option ATA_SYSFS_NAMING to align the libata
> device names in sysfs with those in the kernel message log.
> It 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.
> For consistency the 'ata_dev' name has been changed from 'ata' to 'dev';
> as this induces a sysfs change the config option is disabled per default.
> 
> Patch is relative to the 'for-5.17-logging' branch from Damien.

FYI, I queued the logging series in for-5.17, minus this patch.
Everything is in for-next too to check that nothing is broken.

For this patch, as I commented, I think we can keep a backward
compatible naming using sysfs symlinks. But I am not entirely sure if
that can work with port-multiplier setups. Let's work on that for the
next cycle ?


> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/Kconfig            | 17 ++++++++++++++++
>  drivers/ata/libata-transport.c | 24 +++++++++++++++++++++--
>  include/linux/libata.h         | 36 ++++++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index f6e943c74001..9ebaa3c288dd 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -51,6 +51,23 @@ config ATA_VERBOSE_ERROR
>  
>  	  If unsure, say Y.
>  
> +config ATA_SYSFS_NAMING
> +	bool "Align sysfs device names with kernel messages"
> +	default n
> +	help
> +	  This option modifies the device naming to use a common
> +	  format for all ATA objects (port, link, and devices) and use
> +	  these names for both the sysfs directories and the kernel message
> +	  log to align with the linux driver model.
> +	  The names have the format ata<port>-<link>.<device>, such that an
> +	  ATA port will have the name 'ata<port>', an ATA link the name
> +	  'ata<port>-<link>', and an ATA device the name
> +	  'ata<port>-<link>.<devno>'.
> +	  This option changes the sysfs names, so userland tools might be
> +	  impacted.
> +
> +	  If unsure, say N.
> +
>  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 ca129854a88c..b537eadc6501 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -81,10 +81,14 @@ 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 +292,12 @@ int ata_tport_add(struct device *parent,
>  	dev->parent = parent;
>  	ata_host_get(ap->host);
>  	dev->release = ata_tport_release;
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +	dev->bus = &ata_bus_type;
> +	dev_set_name(dev, "port%d", ap->print_id);
> +#else
>  	dev_set_name(dev, "ata%d", ap->print_id);
> +#endif
>  	transport_setup_device(dev);
>  	ata_acpi_bind_port(ap);
>  	error = device_add(dev);
> @@ -444,6 +453,9 @@ int ata_tlink_add(struct ata_link *link)
>  	device_initialize(dev);
>  	dev->parent = &ap->tdev;
>  	dev->release = ata_tlink_release;
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +	dev->bus = &ata_bus_type;
> +#endif
>  	if (ata_is_host_link(link))
>  		dev_set_name(dev, "link%d", ap->print_id);
>  	else
> @@ -695,8 +707,11 @@ static int ata_tdev_add(struct ata_device *ata_dev)
>  	device_initialize(dev);
>  	dev->parent = &link->tdev;
>  	dev->release = ata_tdev_release;
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +	dev->bus = &ata_bus_type;
> +#endif
>  	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 +837,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:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index c258f69106f4..ab2d404cde08 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1440,6 +1440,41 @@ static inline int sata_srst_pmp(struct ata_link *link)
>  	return link->pmp;
>  }
>  
> +#ifdef CONFIG_ATA_SYSFS_NAMING
> +#define ata_port_err(ap, fmt, ...)				\
> +	dev_err(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_warn(ap, fmt, ...)				\
> +	dev_warn(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_notice(ap, fmt, ...)				\
> +	dev_notice(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_info(ap, fmt, ...)				\
> +	dev_info(&ap->tdev, fmt, ##__VA_ARGS__)
> +#define ata_port_dbg(ap, fmt, ...)				\
> +	dev_dbg(&ap->tdev, fmt, ##__VA_ARGS__)
> +
> +#define ata_link_err(link, fmt, ...)				\
> +	dev_err(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_warn(link, fmt, ...)				\
> +	dev_warn(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_notice(link, fmt, ...)				\
> +	dev_notice(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_info(link, fmt, ...)				\
> +	dev_info(&link->tdev, fmt, ##__VA_ARGS__)
> +#define ata_link_dbg(link, fmt, ...)				\
> +	dev_dbg(&link->tdev, fmt, ##__VA_ARGS__)
> +
> +#define ata_dev_err(dev, fmt, ...)				\
> +	dev_err(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_warn(dev, fmt, ...)				\
> +	dev_warn(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_notice(dev, fmt, ...)				\
> +	dev_notice(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_info(dev, fmt, ...)				\
> +	dev_info(&dev->tdev, fmt, ##__VA_ARGS__)
> +#define ata_dev_dbg(dev, fmt, ...)				\
> +	dev_dbg(&dev->tdev, fmt, ##__VA_ARGS__)
> +
> +#else
>  #define ata_port_printk(level, ap, fmt, ...)			\
>  	pr_ ## level ("ata%u: " fmt, (ap)->print_id, ##__VA_ARGS__)
>  
> @@ -1495,6 +1530,7 @@ do {								\
>  	ata_dev_printk(info, dev, fmt, ##__VA_ARGS__)
>  #define ata_dev_dbg(dev, fmt, ...)				\
>  	ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__)
> +#endif
>  
>  void ata_print_version(const struct device *dev, const char *version);
>  


-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH] libata: sysfs naming option

[Hannes Reinecke]

On 1/5/22 07:18, Damien Le Moal wrote:
> On 1/3/22 18:09, Hannes Reinecke wrote:
>> This patch adds a config option ATA_SYSFS_NAMING to align the libata
>> device names in sysfs with those in the kernel message log.
>> It 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.
>> For consistency the 'ata_dev' name has been changed from 'ata' to 'dev';
>> as this induces a sysfs change the config option is disabled per default.
>>
>> Patch is relative to the 'for-5.17-logging' branch from Damien.
> 
> FYI, I queued the logging series in for-5.17, minus this patch.
> Everything is in for-next too to check that nothing is broken.
> 
> For this patch, as I commented, I think we can keep a backward
> compatible naming using sysfs symlinks. But I am not entirely sure if
> that can work with port-multiplier setups. Let's work on that for the
> next cycle ?
> 
Well, I'm not terribly happy about the current port multiplier 
implementation, either.
Currently they are named 'ataX.Y.0', making them the only ata objects 
with three levels. Personally I would take the PATA master/slave thingie 
as an example, and just increase the last number (such that we would 
have ata1.0, ata1.1, ata1.2 etc).
Reasoning here is that PMP is pretty much an SATA thing, and 'slave' 
drives is a PATA thing. So they'll never clash.

As for the 'ataX.Y' link; that'll require even more sysfs trickery.
Let's see if I can come up with something.
So yeah, let's hold off the patch for now.

First I need to get a PMP cable to test things out :-)

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: [RFC PATCH] libata: sysfs naming option

[Damien Le Moal]

On 1/5/22 16:41, Hannes Reinecke wrote:
> On 1/5/22 07:18, Damien Le Moal wrote:
>> On 1/3/22 18:09, Hannes Reinecke wrote:
>>> This patch adds a config option ATA_SYSFS_NAMING to align the libata
>>> device names in sysfs with those in the kernel message log.
>>> It 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.
>>> For consistency the 'ata_dev' name has been changed from 'ata' to 'dev';
>>> as this induces a sysfs change the config option is disabled per default.
>>>
>>> Patch is relative to the 'for-5.17-logging' branch from Damien.
>>
>> FYI, I queued the logging series in for-5.17, minus this patch.
>> Everything is in for-next too to check that nothing is broken.
>>
>> For this patch, as I commented, I think we can keep a backward
>> compatible naming using sysfs symlinks. But I am not entirely sure if
>> that can work with port-multiplier setups. Let's work on that for the
>> next cycle ?
>>
> Well, I'm not terribly happy about the current port multiplier 
> implementation, either.
> Currently they are named 'ataX.Y.0', making them the only ata objects 
> with three levels. Personally I would take the PATA master/slave thingie 
> as an example, and just increase the last number (such that we would 
> have ata1.0, ata1.1, ata1.2 etc).
> Reasoning here is that PMP is pretty much an SATA thing, and 'slave' 
> drives is a PATA thing. So they'll never clash.

Sounds sane to me.

> 
> As for the 'ataX.Y' link; that'll require even more sysfs trickery.

Hmm, as long as we can create sysfs paths that are compatible with the
old naming scheme (which seem to differ only for the root port object
name), it may be as simple as calling sysfs_create_link() for the port
objects ?

> Let's see if I can come up with something.
> So yeah, let's hold off the patch for now.
> 
> First I need to get a PMP cable to test things out :-)

I do have some in the lab, so I can test. Just need to go to the lab,
for once :)

> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research