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

[Hannes Reinecke <hare@suse.de>]

On 2/17/26 17:58, Igor Pylypiv wrote:
> 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.
> 
Hmm. Okay, sounds reasonable.

Reviewed-by: Hannes Reinecke <hare@suse.de>

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