* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk [not found] ` <CGME20200624103532eucas1p2c0988207e4dfc2f992d309b75deac3ee@eucas1p2.samsung.com> @ 2020-06-24 10:35 ` Bartlomiej Zolnierkiewicz 2020-06-24 15:15 ` Tony Asleson 2020-07-09 21:18 ` Tony Asleson 0 siblings, 2 replies; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-06-24 10:35 UTC (permalink / raw) To: Tony Asleson; +Cc: linux-scsi, linux-block, linux-ide [ 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-06-24 10:35 ` [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk Bartlomiej Zolnierkiewicz @ 2020-06-24 15:15 ` Tony Asleson 2020-06-26 12:45 ` Bartlomiej Zolnierkiewicz 2020-07-09 21:18 ` Tony Asleson 1 sibling, 1 reply; 13+ messages in thread From: Tony Asleson @ 2020-06-24 15:15 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: linux-scsi, linux-block, linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-06-24 15:15 ` Tony Asleson @ 2020-06-26 12:45 ` Bartlomiej Zolnierkiewicz 2020-06-26 13:54 ` Tony Asleson 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-06-26 12:45 UTC (permalink / raw) To: tasleson; +Cc: linux-scsi, linux-block, linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-06-26 12:45 ` Bartlomiej Zolnierkiewicz @ 2020-06-26 13:54 ` Tony Asleson 0 siblings, 0 replies; 13+ messages in thread From: Tony Asleson @ 2020-06-26 13:54 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: linux-scsi, linux-block, linux-ide 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-06-24 10:35 ` [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk Bartlomiej Zolnierkiewicz 2020-06-24 15:15 ` Tony Asleson @ 2020-07-09 21:18 ` Tony Asleson 2020-07-14 8:06 ` Bartlomiej Zolnierkiewicz 1 sibling, 1 reply; 13+ messages in thread From: Tony Asleson @ 2020-07-09 21:18 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-09 21:18 ` Tony Asleson @ 2020-07-14 8:06 ` Bartlomiej Zolnierkiewicz 2020-07-14 8:17 ` Greg Kroah-Hartman 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-07-14 8:06 UTC (permalink / raw) To: tasleson; +Cc: linux-ide, linux-scsi, linux-block, Greg Kroah-Hartman 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-14 8:06 ` Bartlomiej Zolnierkiewicz @ 2020-07-14 8:17 ` Greg Kroah-Hartman 2020-07-14 8:50 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Greg Kroah-Hartman @ 2020-07-14 8:17 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: tasleson, linux-ide, linux-scsi, linux-block 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-14 8:17 ` Greg Kroah-Hartman @ 2020-07-14 8:50 ` Bartlomiej Zolnierkiewicz 2020-07-17 10:06 ` Greg Kroah-Hartman 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-07-14 8:50 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: tasleson, linux-ide, linux-scsi, linux-block 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-14 8:50 ` Bartlomiej Zolnierkiewicz @ 2020-07-17 10:06 ` Greg Kroah-Hartman 2020-07-17 10:17 ` Hannes Reinecke 2020-07-17 10:27 ` Bartlomiej Zolnierkiewicz 0 siblings, 2 replies; 13+ messages in thread From: Greg Kroah-Hartman @ 2020-07-17 10:06 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: tasleson, linux-ide, linux-scsi, linux-block 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-17 10:06 ` Greg Kroah-Hartman @ 2020-07-17 10:17 ` Hannes Reinecke 2020-07-17 10:27 ` Bartlomiej Zolnierkiewicz 1 sibling, 0 replies; 13+ messages in thread From: Hannes Reinecke @ 2020-07-17 10:17 UTC (permalink / raw) To: Greg Kroah-Hartman, Bartlomiej Zolnierkiewicz Cc: tasleson, linux-ide, linux-scsi, linux-block 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 -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), GF: Felix Imendörffer ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-17 10:06 ` Greg Kroah-Hartman 2020-07-17 10:17 ` Hannes Reinecke @ 2020-07-17 10:27 ` Bartlomiej Zolnierkiewicz 2020-07-17 19:47 ` Tony Asleson 1 sibling, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-07-17 10:27 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: tasleson, linux-ide, linux-scsi, linux-block, Jens Axboe 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-17 10:27 ` Bartlomiej Zolnierkiewicz @ 2020-07-17 19:47 ` Tony Asleson 2020-07-24 8:50 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Tony Asleson @ 2020-07-17 19:47 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman Cc: linux-ide, linux-scsi, linux-block, Jens Axboe 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk 2020-07-17 19:47 ` Tony Asleson @ 2020-07-24 8:50 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-07-24 8:50 UTC (permalink / raw) To: tasleson, Jens Axboe Cc: Greg Kroah-Hartman, linux-ide, linux-scsi, linux-block 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-24 8:50 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20200623191749.115200-1-tasleson@redhat.com> [not found] ` <20200623191749.115200-6-tasleson@redhat.com> [not found] ` <CGME20200624103532eucas1p2c0988207e4dfc2f992d309b75deac3ee@eucas1p2.samsung.com> 2020-06-24 10:35 ` [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk Bartlomiej Zolnierkiewicz 2020-06-24 15:15 ` Tony Asleson 2020-06-26 12:45 ` Bartlomiej Zolnierkiewicz 2020-06-26 13:54 ` Tony Asleson 2020-07-09 21:18 ` Tony Asleson 2020-07-14 8:06 ` Bartlomiej Zolnierkiewicz 2020-07-14 8:17 ` Greg Kroah-Hartman 2020-07-14 8:50 ` Bartlomiej Zolnierkiewicz 2020-07-17 10:06 ` Greg Kroah-Hartman 2020-07-17 10:17 ` Hannes Reinecke 2020-07-17 10:27 ` Bartlomiej Zolnierkiewicz 2020-07-17 19:47 ` Tony Asleson 2020-07-24 8:50 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).