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
>
