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

Hannes Reinecke
2023-01-27T13:00:46Z

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