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

[Igor Pylypiv <ipylypiv@google.com>]

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.

> 
> > +	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