Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk

[Tony Asleson <tasleson@redhat.com>]

Hi Bartlomiej,

On 6/24/20 5:35 AM, Bartlomiej Zolnierkiewicz wrote:
> The root source of problem is that libata transport uses different
> naming scheme for ->tdev devices (please see dev_set_name() in
> ata_t{dev,link,port}_add()) than libata core for its logging
> functionality (ata_{dev,link,port}_printk()).
> 
> Since libata transport is part of sysfs ABI we should be careful
> to not break it so one idea for solving the issue is to convert
> ata_t{dev,link,port}_add() to use libata logging naming scheme and
> at the same time add sysfs symlinks for the old libata transport
> naming scheme.

I tried doing as you suggested.  I've included what I've done so far.  I
haven't been able to get all the needed parts for the symlinks to
maintain compatibility.

The /sys/class/.. seems OK, eg.

$  ls -x -w 70 /sys/class/ata_[dl]*
/sys/class/ata_device:
ata1.00  ata2.00  ata3.00  ata4.00  ata5.00  ata6.00  ata7.00
ata7.01  ata8.00  ata8.01  dev1.0   dev2.0   dev3.0   dev4.0
dev5.0   dev6.0   dev7.0   dev7.1   dev8.0   dev8.1

/sys/class/ata_link:
ata1   ata2   ata3   ata4   ata5   ata6   ata7  ata8  link1  link2
link3  link4  link5  link6  link7  link8

but the implementation is a hack, see device.h, core.c changes.  There
must be a better way?

Also I'm missing part of the full path, eg.

/sys/devices/pci0000:00/0000:00:01.1/ata7/link7/dev7.0/ata_device/dev7.0/gscr

becomes

/sys/devices/pci0000:00/0000:00:01.1/ata7/ata7/ata7.01/ata_device/ata7.01/gscr

but the compatibility symlinks added only get me to

/sys/devices/pci0000:00/0000:00:01.1/ata7/link7/dev7.0/ata_device/

I haven't found the right spot to get the last symlink included.

If you or anyone else has suggestions to correct the incomplete symlink
and/or correct the implementation to set the
/sys/class/ata_device it would be greatly appreciated.

Thanks,
-Tony

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index beca5f91bb4c..2aa230ad813e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6444,7 +6444,7 @@ void ata_port_printk(const struct ata_port *ap,
const char *level,
        vaf.fmt = fmt;
        vaf.va = &args;

-       printk("%sata%u: %pV", level, ap->print_id, &vaf);
+       dev_printk(level, &ap->tdev, "%pV", &vaf);

        va_end(args);
 }
@@ -6461,12 +6461,7 @@ void ata_link_printk(const struct ata_link *link,
const char *level,
        vaf.fmt = fmt;
        vaf.va = &args;

-       if (sata_pmp_attached(link->ap) || link->ap->slave_link)
-               printk("%sata%u.%02u: %pV",
-                      level, link->ap->print_id, link->pmp, &vaf);
-       else
-               printk("%sata%u: %pV",
-                      level, link->ap->print_id, &vaf);
+       dev_printk(level, &link->tdev, "%pV", &vaf);

        va_end(args);
 }
@@ -6483,9 +6478,7 @@ void ata_dev_printk(const struct ata_device *dev,
const char *level,
        vaf.fmt = fmt;
        vaf.va = &args;

-       printk("%sata%u.%02u: %pV",
-              level, dev->link->ap->print_id, dev->link->pmp + dev->devno,
-              &vaf);
+       dev_printk(level, &dev->tdev,"%pV", &vaf);

        va_end(args);
 }
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 6a40e3c6cf49..e5870f9b2b21 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -288,7 +288,7 @@ 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_set_name(dev, "ata%u", ap->print_id);
        transport_setup_device(dev);
        ata_acpi_bind_port(ap);
        error = device_add(dev);
@@ -374,6 +374,46 @@ static int ata_tlink_match(struct
attribute_container *cont,
        return &i->link_attr_cont.ac == cont;
 }

+static struct device* tlink_to_symlink(struct device *dev) {
+       struct ata_internal* i =
to_ata_internal(ata_scsi_transport_template);
+       return
attribute_container_find_class_device(&i->link_attr_cont.ac, dev);
+}
+
+void ata_tlink_symlink_add_del(struct ata_link *link, int add) {
+       struct device *dev = &link->tdev;
+       struct ata_port *ap = link->ap;
+       char lname[64];
+       struct device *devp = tlink_to_symlink(dev);
+
+       if (ata_is_host_link(link)) {
+               snprintf(lname, sizeof(lname),
+                       "link%d", ap->print_id);
+       } else {
+               snprintf(lname, sizeof(lname),
+                       "link%d.%d", ap->print_id, link->pmp);
+       }
+
+       if (add) {
+               int e;
+
+               e = device_add_class_symlinks_additional(devp, lname);
+               if (e) {
+                        dev_warn(devp, "Error %d tlink symlink
class\n", e);
+               }
+
+               e = sysfs_create_link(&dev->parent->kobj, &dev->kobj,
lname);
+               if (e) {
+                        dev_warn(dev, "Error %d tlink symlink\n", e);
+               }
+       } else {
+               sysfs_delete_link(&dev->parent->kobj, &dev->kobj, lname);
+               device_delete_class_symlinks_additional(devp, lname);
+       }
+}
+
+#define ata_tlink_symlink_add(x) ata_tlink_symlink_add_del(x, 1)
+#define ata_tlink_symlink_del(x) ata_tlink_symlink_add_del(x, 0)
+
 /**
  * ata_tlink_delete  --  remove ATA LINK
  * @port:      ATA LINK to remove
@@ -389,6 +429,7 @@ void ata_tlink_delete(struct ata_link *link)
                ata_tdev_delete(ata_dev);
        }

+       ata_tlink_symlink_del(link);
        transport_remove_device(dev);
        device_del(dev);
        transport_destroy_device(dev);
@@ -415,9 +456,9 @@ int ata_tlink_add(struct ata_link *link)
        dev->parent = &ap->tdev;
        dev->release = ata_tlink_release;
        if (ata_is_host_link(link))
-               dev_set_name(dev, "link%d", ap->print_id);
-        else
-               dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp);
+               dev_set_name(dev, "ata%u", ap->print_id);
+       else
+               dev_set_name(dev, "ata%u.%02u", ap->print_id, link->pmp);

        transport_setup_device(dev);

@@ -429,6 +470,8 @@ int ata_tlink_add(struct ata_link *link)
        transport_add_device(dev);
        transport_configure_device(dev);

+       ata_tlink_symlink_add(link);
+
        ata_for_each_dev(ata_dev, link, ALL) {
                error = ata_tdev_add(ata_dev);
                if (error) {
@@ -440,6 +483,7 @@ int ata_tlink_add(struct ata_link *link)
        while (--ata_dev >= link->device) {
                ata_tdev_delete(ata_dev);
        }
+       ata_tlink_symlink_del(link);
        transport_remove_device(dev);
        device_del(dev);
   tlink_err:
@@ -630,6 +674,44 @@ static void ata_tdev_free(struct ata_device *dev)
        put_device(&dev->tdev);
 }

+static struct device* tdev_to_symlink(struct device *dev) {
+       struct ata_internal* i =
to_ata_internal(ata_scsi_transport_template);
+       return
attribute_container_find_class_device(&i->dev_attr_cont.ac, dev);
+}
+
+void ata_tdev_symlink_add_del(struct ata_device *ata_dev, int add) {
+       struct device *dev = &ata_dev->tdev;
+       struct ata_link *link = ata_dev->link;
+       struct ata_port *ap = link->ap;
+       char dname[64];
+
+       struct device *devp = tdev_to_symlink(dev);
+
+       if (ata_is_host_link(link))
+               snprintf(dname, sizeof(dname), "dev%d.%d",
ap->print_id,ata_dev->devno);
+       else
+               snprintf(dname, sizeof(dname), "dev%d.%d.0",
ap->print_id, link->pmp);
+
+       if (add) {
+               int e;
+               e = device_add_class_symlinks_additional(devp, dname);
+               if (e) {
+                        dev_warn(devp, "Error %d tdev symlink class\n", e);
+               }
+
+               e = sysfs_create_link(&dev->parent->kobj, &dev->kobj,
dname);
+               if (e) {
+                        dev_warn(dev, "Error %d tdev symlink\n", e);
+               }
+       } else {
+               sysfs_delete_link(&dev->parent->kobj, &dev->kobj, dname);
+               device_delete_class_symlinks_additional(devp, dname);
+       }
+}
+
+#define ata_tdev_symlink_add(x) ata_tdev_symlink_add_del(x, 1)
+#define ata_tdev_symlink_del(x) ata_tdev_symlink_add_del(x, 0)
+
 /**
  * ata_tdev_delete  --  remove ATA device
  * @port:      ATA PORT to remove
@@ -640,6 +722,7 @@ static void ata_tdev_delete(struct ata_device *ata_dev)
 {
        struct device *dev = &ata_dev->tdev;

+       ata_tdev_symlink_del(ata_dev);
        transport_remove_device(dev);
        device_del(dev);
        ata_tdev_free(ata_dev);
@@ -665,10 +748,8 @@ static int ata_tdev_add(struct ata_device *ata_dev)
        device_initialize(dev);
        dev->parent = &link->tdev;
        dev->release = ata_tdev_release;
-       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, "ata%u.%02u", ap->print_id, link->pmp +
ata_dev->devno);

        transport_setup_device(dev);
        ata_acpi_bind_dev(ata_dev);
@@ -680,6 +761,8 @@ static int ata_tdev_add(struct ata_device *ata_dev)

        transport_add_device(dev);
        transport_configure_device(dev);
+
+       ata_tdev_symlink_add(ata_dev);
        return 0;
 }

diff --git a/drivers/base/core.c b/drivers/base/core.c
index c2439d12608d..bc060000837c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2269,6 +2269,16 @@ static int device_add_class_symlinks(struct
device *dev)
        return error;
 }

+int device_add_class_symlinks_additional(struct device *dev, char *name) {
+       return sysfs_create_link(&dev->class->p->subsys.kobj,
&dev->kobj, name);
+}
+EXPORT_SYMBOL_GPL(device_add_class_symlinks_additional);
+
+void device_delete_class_symlinks_additional(struct device *dev, char
*name) {
+       sysfs_delete_link(&dev->class->p->subsys.kobj, &dev->kobj, name);
+}
+EXPORT_SYMBOL_GPL(device_delete_class_symlinks_additional);
+
 static void device_remove_class_symlinks(struct device *dev)
 {
        if (dev_of_node(dev))
diff --git a/include/linux/device.h b/include/linux/device.h
index 281755404c21..4827d86116ab 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -656,6 +656,10 @@ static inline const char *dev_name(const struct
device *dev)
 extern __printf(2, 3)
 int dev_set_name(struct device *dev, const char *name, ...);

+
+int device_add_class_symlinks_additional(struct device *dev, char *name);
+void device_delete_class_symlinks_additional(struct device *dev, char
*name);
+
 int dev_durable_name(const struct device *d, char *buffer, size_t len);

 #ifdef CONFIG_NUMA