[PATCHv2 0/3] libata: sysfs naming

[PATCHv2 0/3] 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 an additional
class object for the ata_port with the name of 'ata'.
This additional object can be disabled with the config option
ATA_SYSFS_COMPAT.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  libata: sysfs naming option
  libata: CONFIG_ATA_SYSFS_COMPAT
  libata: sanitize PMP link naming

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

-- 
2.29.2

[PATCH 1/3] libata: sysfs naming option

[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.
For consistency the 'ata_port' name has been changed from 'ata' to 'port';
to cover for sysfs incompabilities I've added a link from the original
'ata' device to the new 'port' device.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-transport.c | 38 ++++++++++++++++++++++--
 include/linux/libata.h         | 54 ++++++++++------------------------
 2 files changed, 51 insertions(+), 41 deletions(-)

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index ca129854a88c..5169a5db141d 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.
@@ -257,12 +261,15 @@ static int ata_tport_match(struct attribute_container *cont,
 void ata_tport_delete(struct ata_port *ap)
 {
 	struct device *dev = &ap->tdev;
+	struct device *cdev = &ap->cdev;
 
 	ata_tlink_delete(&ap->link);
 
+	device_del(cdev);
 	transport_remove_device(dev);
 	device_del(dev);
 	transport_destroy_device(dev);
+	put_device(cdev);
 	put_device(dev);
 }
 
@@ -281,6 +288,7 @@ int ata_tport_add(struct device *parent,
 {
 	int error;
 	struct device *dev = &ap->tdev;
+	struct device *cdev = &ap->cdev;
 
 	device_initialize(dev);
 	dev->type = &ata_port_type;
@@ -288,7 +296,14 @@ 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);
+
+	device_initialize(cdev);
+	cdev->parent = get_device(dev);
+	cdev->class = &ata_port_class.class;
+	dev_set_name(cdev, "ata%d", ap->print_id);
+
 	transport_setup_device(dev);
 	ata_acpi_bind_port(ap);
 	error = device_add(dev);
@@ -296,6 +311,11 @@ int ata_tport_add(struct device *parent,
 		goto tport_err;
 	}
 
+	error = device_add(cdev);
+	if (error) {
+		goto cdev_err;
+	}
+
 	device_enable_async_suspend(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -311,11 +331,14 @@ int ata_tport_add(struct device *parent,
 	return 0;
 
  tport_link_err:
+	device_del(cdev);
+ cdev_err:
 	transport_remove_device(dev);
 	device_del(dev);
 
  tport_err:
 	transport_destroy_device(dev);
+	put_device(cdev);
 	put_device(dev);
 	ata_host_put(ap->host);
 	return error;
@@ -444,6 +467,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 +720,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 +849,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 605756f645be..3460d7684464 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -829,6 +829,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;
@@ -1452,61 +1453,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/3] libata: CONFIG_ATA_SYSFS_COMPAT

[Hannes Reinecke]

Add a config option 'ATA_SYSFS_COMPAT' to allow the user to select
whether the compability 'ata_port' object with the name of 'ata'
should be registered with sysfs.

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

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index cb54631fd950..fe42a246d21d 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 5169a5db141d..efca41039d1e 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -261,15 +261,21 @@ 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 *cdev = &ap->cdev;
+#endif
 
 	ata_tlink_delete(&ap->link);
 
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	device_del(cdev);
+#endif
 	transport_remove_device(dev);
 	device_del(dev);
 	transport_destroy_device(dev);
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	put_device(cdev);
+#endif
 	put_device(dev);
 }
 
@@ -288,7 +294,9 @@ int ata_tport_add(struct device *parent,
 {
 	int error;
 	struct device *dev = &ap->tdev;
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	struct device *cdev = &ap->cdev;
+#endif
 
 	device_initialize(dev);
 	dev->type = &ata_port_type;
@@ -299,23 +307,24 @@ int ata_tport_add(struct device *parent,
 	dev->bus = &ata_bus_type;
 	dev_set_name(dev, "port%d", ap->print_id);
 
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	device_initialize(cdev);
 	cdev->parent = get_device(dev);
 	cdev->class = &ata_port_class.class;
 	dev_set_name(cdev, "ata%d", ap->print_id);
-
+#endif
 	transport_setup_device(dev);
 	ata_acpi_bind_port(ap);
 	error = device_add(dev);
 	if (error) {
 		goto tport_err;
 	}
-
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	error = device_add(cdev);
 	if (error) {
 		goto cdev_err;
 	}
-
+#endif
 	device_enable_async_suspend(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -331,14 +340,18 @@ int ata_tport_add(struct device *parent,
 	return 0;
 
  tport_link_err:
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	device_del(cdev);
  cdev_err:
+#endif
 	transport_remove_device(dev);
 	device_del(dev);
 
  tport_err:
 	transport_destroy_device(dev);
+#ifdef CONFIG_ATA_SYSFS_COMPAT
 	put_device(cdev);
+#endif
 	put_device(dev);
 	ata_host_put(ap->host);
 	return error;
-- 
2.29.2

[PATCH 3/3] libata: sanitize PMP link naming

[Hannes Reinecke]

Do not set the ata_link name to 'link<port>.<pmp>.0' as we only have
one device attached, rather use the simpler 'link<port>.<pmp>' to
avoid having the only three-level naming in libata.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index efca41039d1e..ee4aaf67234f 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -738,7 +738,7 @@ static int ata_tdev_add(struct ata_device *ata_dev)
 	if (ata_is_host_link(link))
 		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);
+		dev_set_name(dev, "dev%d.%d", ap->print_id, link->pmp);
 
 	transport_setup_device(dev);
 	ata_acpi_bind_dev(ata_dev);
-- 
2.29.2

Re: [PATCHv2 0/3] libata: sysfs naming

[Damien Le Moal]

On 3/21/22 23:56, Hannes Reinecke wrote:
> 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 an additional
> class object for the ata_port with the name of 'ata'.
> This additional object can be disabled with the config option
> ATA_SYSFS_COMPAT.
> 
> As usual, comments and reviews are welcome.
> 
> Hannes Reinecke (3):
>   libata: sysfs naming option
>   libata: CONFIG_ATA_SYSFS_COMPAT
>   libata: sanitize PMP link naming
> 
>  drivers/ata/Kconfig            | 10 +++++++
>  drivers/ata/libata-transport.c | 55 ++++++++++++++++++++++++++++++----
>  include/linux/libata.h         | 54 ++++++++++-----------------------
>  3 files changed, 76 insertions(+), 43 deletions(-)
> 

Kasan is not happy at all when I do "rmmod ahci"...

[ 1657.438508] BUG: KASAN: double-free or invalid-free in
attribute_container_release+0x37/0x50
[ 1657.447070]
[ 1657.448597] CPU: 8 PID: 1597 Comm: rmmod Not tainted 5.17.0-libata+ #25
[ 1657.455314] Hardware name: Supermicro Super Server/X11DPL-i, BIOS 3.3
02/21/2020
[ 1657.462809] Call Trace:
[ 1657.465304]  <TASK>
[ 1657.467447]  dump_stack_lvl+0x45/0x59
[ 1657.471180]  print_address_description.constprop.0+0x1f/0x120
[ 1657.477021]  ? attribute_container_release+0x37/0x50
[ 1657.482060]  ? attribute_container_release+0x37/0x50
[ 1657.487108]  kasan_report_invalid_free+0x51/0x80
[ 1657.491802]  __kasan_slab_free+0xf4/0x110
[ 1657.495885]  ? attribute_container_release+0x37/0x50
[ 1657.500930]  kfree+0xca/0x210
[ 1657.503956]  attribute_container_release+0x37/0x50
[ 1657.508826]  device_release+0x98/0x200
[ 1657.512642]  kobject_put+0x139/0x410
[ 1657.516283]  ata_tport_delete+0x4a/0x60 [libata]
[ 1657.521015]  ata_host_detach+0x336/0x660 [libata]
[ 1657.525820]  ? kernfs_remove_by_name_ns+0x9a/0xe0
[ 1657.530615]  pci_device_remove+0x65/0x110
[ 1657.534696]  __device_release_driver+0x316/0x680
[ 1657.539398]  driver_detach+0x1ec/0x2d0
[ 1657.543217]  bus_remove_driver+0xe7/0x2d0
[ 1657.547293]  ? lock_is_held_type+0x98/0x110
[ 1657.551546]  pci_unregister_driver+0x26/0x250
[ 1657.555982]  __x64_sys_delete_module+0x2fd/0x510
[ 1657.560673]  ? free_module+0xaa0/0xaa0
[ 1657.564487]  ? __cond_resched+0x1c/0x90
[ 1657.568392]  ? lockdep_hardirqs_on_prepare+0x273/0x3e0
[ 1657.573611]  ? syscall_enter_from_user_mode+0x21/0x70
[ 1657.578740]  ? trace_hardirqs_on+0x1c/0x110
[ 1657.583000]  do_syscall_64+0x35/0x80
[ 1657.586635]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 1657.591765] RIP: 0033:0x7f4eb289d84b
[ 1657.595403] Code: 73 01 c3 48 8b 0d dd 75 0e 00 f7 d8 64 89 01 48 83 c8
ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f 05
<48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ad 75 0e 00 f7 d8 64 89 01 48
[ 1657.618085] RSP: 002b:00007fffa28f9158 EFLAGS: 00000206 ORIG_RAX:
00000000000000b0
[ 1657.627664] RAX: ffffffffffffffda RBX: 000055e1a7f54760 RCX:
00007f4eb289d84b
[ 1657.636822] RDX: 000000000000000a RSI: 0000000000000800 RDI:
000055e1a7f547c8
[ 1657.645965] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000000
[ 1657.655065] R10: 00007f4eb292eac0 R11: 0000000000000206 R12:
00007fffa28f93d0
[ 1657.664115] R13: 00007fffa28fa780 R14: 000055e1a7f542a0 R15:
000055e1a7f54760
[ 1657.673133]  </TASK>
[ 1657.677130]
[ 1657.680403] Allocated by task 183:
[ 1657.685577]  kasan_save_stack+0x1e/0x40
[ 1657.691152]  __kasan_kmalloc+0x7f/0xa0
[ 1657.696603]  kmem_cache_alloc_trace+0x1f9/0x470
[ 1657.702809]  ata_port_alloc+0x40/0x5a0 [libata]
[ 1657.709031]  ata_host_alloc+0x1ca/0x260 [libata]
[ 1657.715301]  ata_host_alloc_pinfo+0x1d/0x540 [libata]
[ 1657.721978]  ahci_init_one+0xc5b/0x1d40 [ahci]
[ 1657.728037]  local_pci_probe+0xc6/0x150
[ 1657.733487]  work_for_cpu_fn+0x4e/0xa0
[ 1657.738833]  process_one_work+0x7f0/0x1310
[ 1657.744509]  worker_thread+0x6e0/0xf70
[ 1657.749855]  kthread+0x28f/0x330
[ 1657.754665]  ret_from_fork+0x1f/0x30
[ 1657.759818]
[ 1657.762850] The buggy address belongs to the object at ffff8885e2ce0000
[ 1657.762850]  which belongs to the cache kmalloc-32k of size 32768
[ 1657.778757] The buggy address is located 15088 bytes inside of
[ 1657.778757]  32768-byte region [ffff8885e2ce0000, ffff8885e2ce8000)
[ 1657.794105] The buggy address belongs to the page:
[ 1657.800540] page:00000000ddc44957 refcount:1 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x5e2ce0
[ 1657.811675] head:00000000ddc44957 order:4 compound_mapcount:0
compound_pincount:0
[ 1657.820907] flags: 0x20000000010200(slab|head|node=0|zone=2)
[ 1657.828316] raw: 0020000000010200 ffffea0018392808 ffffea00178b3c08
ffff888100040200
[ 1657.837857] raw: 0000000000000000 ffff8885e2ce0000 0000000100000001
0000000000000000
[ 1657.847408] page dumped because: kasan: bad access detected
[ 1657.854796]
[ 1657.858067] Memory state around the buggy address:
[ 1657.864684]  ffff8885e2ce3980: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[ 1657.873803]  ffff8885e2ce3a00: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[ 1657.882884] >ffff8885e2ce3a80: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[ 1657.891942]                                                              ^
[ 1657.900670]  ffff8885e2ce3b00: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[ 1657.909774]  ffff8885e2ce3b80: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00

And after that, doing "rmmod libata; modprobe libata", Things get really
bad...

[ 1820.226215] sysfs: cannot create duplicate filename '/bus/ata'

And then NULL pointer dereference oops.

My PMP setup also does not show the devices named devX.Y. I suspect this
is because my eSATA box supports FIS based switching. Even with that, the
eSATA box has 4 drives all connected behind one port/one link, but I still
see each device with its own port and link. Weird. Since I am not 100% up
to speed on how the PMP code works, I need to dig into it.



-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] libata: CONFIG_ATA_SYSFS_COMPAT

[Damien Le Moal]

On 3/21/22 23:56, Hannes Reinecke wrote:
> Add a config option 'ATA_SYSFS_COMPAT' to allow the user to select
> whether the compability 'ata_port' object with the name of 'ata'
> should be registered with sysfs.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/Kconfig            | 10 ++++++++++
>  drivers/ata/libata-transport.c | 19 ++++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index cb54631fd950..fe42a246d21d 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 5169a5db141d..efca41039d1e 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -261,15 +261,21 @@ 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 *cdev = &ap->cdev;
> +#endif
>  
>  	ata_tlink_delete(&ap->link);
>  
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	device_del(cdev);
> +#endif
>  	transport_remove_device(dev);
>  	device_del(dev);
>  	transport_destroy_device(dev);
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	put_device(cdev);
> +#endif
>  	put_device(dev);
>  }
>  
> @@ -288,7 +294,9 @@ int ata_tport_add(struct device *parent,
>  {
>  	int error;
>  	struct device *dev = &ap->tdev;
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	struct device *cdev = &ap->cdev;
> +#endif
>  
>  	device_initialize(dev);
>  	dev->type = &ata_port_type;
> @@ -299,23 +307,24 @@ int ata_tport_add(struct device *parent,
>  	dev->bus = &ata_bus_type;
>  	dev_set_name(dev, "port%d", ap->print_id);
>  
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	device_initialize(cdev);
>  	cdev->parent = get_device(dev);
>  	cdev->class = &ata_port_class.class;
>  	dev_set_name(cdev, "ata%d", ap->print_id);
> -
> +#endif
>  	transport_setup_device(dev);
>  	ata_acpi_bind_port(ap);
>  	error = device_add(dev);
>  	if (error) {
>  		goto tport_err;
>  	}
> -
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	error = device_add(cdev);
>  	if (error) {
>  		goto cdev_err;
>  	}
> -

Instead of adding a device, can't we simply create a symlink ?
E.g.:

	sprintf(symlink_name, "ata%d", ap->print_id);
	sysfs_create_link(&parent->kobj, &dev->kobj, symlink_name);

? This seems to work, but I have not checked all possible paths to see if
a symlink or directory name expected to be "ataX" ends up being "portX",
which could break userspace relying on the old name.

> +#endif
>  	device_enable_async_suspend(dev);
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
> @@ -331,14 +340,18 @@ int ata_tport_add(struct device *parent,
>  	return 0;
>  
>   tport_link_err:
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	device_del(cdev);
>   cdev_err:
> +#endif
>  	transport_remove_device(dev);
>  	device_del(dev);
>  
>   tport_err:
>  	transport_destroy_device(dev);
> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>  	put_device(cdev);
> +#endif
>  	put_device(dev);
>  	ata_host_put(ap->host);
>  	return error;


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] libata: CONFIG_ATA_SYSFS_COMPAT

[Hannes Reinecke]

On 3/24/22 10:27, Damien Le Moal wrote:
> On 3/21/22 23:56, Hannes Reinecke wrote:
>> Add a config option 'ATA_SYSFS_COMPAT' to allow the user to select
>> whether the compability 'ata_port' object with the name of 'ata'
>> should be registered with sysfs.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/ata/Kconfig            | 10 ++++++++++
>>   drivers/ata/libata-transport.c | 19 ++++++++++++++++---
>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index cb54631fd950..fe42a246d21d 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 5169a5db141d..efca41039d1e 100644
>> --- a/drivers/ata/libata-transport.c
>> +++ b/drivers/ata/libata-transport.c
>> @@ -261,15 +261,21 @@ 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 *cdev = &ap->cdev;
>> +#endif
>>   
>>   	ata_tlink_delete(&ap->link);
>>   
>> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>>   	device_del(cdev);
>> +#endif
>>   	transport_remove_device(dev);
>>   	device_del(dev);
>>   	transport_destroy_device(dev);
>> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>>   	put_device(cdev);
>> +#endif
>>   	put_device(dev);
>>   }
>>   
>> @@ -288,7 +294,9 @@ int ata_tport_add(struct device *parent,
>>   {
>>   	int error;
>>   	struct device *dev = &ap->tdev;
>> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>>   	struct device *cdev = &ap->cdev;
>> +#endif
>>   
>>   	device_initialize(dev);
>>   	dev->type = &ata_port_type;
>> @@ -299,23 +307,24 @@ int ata_tport_add(struct device *parent,
>>   	dev->bus = &ata_bus_type;
>>   	dev_set_name(dev, "port%d", ap->print_id);
>>   
>> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>>   	device_initialize(cdev);
>>   	cdev->parent = get_device(dev);
>>   	cdev->class = &ata_port_class.class;
>>   	dev_set_name(cdev, "ata%d", ap->print_id);
>> -
>> +#endif
>>   	transport_setup_device(dev);
>>   	ata_acpi_bind_port(ap);
>>   	error = device_add(dev);
>>   	if (error) {
>>   		goto tport_err;
>>   	}
>> -
>> +#ifdef CONFIG_ATA_SYSFS_COMPAT
>>   	error = device_add(cdev);
>>   	if (error) {
>>   		goto cdev_err;
>>   	}
>> -
> 
> Instead of adding a device, can't we simply create a symlink ?
> E.g.:
> 
> 	sprintf(symlink_name, "ata%d", ap->print_id);
> 	sysfs_create_link(&parent->kobj, &dev->kobj, symlink_name);
> 
> ? This seems to work, but I have not checked all possible paths to see if
> a symlink or directory name expected to be "ataX" ends up being "portX",
> which could break userspace relying on the old name.
> 
Yeah; tried a similar thing which turned out not to be working.
Let me test your patch.

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