Re: [PATCH v5 08/19] scsi: detect support for command duration limits

[Hannes Reinecke <hare@suse.de>]

On 4/5/23 01:27, Damien Le Moal wrote:
> On 4/5/23 03:48, Hannes Reinecke wrote:
>> On 4/4/23 20:24, Niklas Cassel wrote:
>>> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>
>>> Introduce the function scsi_cdl_check() to detect if a device supports
>>> command duration limits (CDL). Support for the READ 16, WRITE 16,
>>> READ 32 and WRITE 32 commands are checked using the function
>>> scsi_report_opcode() to probe the rwcdlp and cdlp bits as they indicate
>>> the mode page defining the command duration limits descriptors that
>>> apply to the command being tested.
>>>
>>> If any of these commands support CDL, the field cdl_supported of
>>> struct scsi_device is set to 1 to indicate that the device supports CDL.
>>>
>>> Support for CDL for a device is advertizes through sysfs using the new
>>> cdl_supported device attribute. This attribute value is 1 for a device
>>> supporting CDL and 0 otherwise.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> Co-developed-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>    Documentation/ABI/testing/sysfs-block-device |  9 +++
>>>    drivers/scsi/scsi.c                          | 81 ++++++++++++++++++++
>>>    drivers/scsi/scsi_scan.c                     |  3 +
>>>    drivers/scsi/scsi_sysfs.c                    |  2 +
>>>    include/scsi/scsi_device.h                   |  3 +
>>>    5 files changed, 98 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-block-device b/Documentation/ABI/testing/sysfs-block-device
>>> index 7ac7b19b2f72..ee3610a25845 100644
>>> --- a/Documentation/ABI/testing/sysfs-block-device
>>> +++ b/Documentation/ABI/testing/sysfs-block-device
>>> @@ -95,3 +95,12 @@ Description:
>>>    		This file does not exist if the HBA driver does not implement
>>>    		support for the SATA NCQ priority feature, regardless of the
>>>    		device support for this feature.
>>> +
>>> +
>>> +What:		/sys/block/*/device/cdl_supported
>>> +Date:		Mar, 2023
>>> +KernelVersion:	v6.4
>>> +Contact:	linux-scsi@vger.kernel.org
>>> +Description:
>>> +		(RO) Indicates if the device supports the command duration
>>> +		limits feature found in some ATA and SCSI devices.
>>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>>> index 62d9472e08e9..c03814ce23ca 100644
>>> --- a/drivers/scsi/scsi.c
>>> +++ b/drivers/scsi/scsi.c
>>> @@ -570,6 +570,87 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
>>>    }
>>>    EXPORT_SYMBOL(scsi_report_opcode);
>>>    
>>> +#define SCSI_CDL_CHECK_BUF_LEN	64
>>> +
>>> +static bool scsi_cdl_check_cmd(struct scsi_device *sdev, u8 opcode, u16 sa,
>>> +			       unsigned char *buf)
>>> +{
>>> +	int ret;
>>> +	u8 cdlp;
>>> +
>>> +	/* Check operation code */
>>> +	ret = scsi_report_opcode(sdev, buf, SCSI_CDL_CHECK_BUF_LEN, opcode, sa);
>>> +	if (ret <= 0)
>>> +		return false;
>>> +
>>> +	if ((buf[1] & 0x03) != 0x03)
>>> +		return false;
>>> +
>>> +	/* See SPC-6, one command format of REPORT SUPPORTED OPERATION CODES */
>>> +	cdlp = (buf[1] & 0x18) >> 3;
>>> +	if (buf[0] & 0x01) {
>>> +		/* rwcdlp == 1 */
>>> +		switch (cdlp) {
>>> +		case 0x01:
>>> +			/* T2A page */
>>> +			return true;
>>> +		case 0x02:
>>> +			/* T2B page */
>>> +			return true;
>>> +		}
>>> +	} else {
>>> +		/* rwcdlp == 0 */
>>> +		switch (cdlp) {
>>> +		case 0x01:
>>> +			/* A page */
>>> +			return true;
>>> +		case 0x02:
>>> +			/* B page */
>>> +			return true;
>>> +		}
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>> Why do we need to check this when writing to sysfs? Shouldn't we detect
>> this during startup / revalidate?
> 
> Hmm, I think you missed the call chain on this one (the patch starting with the
> sys attribute doc changes is a little confusing).
> 
> scsi_cdl_check_cmd() is called from scsi_cdl_check(), which is itself called
> from scsi_add_lun() (for initial device scan) and scsi_rescan_device() for
> revalidate. scsi_cdl_check() will set sdev->cdl_supported to 1 for devices
> supporting CDL, which is what sysfs cdl_supported show method uses, and later,
> the cdl_enable attribute show/store method use that too. That function is not
> called from sysfs attributes methods.
> 
> Note that this function was moved from sd.c to scsi.c as CDL is defined is SPC,
> not SBC.
> 
Indeed, you are right.

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, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman