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

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

Hi Tony,

On 7/9/20 11:18 PM, Tony Asleson wrote:
> 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.

Unfortunately I know too little about sysfs class-es handling to help
(I've added linux-{scsi,block} MLs & Greg to Cc:).

PS If libata support turns out to be blocking the patchset progress you
may consider concentrating on getting SCSI & NVME support merged first..

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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