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

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

[Bartlomiej Zolnierkiewicz]

[ 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

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

[Tony Asleson]

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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Tony Asleson]

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

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

[Tony Asleson]

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

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

[Bartlomiej Zolnierkiewicz]

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
> 

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

[Greg Kroah-Hartman]

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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Greg Kroah-Hartman]

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

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

[Hannes Reinecke]

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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Tony Asleson]

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

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

[Bartlomiej Zolnierkiewicz]

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