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

[Igor Pylypiv <ipylypiv@google.com>]

On Wed, Jan 14, 2026 at 12:40:16PM -0800, Bart Van Assche wrote:
> On 1/14/26 10:51 AM, 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.
> 
> How can existing user space tools use a sysfs attribute that has not yet
> been implemented? Please explain.


The 'serial' sysfs attribute is implemented for NVMe. Since 'serial' sysfs
attribute is missing for SCSI/SATA devices, lsblk reports an empty string
instead of a serial number.

# lsblk --nodeps -o name,serial
NAME    SERIAL
sda     
sdb     

> 
> > +int scsi_vpd_lun_serial(struct scsi_device *sdev, char *sn, size_t sn_size)
> > +{
> > +	int len;
> > +	const unsigned char *d;
> > +	const struct scsi_vpd *vpd_pg80;
> 
> The current convention for declarations in Linux kernel code is to order
> these from longest to shortest. In other words, the opposite order of
> the above order.
>

Thanks for pointing this out. I'll reorder the declarations in V2.
 
> > +	rcu_read_lock();
> 
> Please use guard(rcu)() in new code.

Thanks. I'll fix this in V2.
 
> > +	vpd_pg80 = rcu_dereference(sdev->vpd_pg80);
> > +	if (!vpd_pg80) {
> > +		rcu_read_unlock();
> > +		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--;
> > +
> > +	if (sn_size < len + 1) {
> > +		rcu_read_unlock();
> > +		return -EINVAL;
> > +	}
> 
> Has it been considered to call strim() instead of implementing functionality
> that is very similar to strim()?

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().

> > +	return sysfs_emit(buf, "%s\n", buf);
> 
> The C99 standard says that passing the output buffer pointer as an argument
> to sprintf()/snprintf() triggers undefined behavior. I'm not sure whether
> this also applies to the kernel equivalents of these
> functions but it's probably better to be careful.

Kernel implementation of sysfs_emit() seems to be fine. The documentation
states that sysfs_emit()/sysfs_emit_at() should be used for show() methods.

https://github.com/torvalds/linux/blob/603c05a1639f60e0c52c5fdd25cf5e0b44b9bd8e/Documentation/filesystems/sysfs.rst?plain=1#L246-L247

Thanks,
Igor

> Thanks,
> 
> Bart.