Re: [PATCH v3 07/18] scsi: sd: detect support for command duration limits

[Hannes Reinecke <hare@suse.de>]

On 1/24/23 20:02, Niklas Cassel wrote:
> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Detect if a disk supports command duration limits. Support for
> the READ 16, WRITE 16, READ 32 and WRITE 32 commands is tested using
> the function scsi_report_opcode(). For a disk supporting command
> duration limits, the mode page indicating the command duration limits
> descriptors that apply to the command is indicated using the rwcdlp
> and cdlp bits.
> 
> Support duration limits is advertizes through sysfs using the new
> "duration_limits" sysfs sub-directory of the generic device directory,
> that is, /sys/block/sdX/device/duration_limits. Within this new
> directory, the limit descriptors that apply to read and write operations
> are exposed within the read and write directories, with descriptor
> attributes grouped together in directories. The overall sysfs structure
> created is:
> 
> /sys/block/sde/device/duration_limits/
> ├── perf_vs_duration_guideline
> ├── read
> │   ├── 1
> │   │   ├── duration_guideline
> │   │   ├── duration_guideline_policy
> │   │   ├── max_active_time
> │   │   ├── max_active_time_policy
> │   │   ├── max_inactive_time
> │   │   └── max_inactive_time_policy
> │   ├── 2
> │   │   ├── duration_guideline
> ...
> │   └── page
> └── write
>      ├── 1
>      │   ├── duration_guideline
>      │   ├── duration_guideline_policy
> ...
> 
> For each of the read and write descriptor directories, the page
> attribute file indicate the command duration limit page providing the
> descriptors. The possible values for the page attribute are "A", "B",
> "T2A" and "T2B".
> 
> The new "duration_limits" attributes directory is added only for disks
> that supports command duration limits.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/scsi/Makefile |   2 +-
>   drivers/scsi/sd.c     |   2 +
>   drivers/scsi/sd.h     |  61 ++++
>   drivers/scsi/sd_cdl.c | 764 ++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 828 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/scsi/sd_cdl.c
> 
I'm not particularly happy with having sysfs reflect user settings, but 
every other place I can think of is even more convoluted.
So there.

> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index f055bfd54a68..0e48cb6d21d6 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -170,7 +170,7 @@ scsi_mod-$(CONFIG_BLK_DEV_BSG)	+= scsi_bsg.o
>   
>   hv_storvsc-y			:= storvsc_drv.o
>   
> -sd_mod-objs	:= sd.o
> +sd_mod-objs	:= sd.o sd_cdl.o
>   sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
>   sd_mod-$(CONFIG_BLK_DEV_ZONED) += sd_zbc.o
>   
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 45945bfeee92..7879a5470773 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3326,6 +3326,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   		sd_read_write_same(sdkp, buffer);
>   		sd_read_security(sdkp, buffer);
>   		sd_config_protection(sdkp);
> +		sd_read_cdl(sdkp, buffer);
>   	}
>   
>   	/*
> @@ -3646,6 +3647,7 @@ static void scsi_disk_release(struct device *dev)
>   
>   	ida_free(&sd_index_ida, sdkp->index);
>   	sd_zbc_free_zone_info(sdkp);
> +	sd_cdl_release(sdkp);
>   	put_device(&sdkp->device->sdev_gendev);
>   	free_opal_dev(sdkp->opal_dev);
>   
Hmm. Calling this during revalidate() makes sense, but how can we ensure 
that we call revalidate() when the user issues a MODE_SELECT command?

Other than that:

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

Cheers,

Hannes