On 1/27/23 22:00, Hannes Reinecke wrote: > On 1/24/23 20:02, Niklas Cassel wrote: >> From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxxxxx> >> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx> >> --- >> 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? Given that CDLs can be changed with a passthrough command, I do not think we can do anything about that, unfortunately. But I think the same is true of many things like that. E.g. "let's turn onf/off the write cache without the kernel noticing"... But given that on a normal system only privileged applications can do passthrough, if that happens, then the system has been hacked or the user is shooting himself in the foot. cdl-tools project (cdladm utility) uses passtrhough but triggers a revalidate after changing CDLs to make sure sysfs stays in sync. As Christoph suggested, we could change all this to an ioctl(GET_CDL) for applications... But sysfs is so much simpler in my opinion, not to mention that it allows access to the information for any application written in a language that does not have ioctl() or an equivalent. cdl-tools has a test suite all written in bash scripts thanks to the sysfs interface :) > > Other than that: > > Reviewed-by: Hannes Reinecke <hare@xxxxxxx> > > Cheers, > > Hannes -- Damien Le Moal Western Digital Research
