| Message ID | 20200623191749.115200-6-tasleson@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add persistent durable identifier to storage log messages | expand |
[ added linux-ide ML to Cc: ] Hi, On 6/23/20 9:17 PM, Tony Asleson wrote: > Utilize the dev_printk function which will add structured data > to the log message. > > Signed-off-by: Tony Asleson <tasleson@redhat.com> > --- > drivers/ata/libata-core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index beca5f91bb4c..44c874367fe3 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -6475,6 +6475,7 @@ EXPORT_SYMBOL(ata_link_printk); > void ata_dev_printk(const struct ata_device *dev, const char *level, > const char *fmt, ...) > { > + const struct device *gendev; > struct va_format vaf; > va_list args; > > @@ -6483,9 +6484,12 @@ 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); > + gendev = (dev->sdev) ? &dev->sdev->sdev_gendev : &dev->tdev; > + > + dev_printk(level, gendev, "ata%u.%02u: %pV", > + dev->link->ap->print_id, This duplicates the device information and breaks integrity of libata logging functionality (ata_{dev,link,port}_printk() should be all converted to use dev_printk() at the same time). 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. dev->sdev usage is not required for dev_printk() conversion and should be considered as a separate change (since it also breaks integrity of libata logging and can be considered as a mild "layering violation" I don't think that it should be applied). > + dev->link->pmp + dev->devno, > + &vaf); > > va_end(args); > } > Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 6/24/20 5:35 AM, Bartlomiej Zolnierkiewicz wrote: > > [ added linux-ide ML to Cc: ] > > Hi, Hello, > > On 6/23/20 9:17 PM, Tony Asleson wrote: >> Utilize the dev_printk function which will add structured data >> to the log message. >> >> Signed-off-by: Tony Asleson <tasleson@redhat.com> >> --- >> drivers/ata/libata-core.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index beca5f91bb4c..44c874367fe3 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -6475,6 +6475,7 @@ EXPORT_SYMBOL(ata_link_printk); >> void ata_dev_printk(const struct ata_device *dev, const char *level, >> const char *fmt, ...) >> { >> + const struct device *gendev; >> struct va_format vaf; >> va_list args; >> >> @@ -6483,9 +6484,12 @@ 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); >> + gendev = (dev->sdev) ? &dev->sdev->sdev_gendev : &dev->tdev; >> + >> + dev_printk(level, gendev, "ata%u.%02u: %pV", >> + dev->link->ap->print_id, > > This duplicates the device information and breaks integrity of > libata logging functionality (ata_{dev,link,port}_printk() should > be all converted to use dev_printk() at the same time). > > 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. > > dev->sdev usage is not required for dev_printk() conversion and > should be considered as a separate change (since it also breaks > integrity of libata logging and can be considered as a mild > "layering violation" I don't think that it should be applied). The point of this patch series is to attach a device unique identifier to the storage device log messages as structured data. Originally I resurrected and used printk_emit, but it was suggested I leverage dev_printk. dev_printk does change the output of the log message to include duplicate information if the message isn't changed. You are not the first person to raise that concern. I listed this as an open question in the cover letter. I've wanted to preserve the original log message, so as to not break user space mining tools and I've been concerned that dev_printk prefixing with an id may already do that. Adding structured data is invisible to them, or at the least shouldn't break them, eg. adding a new key-value pair. I can understand the desire to make all the ata logging functions consistent, and use dev_printk if we go this way. However, for this change it wasn't really the goal to refactor all the logging everywhere to use dev_printk, although that may be a good change in general. This is especially true that if at the end of the refactor to use dev_printk we consider it a layering violation to call into the existing functionality to return the vpd ID for the device and reject that part of the change. What I am hoping is that we can all agree that having a persistent identifier associated to storage related log messages is indeed useful. If we can agree on that, then I would like to have the discussion on what's the best way to achieve that. Thanks, Tony
On 6/24/20 5:15 PM, Tony Asleson wrote: > On 6/24/20 5:35 AM, Bartlomiej Zolnierkiewicz wrote: >> >> [ added linux-ide ML to Cc: ] >> >> Hi, > > Hello, > >> >> On 6/23/20 9:17 PM, Tony Asleson wrote: >>> Utilize the dev_printk function which will add structured data >>> to the log message. >>> >>> Signed-off-by: Tony Asleson <tasleson@redhat.com> >>> --- >>> drivers/ata/libata-core.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index beca5f91bb4c..44c874367fe3 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c >>> @@ -6475,6 +6475,7 @@ EXPORT_SYMBOL(ata_link_printk); >>> void ata_dev_printk(const struct ata_device *dev, const char *level, >>> const char *fmt, ...) >>> { >>> + const struct device *gendev; >>> struct va_format vaf; >>> va_list args; >>> >>> @@ -6483,9 +6484,12 @@ 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); >>> + gendev = (dev->sdev) ? &dev->sdev->sdev_gendev : &dev->tdev; >>> + >>> + dev_printk(level, gendev, "ata%u.%02u: %pV", >>> + dev->link->ap->print_id, >> >> This duplicates the device information and breaks integrity of >> libata logging functionality (ata_{dev,link,port}_printk() should >> be all converted to use dev_printk() at the same time). BTW: Similar dev_printk() conversion for ata_dev_printk() has been tried recently as a part of changes to add dynamic debugging support to libata (as it also requires dev_printk() to be used) and had to be dropped: https://lore.kernel.org/linux-ide/alpine.DEB.2.21.2003241414490.21582@ramsan.of.borg/ Thus I worry that the proposed ata_dev_printk() conversion is not only causing duplicated information to be printed but it may also confuse users. >> 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. >> >> dev->sdev usage is not required for dev_printk() conversion and >> should be considered as a separate change (since it also breaks >> integrity of libata logging and can be considered as a mild >> "layering violation" I don't think that it should be applied). > > The point of this patch series is to attach a device unique identifier > to the storage device log messages as structured data. Originally I > resurrected and used printk_emit, but it was suggested I leverage > dev_printk. dev_printk does change the output of the log message to > include duplicate information if the message isn't changed. You are not > the first person to raise that concern. I listed this as an open > question in the cover letter. I've wanted to preserve the original log > message, so as to not break user space mining tools and I've been > concerned that dev_printk prefixing with an id may already do that. > Adding structured data is invisible to them, or at the least shouldn't > break them, eg. adding a new key-value pair. Please note that with libata transport naming scheme fixed we can use dev_printk() in libata with unchanged log messages. > I can understand the desire to make all the ata logging functions > consistent, and use dev_printk if we go this way. However, for this > change it wasn't really the goal to refactor all the logging everywhere > to use dev_printk, although that may be a good change in general. This > is especially true that if at the end of the refactor to use dev_printk > we consider it a layering violation to call into the existing > functionality to return the vpd ID for the device and reject that part > of the change. Well, I'm against changing the libata log messages but durable name functionality still should be achievable in libata. How's about: * adding: ata_dev->tdev.durable_name = ata_scsi_durable_name; near the end of ata_scsi_slave_config() and * implementing ata_scsi_durable_name() which does struct ata_device *ata_dev = container_of(dev, struct ata_device, tdev); return scsi_durable_name(ata_dev->sdev, buf, len); ? > What I am hoping is that we can all agree that having a persistent > identifier associated to storage related log messages is indeed useful. > If we can agree on that, then I would like to have the discussion on > what's the best way to achieve that. Of course I agree that having a persistent identifier associated to storage related log messages is useful and my previous mail was exactly a part of discussion on the best way to achieving it. :-) I agree with James that dev_printk() usage is preferred over legacy printk_emit() and I've described a way to do it correctly for libata. Unfortunately it means additional work for getting the new feature merged so if you don't agree with doing it you need to convince: - Jens (libata Maintainer) to accept libata patch as it is or - James (& other higher level Maintainers) to use printk_emit() instead Ultimately they will be the ones merging/long-term supporting proposed patches and not me.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics > > Thanks, > Tony
On 6/26/20 7:45 AM, Bartlomiej Zolnierkiewicz wrote: > Of course I agree that having a persistent identifier associated to > storage related log messages is useful and my previous mail was exactly > a part of discussion on the best way to achieving it. :-) > > I agree with James that dev_printk() usage is preferred over legacy > printk_emit() and I've described a way to do it correctly for libata. > > Unfortunately it means additional work for getting the new feature > merged so if you don't agree with doing it you need to convince: > > - Jens (libata Maintainer) to accept libata patch as it is > > or > > - James (& other higher level Maintainers) to use printk_emit() instead > > Ultimately they will be the ones merging/long-term supporting proposed > patches and not me.. Thank you for the helpful response, I appreciate it. I'll take a look at the information you've provided and re-work the patch series. I may have additional question(s) :-) -Tony
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 >
On Tue, Jul 14, 2020 at 10:06:05AM +0200, Bartlomiej Zolnierkiewicz wrote: > > 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. Given the age of the current implementation, what suddenly broke that requires this to change at this point in time? > > > > 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 A link class? Ick ick ick. > > 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. I can't understand what you are trying to do here. What do you want to represent in sysfs with a symlink that you can't just have in a single sysfs file like "name" or "new_name" or "name_because_we_didnt_think_about_this_10_years_ago" that shows you the other "name" that you are trying to look up here? Why abuse symlinks like this at all? And no, the device.h and core.c changes aren't ok :) thanks, greg k-h
On 7/14/20 10:17 AM, Greg Kroah-Hartman wrote: > On Tue, Jul 14, 2020 at 10:06:05AM +0200, Bartlomiej Zolnierkiewicz wrote: >> >> 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. > > Given the age of the current implementation, what suddenly broke that > requires this to change at this point in time? Unfortunately when adding libata transport classes (+ at the same time embedding struct device-s in libata dev/link/port objects) in the past someone has decided to use different naming scheme than the one used for standard libata log messages (which use printk() without any reference to struct device-s in libata dev/link/port objects). Now we would like to use dev_printk() for standard libata logging functionality as this is required for 2 pending patchsets: - move DPRINTK to dynamic debugging (from Hannes Reinecke) - add persistent durable identifier storage log messages (from Tony) but we don't want to change standard libata log messages and confuse users.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Tue, Jul 14, 2020 at 10:50:39AM +0200, Bartlomiej Zolnierkiewicz wrote: > > On 7/14/20 10:17 AM, Greg Kroah-Hartman wrote: > > On Tue, Jul 14, 2020 at 10:06:05AM +0200, Bartlomiej Zolnierkiewicz wrote: > >> > >> 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. > > > > Given the age of the current implementation, what suddenly broke that > > requires this to change at this point in time? > > Unfortunately when adding libata transport classes (+ at the same > time embedding struct device-s in libata dev/link/port objects) in > the past someone has decided to use different naming scheme than > the one used for standard libata log messages (which use printk() > without any reference to struct device-s in libata dev/link/port > objects). > > Now we would like to use dev_printk() for standard libata logging > functionality as this is required for 2 pending patchsets: > > - move DPRINTK to dynamic debugging (from Hannes Reinecke) > > - add persistent durable identifier storage log messages (from Tony) > > but we don't want to change standard libata log messages and > confuse users.. All of that mess with symlinks just for a common debug printk? That seems excessive :) Just use the device name and don't worry about it, I doubt anyone will notice, unless the name is _really_ different. thanks, greg k-h
On 7/17/20 12:06 PM, Greg Kroah-Hartman wrote: > On Tue, Jul 14, 2020 at 10:50:39AM +0200, Bartlomiej Zolnierkiewicz wrote: >> >> On 7/14/20 10:17 AM, Greg Kroah-Hartman wrote: >>> On Tue, Jul 14, 2020 at 10:06:05AM +0200, Bartlomiej Zolnierkiewicz wrote: >>>> >>>> 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. >>> >>> Given the age of the current implementation, what suddenly broke that >>> requires this to change at this point in time? >> >> Unfortunately when adding libata transport classes (+ at the same >> time embedding struct device-s in libata dev/link/port objects) in >> the past someone has decided to use different naming scheme than >> the one used for standard libata log messages (which use printk() >> without any reference to struct device-s in libata dev/link/port >> objects). >> >> Now we would like to use dev_printk() for standard libata logging >> functionality as this is required for 2 pending patchsets: >> >> - move DPRINTK to dynamic debugging (from Hannes Reinecke) >> >> - add persistent durable identifier storage log messages (from Tony) >> >> but we don't want to change standard libata log messages and >> confuse users.. > > All of that mess with symlinks just for a common debug printk? That > seems excessive :) > > Just use the device name and don't worry about it, I doubt anyone will > notice, unless the name is _really_ different. > Good luck. I tried (cf patchset 'ata: kill ATA_DEBUG') but got rejected for exactly this reason. Cheers, Hannes
On 7/17/20 12:06 PM, Greg Kroah-Hartman wrote: > On Tue, Jul 14, 2020 at 10:50:39AM +0200, Bartlomiej Zolnierkiewicz wrote: >> >> On 7/14/20 10:17 AM, Greg Kroah-Hartman wrote: >>> On Tue, Jul 14, 2020 at 10:06:05AM +0200, Bartlomiej Zolnierkiewicz wrote: >>>> >>>> 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. >>> >>> Given the age of the current implementation, what suddenly broke that >>> requires this to change at this point in time? >> >> Unfortunately when adding libata transport classes (+ at the same >> time embedding struct device-s in libata dev/link/port objects) in >> the past someone has decided to use different naming scheme than >> the one used for standard libata log messages (which use printk() >> without any reference to struct device-s in libata dev/link/port >> objects). >> >> Now we would like to use dev_printk() for standard libata logging >> functionality as this is required for 2 pending patchsets: >> >> - move DPRINTK to dynamic debugging (from Hannes Reinecke) >> >> - add persistent durable identifier storage log messages (from Tony) >> >> but we don't want to change standard libata log messages and >> confuse users.. > > All of that mess with symlinks just for a common debug printk? That > seems excessive :) Unfortunately "a common debug printk" means all log messages generated by libata core.. > Just use the device name and don't worry about it, I doubt anyone will > notice, unless the name is _really_ different. Well, Geert has noticed and complained pretty quickly: https://lore.kernel.org/linux-ide/alpine.DEB.2.21.2003241414490.21582@ramsan.of.borg/ Anyway, I don't insist that hard on keeping the old names and I won't be the one handling potential bug-reports.. (added Jens to Cc:). Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 7/17/20 5:27 AM, Bartlomiej Zolnierkiewicz wrote: > > On 7/17/20 12:06 PM, Greg Kroah-Hartman wrote: > >> Just use the device name and don't worry about it, I doubt anyone will >> notice, unless the name is _really_ different. > > Well, Geert has noticed and complained pretty quickly: > > https://lore.kernel.org/linux-ide/alpine.DEB.2.21.2003241414490.21582@ramsan.of.borg/ > > Anyway, I don't insist that hard on keeping the old names and > I won't be the one handling potential bug-reports.. (added Jens to Cc:). I would think having sysfs use one naming convention and the logging using another would be confusing for users, but apparently they've managed this long with that. It appears changes are being rejected because of logging content differences, implying we shouldn't be changing printk usage to dev_printk. Should I re-work my changes to support dev_printk path in addition to utilizing printk_emit functionality so that we can avoid user space visible log changes? I don't see a way to make the transition from printk to dev_printk without having user visible changes to message content. Thanks -Tony
On 7/17/20 9:47 PM, Tony Asleson wrote: > On 7/17/20 5:27 AM, Bartlomiej Zolnierkiewicz wrote: >> >> On 7/17/20 12:06 PM, Greg Kroah-Hartman wrote: >> >>> Just use the device name and don't worry about it, I doubt anyone will >>> notice, unless the name is _really_ different. >> >> Well, Geert has noticed and complained pretty quickly: >> >> https://lore.kernel.org/linux-ide/alpine.DEB.2.21.2003241414490.21582@ramsan.of.borg/ >> >> Anyway, I don't insist that hard on keeping the old names and >> I won't be the one handling potential bug-reports.. (added Jens to Cc:). > > I would think having sysfs use one naming convention and the logging > using another would be confusing for users, but apparently they've > managed this long with that. > > > It appears changes are being rejected because of logging content > differences, implying we shouldn't be changing printk usage to dev_printk. > > Should I re-work my changes to support dev_printk path in addition to > utilizing printk_emit functionality so that we can avoid user space Unfortunately this won't fix the issue for Hannes' patchset. > visible log changes? I don't see a way to make the transition from > printk to dev_printk without having user visible changes to message content. The usage of sysfs symlinks for fixing the naming issue turned out to not be (easily) possible so we should consider other options (or just go forward with the original dev_printk() conversion).. Jens? Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index beca5f91bb4c..44c874367fe3 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6475,6 +6475,7 @@ EXPORT_SYMBOL(ata_link_printk); void ata_dev_printk(const struct ata_device *dev, const char *level, const char *fmt, ...) { + const struct device *gendev; struct va_format vaf; va_list args; @@ -6483,9 +6484,12 @@ 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); + gendev = (dev->sdev) ? &dev->sdev->sdev_gendev : &dev->tdev; + + dev_printk(level, gendev, "ata%u.%02u: %pV", + dev->link->ap->print_id, + dev->link->pmp + dev->devno, + &vaf); va_end(args); }
Utilize the dev_printk function which will add structured data to the log message. Signed-off-by: Tony Asleson <tasleson@redhat.com> --- drivers/ata/libata-core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)