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

[Bart Van Assche]

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.

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

> +	rcu_read_lock();

Please use guard(rcu)() in new code.

> +	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()?

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

Thanks,

Bart.