[RFC PATCH v3 0/8] Add persistent durable identifier to storage log messages

[RFC PATCH v3 0/8] Add persistent durable identifier to storage log messages

[Tony Asleson]

Today users have no easy way to correlate kernel log messages for storage
devices across reboots, device dynamic add/remove, or when the device is
physically or logically moved from from system to system.  This is due
to the existing log IDs which identify how the device is attached and not
a unique ID of what is attached.  Additionally, even when the attachment
hasn't changed, it's not always obvious which messages belong to the
device as the different areas in the storage stack use different
identifiers, eg. (sda, sata1.00, sd 0:0:0:0).

This change addresses this by adding a unique ID to each log
message.  It couples the existing structured key/value logging capability
and VPD 0x83 device identification.


An example of logs filtered for a specific device utilizing this patch
series.

$ journalctl -b  _KERNEL_DURABLE_NAME="`cat /sys/block/sdb/device/wwid`"
| cut -c 25- | fmt -t
9-08-22 13:21:35 CDT, end at Wed 2020-05-13 15:40:26 CDT. --
l: scsi 1:0:0:0: Attached scsi generic sg1 type 0
l: sd 1:0:0:0: [sdb] 209715200 512-byte logical blocks: (107 GB/100 GiB)
l: sd 1:0:0:0: [sdb] Write Protect is off
l: sd 1:0:0:0: [sdb] Mode Sense: 00 3a 00 00
l: sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't
   support DPO or FUA
l: sd 1:0:0:0: [sdb] Attached SCSI disk
l: sd 1:0:0:0: ata2.00: exception Emask 0x0 SAct 0x800000 SErr 0x800000
   action 0x6 frozen
l: sd 1:0:0:0: ata2.00: failed command: READ FPDMA QUEUED
l: sd 1:0:0:0: ata2.00: cmd 60/20:b8:10:27:00/00:00:00:00:00/40 tag 23
            ncq dma 16384 in res 40/00:00:00:4f:c2/00:00:00:00:00/00
            Emask 0x4 (timeout)
l: sd 1:0:0:0: ata2.00: status: { DRDY }
l: sd 1:0:0:0: ata2.00: configured for UDMA/100
l: sd 1:0:0:0: [sdb] tag#23 FAILED Result: hostbyte=DID_OK
            driverbyte=DRIVER_SENSE cmd_age=30s
l: sd 1:0:0:0: [sdb] tag#23 Sense Key : Illegal Request [current]
l: sd 1:0:0:0: [sdb] tag#23 Add. Sense: Unaligned write command
l: sd 1:0:0:0: [sdb] tag#23 CDB: Read(10) 28 00 00 00 27 10 00 00 20 00
l: block sdb: blk_update_request: I/O error, dev sdb, sector 10000 op
            0x0:(READ) flags 0x80700 phys_seg 4 prio class 0
l: sd 1:0:0:0: ata2.00: exception Emask 0x0 SAct 0x2 SErr 0x2 action
            0x6 frozen
l: sd 1:0:0:0: ata2.00: failed command: READ FPDMA QUEUED
l: sd 1:0:0:0: ata2.00: cmd 60/08:08:10:27:00/00:00:00:00:00/40 tag 1 ncq
            dma 4096 in res 40/00:ff:00:00:00/00:00:00:00:00/00 Emask 0x4
            (timeout)
l: sd 1:0:0:0: ata2.00: status: { DRDY }
l: sd 1:0:0:0: ata2.00: configured for UDMA/100
l: sd 1:0:0:0: ata2.00: exception Emask 0x0 SAct 0x800000 SErr 0x800000
            action 0x6 frozen
l: sd 1:0:0:0: ata2.00: failed command: READ FPDMA QUEUED
l: sd 1:0:0:0: ata2.00: cmd 60/08:b8:10:27:00/00:00:00:00:00/40 tag 23 ncq
            dma 4096 in res 40/00:00:00:4f:c2/00:00:00:00:00/00 Emask 0x4
            (timeout)
l: sd 1:0:0:0: ata2.00: status: { DRDY }
l: sd 1:0:0:0: ata2.00: configured for UDMA/100
l: sd 1:0:0:0: ata2.00: NCQ disabled due to excessive errors
l: sd 1:0:0:0: ata2.00: exception Emask 0x0 SAct 0x4000 SErr 0x4000
            action 0x6 frozen
l: sd 1:0:0:0: ata2.00: failed command: READ FPDMA QUEUED
l: sd 1:0:0:0: ata2.00: cmd 60/08:70:10:27:00/00:00:00:00:00/40 tag 14 ncq
            dma 4096 in res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4
            (timeout)
l: sd 1:0:0:0: ata2.00: status: { DRDY }
l: sd 1:0:0:0: ata2.00: configured for UDMA/100
l: sd 1:0:0:0: ata2.00: device reported invalid CHS sector 0

This change is incomplete.  With the plethora of different logging
techniques utilized in the kernel it will take some coordinated effort
and additional changes.  I tried a few different approaches, to cover
as much as I could without resorting to changing every print statement
in all the storage layers, but maybe there is a better,
more elegant approach?

I believe having this functionality is very useful and important for
system configurations of all sizes.  I mentioned this change briefly in:
https://lore.kernel.org/lkml/30f29fe6-8445-0016-8cdc-3ef99d43fbf5@redhat.com/

Questions
1. Where is the best place to put the durable_name callback function?
2. What is best "KEY" value?
3. Should we re-work the message contents themselves to remove any redundant
   information that is included when using dev_printk? ref. 
   https://lore.kernel.org/linux-scsi/e12aeb9e-fe5d-5b5e-d190-401997cecc34@redhat.com/#t


v2:
- Incorporated changes suggested by James Bottomley
- Removed string function which removed leading/trailing/duplicate adjacent
  spaces from generated id, value matches /sys/block/<device>/device/wwid
- Remove xfs patch, limiting changes to lower block layers
- Moved callback from struct device_type to struct device.  Struct device_type
  is typically static const and with a number of different areas using shared
  implementation of genhd unable to modify for each of the different areas.

v3:
- Increase the size of the buffers for NVMe id generation and
  dev_vprintk_emit

Tony Asleson (8):
  struct device: Add function callback durable_name
  create_syslog_header: Add durable name
  print_req_error: Use dev_printk
  buffer_io_error: Use dev_printk
  ata_dev_printk: Use dev_printk
  scsi: Add durable_name for dev_printk
  nvme: Add durable name for dev_printk
  dev_vprintk_emit: Increase hdr size

 block/blk-core.c           |  5 ++++-
 drivers/ata/libata-core.c  | 10 +++++++---
 drivers/base/core.c        | 31 ++++++++++++++++++++++++++++++-
 drivers/nvme/host/core.c   | 18 ++++++++++++++++++
 drivers/scsi/scsi_lib.c    | 14 ++++++++++++++
 drivers/scsi/scsi_sysfs.c  | 23 +++++++++++++++++++++++
 drivers/scsi/sd.c          |  2 ++
 fs/buffer.c                | 10 ++++++++--
 include/linux/device.h     |  4 ++++
 include/scsi/scsi_device.h |  3 +++
 10 files changed, 113 insertions(+), 7 deletions(-)


base-commit: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
-- 
2.25.4

[RFC PATCH v3 1/8] struct device: Add function callback durable_name

[Tony Asleson]

Function callback and function to be used to write a persistent
durable name to the supplied character buffer.  This will be used to add
structured key-value data to log messages for hardware related errors
which allows end users to correlate message and specific hardware.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 drivers/base/core.c    | 24 ++++++++++++++++++++++++
 include/linux/device.h |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0cad34f1eede..511b7d2fc916 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2304,6 +2304,30 @@ int dev_set_name(struct device *dev, const char *fmt, ...)
 }
 EXPORT_SYMBOL_GPL(dev_set_name);
 
+/**
+ * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer
+ * @dev: device
+ * @buffer: character buffer to write results
+ * @len: length of buffer
+ * @return: Number of bytes written to buffer
+ */
+int dev_durable_name(const struct device *dev, char *buffer, size_t len)
+{
+	int tmp, dlen;
+
+	if (dev && dev->durable_name) {
+		tmp = snprintf(buffer, len, "DURABLE_NAME=");
+		if (tmp < len) {
+			dlen = dev->durable_name(dev, buffer + tmp,
+							len - tmp);
+			if (dlen > 0 && ((dlen + tmp) < len))
+				return dlen + tmp;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_durable_name);
+
 /**
  * device_to_dev_kobj - select a /sys/dev/ directory for the device
  * @dev: device
diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..281755404c21 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -613,6 +613,8 @@ struct device {
 	struct iommu_group	*iommu_group;
 	struct dev_iommu	*iommu;
 
+	int (*durable_name)(const struct device *dev, char *buff, size_t len);
+
 	bool			offline_disabled:1;
 	bool			offline:1;
 	bool			of_node_reused:1;
@@ -654,6 +656,8 @@ 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 dev_durable_name(const struct device *d, char *buffer, size_t len);
+
 #ifdef CONFIG_NUMA
 static inline int dev_to_node(struct device *dev)
 {
-- 
2.25.4

[RFC PATCH v3 2/8] create_syslog_header: Add durable name

[Tony Asleson]

This gets us a persistent durable name for code that logs messages in the
block layer that have the appropriate callbacks setup for durable name.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 drivers/base/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 511b7d2fc916..964690572a89 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3754,6 +3754,7 @@ create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
 {
 	const char *subsys;
 	size_t pos = 0;
+	int dlen;
 
 	if (dev->class)
 		subsys = dev->class->name;
@@ -3796,6 +3797,10 @@ create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
 				"DEVICE=+%s:%s", subsys, dev_name(dev));
 	}
 
+	dlen = dev_durable_name(dev, hdr + (pos + 1), hdrlen - (pos + 1));
+	if (dlen)
+		pos += dlen + 1;
+
 	if (pos >= hdrlen)
 		goto overflow;
 
-- 
2.25.4

[RFC PATCH v3 3/8] print_req_error: Use dev_printk

[Tony Asleson]

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 block/blk-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9bfaee050c82..bd57278d7113 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -213,12 +213,15 @@ EXPORT_SYMBOL_GPL(blk_status_to_errno);
 static void print_req_error(struct request *req, blk_status_t status,
 		const char *caller)
 {
+	struct device *dev;
 	int idx = (__force int)status;
 
 	if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors)))
 		return;
 
-	printk_ratelimited(KERN_ERR
+	dev = (req->rq_disk) ? disk_to_dev(req->rq_disk) : NULL;
+
+	dev_err_ratelimited(dev,
 		"%s: %s error, dev %s, sector %llu op 0x%x:(%s) flags 0x%x "
 		"phys_seg %u prio class %u\n",
 		caller, blk_errors[idx].name,
-- 
2.25.4

[RFC PATCH v3 4/8] buffer_io_error: Use dev_printk

[Tony Asleson]

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 fs/buffer.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index a60f60396cfa..97b8f455c031 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -133,10 +133,16 @@ __clear_page_buffers(struct page *page)
 
 static void buffer_io_error(struct buffer_head *bh, char *msg)
 {
-	if (!test_bit(BH_Quiet, &bh->b_state))
-		printk_ratelimited(KERN_ERR
+	if (!test_bit(BH_Quiet, &bh->b_state)) {
+		struct device *gendev;
+
+		gendev = (bh->b_bdev->bd_disk) ?
+			disk_to_dev(bh->b_bdev->bd_disk) : NULL;
+
+		dev_err_ratelimited(gendev,
 			"Buffer I/O error on dev %pg, logical block %llu%s\n",
 			bh->b_bdev, (unsigned long long)bh->b_blocknr, msg);
+	}
 }
 
 /*
-- 
2.25.4

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

[Tony Asleson]

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,
+			dev->link->pmp + dev->devno,
+			&vaf);
 
 	va_end(args);
 }
-- 
2.25.4

[RFC PATCH v3 6/8] scsi: Add durable_name for dev_printk

[Tony Asleson]

Add the needed functions to fill out the durable_name function
call back for scsi based storage devices.  This allows calls
into dev_printk for scsi devices to have a persistent id
associated with them.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 14 ++++++++++++++
 drivers/scsi/scsi_sysfs.c  | 23 +++++++++++++++++++++++
 drivers/scsi/sd.c          |  2 ++
 include/scsi/scsi_device.h |  3 +++
 4 files changed, 42 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 06c260f6cdae..9f6c41162c55 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3142,3 +3142,17 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 	return group_id;
 }
 EXPORT_SYMBOL(scsi_vpd_tpg_id);
+
+int scsi_durable_name(struct scsi_device *sdev, char *buf, size_t len)
+{
+	int vpd_len = 0;
+
+	vpd_len = scsi_vpd_lun_id(sdev, buf, len);
+	if (vpd_len > 0 && vpd_len < len)
+		vpd_len++;
+	else
+		vpd_len = 0;
+
+	return vpd_len;
+}
+EXPORT_SYMBOL(scsi_durable_name);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 163dbcb741c1..f719b63f4b63 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1582,6 +1582,28 @@ static struct device_type scsi_dev_type = {
 	.groups =	scsi_sdev_attr_groups,
 };
 
+
+int dev_to_scsi_durable_name(const struct device *dev, char *buf, size_t len)
+{
+	struct scsi_device *sd_dev = NULL;
+
+	// When we go through dev_printk in the scsi layer, dev is embedded
+	// in a struct scsi_device.  When we go through the block layer,
+	// dev is embedded in struct genhd, thus we need different paths to
+	// retrieve the struct scsi_device to call scsi_durable_name.
+	if (dev->type == &scsi_dev_type) {
+		sd_dev = to_scsi_device(dev);
+	} else if (dev->parent && dev->parent->type == &scsi_dev_type) {
+		sd_dev = to_scsi_device(dev->parent);
+	} else {
+		// We have a pointer to something else, bail
+		return 0;
+	}
+
+	return scsi_durable_name(sd_dev, buf, len);
+}
+EXPORT_SYMBOL(dev_to_scsi_durable_name);
+
 void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 {
 	unsigned long flags;
@@ -1591,6 +1613,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 	device_initialize(&sdev->sdev_gendev);
 	sdev->sdev_gendev.bus = &scsi_bus_type;
 	sdev->sdev_gendev.type = &scsi_dev_type;
+	sdev->sdev_gendev.durable_name = dev_to_scsi_durable_name;
 	dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
 		     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a793cb08d025..f40e4cb4a5f6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3360,6 +3360,8 @@ static int sd_probe(struct device *dev)
 	gd->private_data = &sdkp->driver;
 	gd->queue = sdkp->device->request_queue;
 
+	disk_to_dev(gd)->durable_name = dev_to_scsi_durable_name;
+
 	/* defaults, until the device tells us otherwise */
 	sdp->sector_size = 512;
 	sdkp->capacity = 0;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c3cba2aaf934..7be5861565f7 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -461,6 +461,9 @@ extern void sdev_disable_disk_events(struct scsi_device *sdev);
 extern void sdev_enable_disk_events(struct scsi_device *sdev);
 extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
 extern int scsi_vpd_tpg_id(struct scsi_device *, int *);
+extern int dev_to_scsi_durable_name(const struct device *dev, char *buf,
+					size_t len);
+extern int scsi_durable_name(struct scsi_device *sdev, char *buf, size_t len);
 
 #ifdef CONFIG_PM
 extern int scsi_autopm_get_device(struct scsi_device *);
-- 
2.25.4

[RFC PATCH v3 7/8] nvme: Add durable name for dev_printk

[Tony Asleson]

Corrections from Keith Busch review comments.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 drivers/nvme/host/core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f3c037f5a9ba..f2e5b91668a1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2667,6 +2667,22 @@ static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
 	return true;
 }
 
+static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
+			char *buf);
+
+static int dev_to_nvme_durable_name(const struct device *dev, char *buf, size_t len)
+{
+	char serial[144];	/* Max 141 for wwid_show */
+	ssize_t serial_len = wwid_show((struct device *)dev, NULL, serial);
+
+	if (serial_len > 0 && serial_len < len) {
+		serial_len -= 1;  /* Remove the '\n' from the string */
+		strncpy(buf, serial, serial_len);
+		return serial_len;
+	}
+	return 0;
+}
+
 static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
 	struct nvme_subsystem *subsys, *found;
@@ -3616,6 +3632,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	disk->queue = ns->queue;
 	disk->flags = flags;
 	memcpy(disk->disk_name, disk_name, DISK_NAME_LEN);
+	disk_to_dev(disk)->durable_name = dev_to_nvme_durable_name;
+
 	ns->disk = disk;
 
 	__nvme_revalidate_disk(disk, id);
-- 
2.25.4

[RFC PATCH v3 8/8] dev_vprintk_emit: Increase hdr size

[Tony Asleson]

With the addition of the device persistent id we have the possibility
of adding 154 more bytes to the hdr.  Thus if we assume the previous
size of 128 was sufficent we can simply add the 2 amounts and round
up.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 drivers/base/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 964690572a89..c2439d12608d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3814,7 +3814,7 @@ create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
 int dev_vprintk_emit(int level, const struct device *dev,
 		     const char *fmt, va_list args)
 {
-	char hdr[128];
+	char hdr[288];
 	size_t hdrlen;
 
 	hdrlen = create_syslog_header(dev, hdr, sizeof(hdr));
-- 
2.25.4

Re: [RFC PATCH v3 7/8] nvme: Add durable name for dev_printk

[Chaitanya Kulkarni]

On 6/23/20 12:18 PM, Tony Asleson wrote:
> Corrections from Keith Busch review comments.
> 
> Signed-off-by: Tony Asleson<tasleson@redhat.com>
> ---
>   drivers/nvme/host/core.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
This looks useful me, but why you still have an RFC fag ?

Re: [RFC PATCH v3 7/8] nvme: Add durable name for dev_printk

[Tony Asleson]

On 6/23/20 3:04 PM, Chaitanya Kulkarni wrote:
> On 6/23/20 12:18 PM, Tony Asleson wrote:
>> Corrections from Keith Busch review comments.
>>
>> Signed-off-by: Tony Asleson<tasleson@redhat.com>
>> ---
>>   drivers/nvme/host/core.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
> This looks useful me, but why you still have an RFC fag ?

I have some questions in the cover letter that I was thinking should be
discussed before removing RFC, but maybe I'm going about this the wrong
way and should remove that?

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

[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