Re: [PATCH v3 08/18] scsi: sd: set read/write commands CDL index

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 1/28/23 00:30, Hannes Reinecke wrote:
> On 1/24/23 20:02, Niklas Cassel wrote:
>> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>
>> Introduce the command duration limits helper function
>> sd_cdl_cmd_limit() to retrieve and set the DLD bits of the
>> READ/WRITE 16 and READ/WRITE 32 commands to indicate to the device
>> the command duration limit descriptor to apply to the command.
>>
>> When command duration limits are enabled, sd_cdl_cmd_limit() obtains the
>> index of the descriptor to apply to the command for requests that have
>> the IOPRIO_CLASS_DL priority class with a priority data sepcifying a
>> valid descriptor index (1 to 7).
>>
>> The read-write sysfs attribute "enable" is introduced to control
>> setting the command duration limits indexes. If this attribute is set
>> to 0 (default), command duration limits specified by the user are
>> ignored. The user must set this attribute to 1 for command duration
>> limits to be set. Enabling and disabling the command duration limits
>> feature for ATA devices must be done using the ATA feature sub-page of
>> the control mode page. The sd_cdl_enable() function is introduced to
>> check if this mode page is supported by the device and if it is, use
>> it to enable/disable CDL.
>>
>> 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>
>> ---
>>   drivers/scsi/sd.c     |  16 +++--
>>   drivers/scsi/sd.h     |  10 ++++
>>   drivers/scsi/sd_cdl.c | 134 +++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 152 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 7879a5470773..d2eb01337943 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1045,13 +1045,14 @@ static blk_status_t sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
>>   
>>   static blk_status_t sd_setup_rw32_cmnd(struct scsi_cmnd *cmd, bool write,
>>   				       sector_t lba, unsigned int nr_blocks,
>> -				       unsigned char flags)
>> +				       unsigned char flags, unsigned int dld)
>>   {
>>   	cmd->cmd_len = SD_EXT_CDB_SIZE;
>>   	cmd->cmnd[0]  = VARIABLE_LENGTH_CMD;
>>   	cmd->cmnd[7]  = 0x18; /* Additional CDB len */
>>   	cmd->cmnd[9]  = write ? WRITE_32 : READ_32;
>>   	cmd->cmnd[10] = flags;
>> +	cmd->cmnd[11] = dld & 0x07;
>>   	put_unaligned_be64(lba, &cmd->cmnd[12]);
>>   	put_unaligned_be32(lba, &cmd->cmnd[20]); /* Expected Indirect LBA */
>>   	put_unaligned_be32(nr_blocks, &cmd->cmnd[28]);
>> @@ -1061,12 +1062,12 @@ static blk_status_t sd_setup_rw32_cmnd(struct scsi_cmnd *cmd, bool write,
>>   
>>   static blk_status_t sd_setup_rw16_cmnd(struct scsi_cmnd *cmd, bool write,
>>   				       sector_t lba, unsigned int nr_blocks,
>> -				       unsigned char flags)
>> +				       unsigned char flags, unsigned int dld)
>>   {
>>   	cmd->cmd_len  = 16;
>>   	cmd->cmnd[0]  = write ? WRITE_16 : READ_16;
>> -	cmd->cmnd[1]  = flags;
>> -	cmd->cmnd[14] = 0;
>> +	cmd->cmnd[1]  = flags | ((dld >> 2) & 0x01);
>> +	cmd->cmnd[14] = (dld & 0x03) << 6;
>>   	cmd->cmnd[15] = 0;
>>   	put_unaligned_be64(lba, &cmd->cmnd[2]);
>>   	put_unaligned_be32(nr_blocks, &cmd->cmnd[10]);
>> @@ -1129,6 +1130,7 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
>>   	unsigned int mask = logical_to_sectors(sdp, 1) - 1;
>>   	bool write = rq_data_dir(rq) == WRITE;
>>   	unsigned char protect, fua;
>> +	unsigned int dld = 0;
>>   	blk_status_t ret;
>>   	unsigned int dif;
>>   	bool dix;
>> @@ -1178,6 +1180,8 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
>>   	fua = rq->cmd_flags & REQ_FUA ? 0x8 : 0;
>>   	dix = scsi_prot_sg_count(cmd);
>>   	dif = scsi_host_dif_capable(cmd->device->host, sdkp->protection_type);
>> +	if (sd_cdl_enabled(sdkp))
>> +		dld = sd_cdl_dld(sdkp, cmd);
>>   
>>   	if (dif || dix)
>>   		protect = sd_setup_protect_cmnd(cmd, dix, dif);
>> @@ -1186,10 +1190,10 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
>>   
>>   	if (protect && sdkp->protection_type == T10_PI_TYPE2_PROTECTION) {
>>   		ret = sd_setup_rw32_cmnd(cmd, write, lba, nr_blocks,
>> -					 protect | fua);
>> +					 protect | fua, dld);
>>   	} else if (sdp->use_16_for_rw || (nr_blocks > 0xffff)) {
>>   		ret = sd_setup_rw16_cmnd(cmd, write, lba, nr_blocks,
>> -					 protect | fua);
>> +					 protect | fua, dld);
>>   	} else if ((nr_blocks > 0xff) || (lba > 0x1fffff) ||
>>   		   sdp->use_10_for_rw || protect) {
>>   		ret = sd_setup_rw10_cmnd(cmd, write, lba, nr_blocks,
>> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
>> index e60d33bd222a..5b6b6dc4b92d 100644
>> --- a/drivers/scsi/sd.h
>> +++ b/drivers/scsi/sd.h
>> @@ -130,8 +130,11 @@ struct sd_cdl_page {
>>   	struct sd_cdl_desc      descs[SD_CDL_MAX_DESC];
>>   };
>>   
>> +struct scsi_disk;
>> +
>>   struct sd_cdl {
>>   	struct kobject		kobj;
>> +	struct scsi_disk	*sdkp;
>>   	bool			sysfs_registered;
>>   	u8			perf_vs_duration_guideline;
>>   	struct sd_cdl_page	pages[SD_CDL_RW];
>> @@ -188,6 +191,7 @@ struct scsi_disk {
>>   	u8		zeroing_mode;
>>   	u8		nr_actuators;		/* Number of actuators */
>>   	struct sd_cdl	*cdl;
>> +	unsigned	cdl_enabled : 1;
>>   	unsigned	ATO : 1;	/* state of disk ATO bit */
>>   	unsigned	cache_override : 1; /* temp override of WCE,RCD */
>>   	unsigned	WCE : 1;	/* state of disk WCE bit */
>> @@ -355,5 +359,11 @@ void sd_print_result(const struct scsi_disk *sdkp, const char *msg, int result);
>>   /* Command duration limits support (in sd_cdl.c) */
>>   void sd_read_cdl(struct scsi_disk *sdkp, unsigned char *buf);
>>   void sd_cdl_release(struct scsi_disk *sdkp);
>> +int sd_cdl_dld(struct scsi_disk *sdkp, struct scsi_cmnd *scmd);
>> +
>> +static inline bool sd_cdl_enabled(struct scsi_disk *sdkp)
>> +{
>> +	return sdkp->cdl && sdkp->cdl_enabled;
>> +}
>>   
>>   #endif /* _SCSI_DISK_H */
>> diff --git a/drivers/scsi/sd_cdl.c b/drivers/scsi/sd_cdl.c
>> index 513cd989f19a..59d02dbb5ea1 100644
>> --- a/drivers/scsi/sd_cdl.c
>> +++ b/drivers/scsi/sd_cdl.c
>> @@ -93,6 +93,63 @@ static const char *sd_cdl_policy_name(u8 policy)
>>   	}
>>   }
>>   
>> +/*
>> + * Enable/disable CDL.
>> + */
>> +static int sd_cdl_enable(struct scsi_disk *sdkp, bool enable)
>> +{
>> +	struct scsi_device *sdp = sdkp->device;
>> +	struct scsi_mode_data data;
>> +	struct scsi_sense_hdr sshdr;
>> +	struct scsi_vpd *vpd;
>> +	bool is_ata = false;
>> +	char buf[64];
>> +	int ret;
>> +
>> +	rcu_read_lock();
>> +	vpd = rcu_dereference(sdp->vpd_pg89);
>> +	if (vpd)
>> +		is_ata = true;
>> +	rcu_read_unlock();
>> +
>> +	/*
>> +	 * For ATA devices, CDL needs to be enabled with a SET FEATURES command.
>> +	 */
>> +	if (is_ata) {
>> +		char *buf_data;
>> +		int len;
>> +
>> +		ret = scsi_mode_sense(sdp, 0x08, 0x0a, 0xf2, buf, sizeof(buf),
>> +				      SD_TIMEOUT, sdkp->max_retries, &data,
>> +				      NULL);
>> +		if (ret)
>> +			return -EINVAL;
>> +
> That is a tad odd.
> Is CDL always enabled for 'normal' SCSI?

Yes it is on the device side. There is no mode sense to turn it on/off. Not sure
why it was designed like that in the specs... The sysfs duration_limits/enable
attribute is a "soft" on/off switch and it is off by default, even for drives
reporting supporting CDL.
Hence the "if (is_ata)" to do the mode sense to enable the feature on the device
side only for ATA devices. We need this to avoid having 2 different enable
pathes with 2 different sysfs "enable" attributes. Doing it like this is a lot
less code.

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research