[PATCHv4 0/2] libata: sysfs naming

[PATCHv4 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 compatibility 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 v3:
- Included reviews from Damien

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 | 62 ++++++++++++++++++++++++++++++++--
 include/linux/libata.h         | 53 ++++++++---------------------
 3 files changed, 84 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         | 53 ++++++++++------------------------
 2 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index ca129854a88c..e5ed5046b299 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_dev;
 	return 0;
 
+ out_unregister_dev:
+	transport_class_unregister(&ata_dev_class);
  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..7dc06e5cbc3a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1458,61 +1458,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 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

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

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

[RFC PATCH] libata: ata_bus_type can be static

[kernel test robot]

drivers/ata/libata-transport.c:88:17: warning: symbol 'ata_bus_type' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 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 e5ed5046b299e2..b9412d1fe3e083 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -85,7 +85,7 @@ struct ata_internal {
 static int ata_tdev_add(struct ata_device *dev);
 static void ata_tdev_delete(struct ata_device *dev);
 
-struct bus_type ata_bus_type = {
+static struct bus_type ata_bus_type = {
         .name		= "ata",
 };
 

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

[kernel test robot]

Hi Hannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.17 next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/libata-sysfs-naming/20220325-205722
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 34af78c4e616c359ed428d79fe4758a35d2c5473
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220327/202203270220.q1Q39xe4-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/ba4273174499b7351d34ffc22021d0c382cb1d63
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hannes-Reinecke/libata-sysfs-naming/20220325-205722
        git checkout ba4273174499b7351d34ffc22021d0c382cb1d63
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/ata/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/ata/libata-transport.c:88:17: sparse: sparse: symbol 'ata_bus_type' was not declared. Should it be static?

Please review and possibly fold the followup patch.

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp