Re: [PATCH v3] scsi: core: Add 'serial' sysfs attribute for SCSI/SATA

[Igor Pylypiv <ipylypiv@google.com>]

On Tue, Feb 10, 2026 at 08:51:24AM -0800, Igor Pylypiv wrote:
> On Tue, Feb 10, 2026 at 12:38:51PM +0100, Hannes Reinecke wrote:
> > On 2/9/26 22:21, Igor Pylypiv wrote:
> > > Add a 'serial' sysfs attribute for SCSI and SATA devices. This attribute
> > > exposes the Unit Serial Number, which is derived from the Device
> > > Identification Vital Product Data (VPD) page 0x80.
> > > 
> > > Whitespace is stripped from the retrieved serial number to handle
> > > the different alignment (right-aligned for SCSI, potentially
> > > left-aligned for SATA). As noted in SAT-5 10.5.3, "Although SPC-5 defines
> > > the PRODUCT SERIAL NUMBER field as right-aligned, ACS-5 does not require
> > > its SERIAL NUMBER field to be right-aligned. Therefore, right-alignment
> > > of the PRODUCT SERIAL NUMBER field for the translation is not assured."
> > > 
> > > This attribute is used by tools such as lsblk to display the serial
> > > number of block devices.
> > > 
> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > > ---
> > > 
> > > v2->v3 changes:
> > > - Replaced sysfs_emit(buf, "%s\n", buf) with a manual newline placement
> > >    to avoid undefined behavior of passing the output buffer as an input.
> > > 
> > > v1->v2 changes:
> > > - Reordered declarations in scsi_vpd_lun_serial() from longest to shortest.
> > > - Replaced rcu_read_lock()/rcu_read_unlock() with guard(rcu)().
> > > 
> > > 
> > >   drivers/scsi/scsi_lib.c    | 47 ++++++++++++++++++++++++++++++++++++++
> > >   drivers/scsi/scsi_sysfs.c  | 16 +++++++++++++
> > >   include/scsi/scsi_device.h |  1 +
> > >   3 files changed, 64 insertions(+)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 4a902c9dfd8b..c17fbe4dd845 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -13,6 +13,7 @@
> > >   #include <linux/bitops.h>
> > >   #include <linux/blkdev.h>
> > >   #include <linux/completion.h>
> > > +#include <linux/ctype.h>
> > >   #include <linux/kernel.h>
> > >   #include <linux/export.h>
> > >   #include <linux/init.h>
> > > @@ -3459,6 +3460,52 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
> > >   }
> > >   EXPORT_SYMBOL(scsi_vpd_lun_id);
> > > +/**
> > > + * scsi_vpd_lun_serial - return a unique device serial number
> > > + * @sdev: SCSI device
> > > + * @sn:   buffer for the serial number
> > > + * @sn_size: size of the buffer
> > > + *
> > > + * Copies the device serial number into @sn based on the information in
> > > + * the VPD page 0x80 of the device. The string will be null terminated
> > > + * and have leading and trailing whitespace stripped.
> > > + *
> > > + * Returns the length of the serial number or error on failure.
> > > + */
> > > +int scsi_vpd_lun_serial(struct scsi_device *sdev, char *sn, size_t sn_size)
> > > +{
> > > +	const struct scsi_vpd *vpd_pg80;
> > > +	const unsigned char *d;
> > > +	int len;
> > > +
> > > +	guard(rcu)();
> > > +	vpd_pg80 = rcu_dereference(sdev->vpd_pg80);
> > > +	if (!vpd_pg80)
> > > +		return -ENXIO;
> > > +
> > > +	len = vpd_pg80->len - 4;
> > > +	d = vpd_pg80->data + 4;
> > > +
> > > +	/* Skip leading spaces */
> > > +	while (len > 0 && isspace(*d)) {
> > > +		len--;
> > > +		d++;
> > > +	}
> > > +
> > > +	/* Skip trailing spaces */
> > > +	while (len > 0 && isspace(d[len - 1]))
> > > +		len--;
> > > +
> > 
> > Please use 'strim()' instead.
> 
> Hi Hannes,
> 
> Bart pointed this out in V1 as well. I'll copy-paste my reply from V1:
> 
> "Yes, I considered using strim(). strim() modifies the input buffer by
> replacing first trailing whitespace with '\0' so we can't use it directly
> on the vpd_pg80->data. The solution would be to copy the whole vpd page
> data into the sn buffer and call strim() on the sn buffer. strim() returns
> a pointer to the first non-whitespace character so we would also need to
> memmove the serial number to the beginning of the sn buffer. All this extra
> copying seems to be redundant so I went ahead with a simpler solution
> that does a single memcpy()."
> 
> Please let me know your thoughts on this.

Hi Hannes,

Ping for a feedback.
Sending this in case my previous reply fell through the cracks.

Thank you!
Igor

> 
> > 
> > > +	if (sn_size < len + 1)
> > > +		return -EINVAL;
> > > +
> > > +	memcpy(sn, d, len);
> > 
> > 'len' might well be '0' after 'strim()', please check
> > before calling 'memcpy'.
> 
> It looks like calling a memcpy() with zero length is a no-op. Is checking
> for len > 0 really necessary in this case?
> 
> Thank you,
> Igor
> 
> > > +	sn[len] = '\0';
> > > +
> > > +	return len;
> > > +}
> > > +EXPORT_SYMBOL(scsi_vpd_lun_serial);
> > > +
> > >   /**
> > >    * scsi_vpd_tpg_id - return a target port group identifier
> > >    * @sdev: SCSI device
> > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > > index 99eb0a30df61..9c4f47e7a298 100644
> > > --- a/drivers/scsi/scsi_sysfs.c
> > > +++ b/drivers/scsi/scsi_sysfs.c
> > > @@ -1013,6 +1013,21 @@ sdev_show_wwid(struct device *dev, struct device_attribute *attr,
> > >   }
> > >   static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
> > > +static ssize_t
> > > +sdev_show_serial(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct scsi_device *sdev = to_scsi_device(dev);
> > > +	ssize_t ret;
> > > +
> > > +	ret = scsi_vpd_lun_serial(sdev, buf, PAGE_SIZE);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	buf[ret] = '\n';
> > > +	return ret + 1;
> > > +}
> > > +static DEVICE_ATTR(serial, S_IRUGO, sdev_show_serial, NULL);
> > > +
> > >   #define BLIST_FLAG_NAME(name)					\
> > >   	[const_ilog2((__force __u64)BLIST_##name)] = #name
> > >   static const char *const sdev_bflags_name[] = {
> > > @@ -1257,6 +1272,7 @@ static struct attribute *scsi_sdev_attrs[] = {
> > >   	&dev_attr_device_busy.attr,
> > >   	&dev_attr_vendor.attr,
> > >   	&dev_attr_model.attr,
> > > +	&dev_attr_serial.attr,
> > >   	&dev_attr_rev.attr,
> > >   	&dev_attr_rescan.attr,
> > >   	&dev_attr_delete.attr,
> > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > > index d32f5841f4f8..9c2a7bbe5891 100644
> > > --- a/include/scsi/scsi_device.h
> > > +++ b/include/scsi/scsi_device.h
> > > @@ -571,6 +571,7 @@ void scsi_put_internal_cmd(struct scsi_cmnd *scmd);
> > >   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_lun_serial(struct scsi_device *, char *, size_t);
> > >   extern int scsi_vpd_tpg_id(struct scsi_device *, int *);
> > >   #ifdef CONFIG_PM
> > 
> > Otherwise looks okay.
> > 
> > Cheers,
> > 
> > Hannes
> > -- 
> > Dr. Hannes Reinecke                  Kernel Storage Architect
> > hare@suse.de                                +49 911 74053 688
> > SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> > HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich