| Message ID | 20240229172333.2494378-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi/sd_zbc: Use READ(10)/WRITE(10) for zoned UFS devices | expand |
On 2024/02/29 9:23, Bart Van Assche wrote: > READ(10) and WRITE(10) are sufficient for zoned UFS devices. UFS device > manufacturers prefer to minimize the size of their firmware and hence > also the number of SCSI commands that are supported. Hence this patch > that switches from READ(16)/WRITE(16)/SYNCHRONIZE CACHE(16) to READ(10)/ > WRITE(10)/SYNCHRONIZE CACHE(10) for zoned UFS devices. The 16-byte > commands are still used for zoned devices with more than 2**32 logical > blocks because of the following code in sd_read_capacity(): > > if (sdkp->capacity > 0xffffffff) > sdp->use_16_for_rw = 1; > > Cc: Damien Le Moal <dlemoal@kernel.org> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ata/libata-scsi.c | 10 ++++++++-- > drivers/scsi/sd_zbc.c | 5 ----- > 2 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 0a0f483124c3..a196dce4bbc3 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -986,8 +986,14 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > > void ata_scsi_sdev_config(struct scsi_device *sdev) > { > - sdev->use_10_for_rw = 1; > - sdev->use_10_for_ms = 1; > + if (sdev->type == TYPE_ZBC) { > + /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ > + sdev->use_16_for_rw = 1; > + sdev->use_16_for_sync = 1; scsi_add_lun() sets use_10_for_rw to "1" so can we clear it to "0" here again like was done in the sd_zbc.c hunk below that you removed ? > + } else { > + sdev->use_10_for_rw = 1; > + sdev->use_10_for_ms = 1; > + } > sdev->no_write_same = 1; > > /* Schedule policy is determined by ->qc_defer() callback and > diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c > index 26af5ab7d7c1..bcdb21706d3f 100644 > --- a/drivers/scsi/sd_zbc.c > +++ b/drivers/scsi/sd_zbc.c > @@ -924,11 +924,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]) > return 0; > } > > - /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ > - sdkp->device->use_16_for_rw = 1; > - sdkp->device->use_10_for_rw = 0; > - sdkp->device->use_16_for_sync = 1; > - > /* Check zoned block device characteristics (unconstrained reads) */ > ret = sd_zbc_check_zoned_characteristics(sdkp, buf); > if (ret)
On 2/29/24 09:29, Damien Le Moal wrote: > On 2024/02/29 9:23, Bart Van Assche wrote: >> + if (sdev->type == TYPE_ZBC) { >> + /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ >> + sdev->use_16_for_rw = 1; >> + sdev->use_16_for_sync = 1; > > scsi_add_lun() sets use_10_for_rw to "1" so can we clear it to "0" here again > like was done in the sd_zbc.c hunk below that you removed ? Hi Damien, Although it would be easy to make that change, will it cause any difference in behavior? sd_setup_read_write_cmnd() checks the value of .use_16_for_rw before it checks the value of .use_10_for_rw. Hence, the value of .use_10_for_rw does not matter if .use_16_for_rw is set. Thanks, Bart.
On 2024/02/29 10:05, Bart Van Assche wrote: > On 2/29/24 09:29, Damien Le Moal wrote: >> On 2024/02/29 9:23, Bart Van Assche wrote: >>> + if (sdev->type == TYPE_ZBC) { >>> + /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ >>> + sdev->use_16_for_rw = 1; >>> + sdev->use_16_for_sync = 1; >> >> scsi_add_lun() sets use_10_for_rw to "1" so can we clear it to "0" here again >> like was done in the sd_zbc.c hunk below that you removed ? > Hi Damien, > > Although it would be easy to make that change, will it cause any > difference in behavior? sd_setup_read_write_cmnd() checks the value of > .use_16_for_rw before it checks the value of .use_10_for_rw. Hence, the > value of .use_10_for_rw does not matter if .use_16_for_rw is set. Yes, but I find that a little fragile and given that rw-10 causes problems with ZBC, I prefer to make it very explicit that the 10B command variants should not be used.
On 2/29/24 10:31, Damien Le Moal wrote: > Yes, but I find that a little fragile and given that rw-10 causes problems with > ZBC, I prefer to make it very explicit that the 10B command variants should not > be used. Hi Damien, From commit c6463c651d7a ("sd_zbc: Force use of READ16/WRITE16"; v4.10): ------------------------------------------------------------------------- sd_zbc: Force use of READ16/WRITE16 Normally, sd_read_capacity sets sdp->use_16_for_rw to 1 based on the disk capacity so that READ16/WRITE16 are used for large drives. However, for a zoned disk with RC_BASIS set to 0, the capacity reported through READ_CAPACITY may be very small, leading to use_16_for_rw not being set and READ10/WRITE10 commands being used, even after the actual zoned disk capacity is corrected in sd_zbc_read_zones. This causes LBA offset overflow for accesses beyond 2TB. As the ZBC standard makes it mandatory for ZBC drives to support the READ16/WRITE16 commands anyway, make sure that use_16_for_rw is set. ------------------------------------------------------------------------- Would this change be sufficient to fix the problems mentioned above? diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 997de4daa8c4..71f477e502e9 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1279,7 +1279,7 @@ 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, dld); - } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff)) { + } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff) || (lba >> 32)) { ret = sd_setup_rw16_cmnd(cmd, write, lba, nr_blocks, protect | fua, dld); } else if ((nr_blocks > 0xff) || (lba > 0x1fffff) || Thanks, Bart.
On 2024/02/29 10:54, Bart Van Assche wrote: > On 2/29/24 10:31, Damien Le Moal wrote: >> Yes, but I find that a little fragile and given that rw-10 causes problems with >> ZBC, I prefer to make it very explicit that the 10B command variants should not >> be used. > > Hi Damien, > > From commit c6463c651d7a ("sd_zbc: Force use of READ16/WRITE16"; v4.10): > > ------------------------------------------------------------------------- > sd_zbc: Force use of READ16/WRITE16 > > Normally, sd_read_capacity sets sdp->use_16_for_rw to 1 based on the > disk capacity so that READ16/WRITE16 are used for large drives. However, > for a zoned disk with RC_BASIS set to 0, the capacity reported through > READ_CAPACITY may be very small, leading to use_16_for_rw not being > set and READ10/WRITE10 commands being used, even after the actual zoned > disk capacity is corrected in sd_zbc_read_zones. This causes LBA offset > overflow for accesses beyond 2TB. > > As the ZBC standard makes it mandatory for ZBC drives to support > the READ16/WRITE16 commands anyway, make sure that use_16_for_rw is set. > ------------------------------------------------------------------------- > > Would this change be sufficient to fix the problems mentioned above? > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 997de4daa8c4..71f477e502e9 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1279,7 +1279,7 @@ 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, dld); > - } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff)) { > + } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff) || (lba >> 32)) { Sure, that works too, but seems useless given that we do have use_16_for_rw set. Would clearing use_10_for_rw to 0 cause a problem for zoned UFS drives ? > ret = sd_setup_rw16_cmnd(cmd, write, lba, nr_blocks, > protect | fua, dld); > } else if ((nr_blocks > 0xff) || (lba > 0x1fffff) || > > Thanks, > > Bart.
On 2/29/24 11:58, Damien Le Moal wrote: > On 2024/02/29 10:54, Bart Van Assche wrote: >> On 2/29/24 10:31, Damien Le Moal wrote: >>> Yes, but I find that a little fragile and given that rw-10 causes problems with >>> ZBC, I prefer to make it very explicit that the 10B command variants should not >>> be used. >> >> Hi Damien, >> >> From commit c6463c651d7a ("sd_zbc: Force use of READ16/WRITE16"; v4.10): >> >> ------------------------------------------------------------------------- >> sd_zbc: Force use of READ16/WRITE16 >> >> Normally, sd_read_capacity sets sdp->use_16_for_rw to 1 based on the >> disk capacity so that READ16/WRITE16 are used for large drives. However, >> for a zoned disk with RC_BASIS set to 0, the capacity reported through >> READ_CAPACITY may be very small, leading to use_16_for_rw not being >> set and READ10/WRITE10 commands being used, even after the actual zoned >> disk capacity is corrected in sd_zbc_read_zones. This causes LBA offset >> overflow for accesses beyond 2TB. >> >> As the ZBC standard makes it mandatory for ZBC drives to support >> the READ16/WRITE16 commands anyway, make sure that use_16_for_rw is set. >> ------------------------------------------------------------------------- >> >> Would this change be sufficient to fix the problems mentioned above? >> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c >> index 997de4daa8c4..71f477e502e9 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -1279,7 +1279,7 @@ 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, dld); >> - } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff)) { >> + } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff) || (lba >> 32)) { > > Sure, that works too, but seems useless given that we do have use_16_for_rw set. > Would clearing use_10_for_rw to 0 cause a problem for zoned UFS drives ? The patch at the start of this email thread should be sufficient. I shared the above change because I wanted to make sure that I understand why 16-byte commands need to be selected explicitly for zoned hard disks. I plan to post a second version of that patch with the use_10_for_rw = 0 assignment restored. Thanks, Bart.
On 2/29/24 12:05, Bart Van Assche wrote:
> [ ... ]
Thinking further about this, wouldn't this be a better solution
(untested)?
Thanks,
Bart.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 997de4daa8c4..8d3020486095 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2798,6 +2798,13 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned
char *buffer)
sdkp->physical_block_size);
sdkp->device->sector_size = sector_size;
+ /*
+ * For zoned block devices, if RC BASIS = 1 in the READ CAPACITY
+ * response, sdkp->capacity does not represent the device capacity but
+ * the highest LBA of the conventional zones at the start of the LBA
+ * space. sd_zbc_check_capacity() sets sdkp->capacity correctly for
+ * zoned devices.
+ */
if (sdkp->capacity > 0xffffffff)
sdp->use_16_for_rw = 1;
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 26af5ab7d7c1..d4d6b3056410 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk
*sdkp, unsigned char *buf,
(unsigned long long)sdkp->capacity,
(unsigned long long)max_lba + 1);
sdkp->capacity = max_lba + 1;
+ if (sdkp->capacity > 0xffffffff)
+ sdkp->device->use_16_for_rw = 1;
}
}
@@ -924,11 +926,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8
buf[SD_BUF_SIZE])
return 0;
}
- /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */
- sdkp->device->use_16_for_rw = 1;
- sdkp->device->use_10_for_rw = 0;
- sdkp->device->use_16_for_sync = 1;
-
/* Check zoned block device characteristics (unconstrained reads) */
ret = sd_zbc_check_zoned_characteristics(sdkp, buf);
if (ret)
On 2024/02/29 14:07, Bart Van Assche wrote: > On 2/29/24 12:05, Bart Van Assche wrote: >> [ ... ] > > Thinking further about this, wouldn't this be a better solution > (untested)? > > Thanks, > > Bart. > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 997de4daa8c4..8d3020486095 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2798,6 +2798,13 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned > char *buffer) > sdkp->physical_block_size); > sdkp->device->sector_size = sector_size; > > + /* > + * For zoned block devices, if RC BASIS = 1 in the READ CAPACITY > + * response, sdkp->capacity does not represent the device capacity but > + * the highest LBA of the conventional zones at the start of the LBA > + * space. sd_zbc_check_capacity() sets sdkp->capacity correctly for > + * zoned devices. > + */ > if (sdkp->capacity > 0xffffffff) > sdp->use_16_for_rw = 1; > > diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c > index 26af5ab7d7c1..d4d6b3056410 100644 > --- a/drivers/scsi/sd_zbc.c > +++ b/drivers/scsi/sd_zbc.c > @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk > *sdkp, unsigned char *buf, > (unsigned long long)sdkp->capacity, > (unsigned long long)max_lba + 1); > sdkp->capacity = max_lba + 1; > + if (sdkp->capacity > 0xffffffff) > + sdkp->device->use_16_for_rw = 1; ZBC makes 16B RW mandatory, regardless of the device capacity. So I am not very keen on playing games like this and think that it is safer to always use RW-16, regardless of the device capacity. I prefer your first patch, with the added "use_10_for_rw = 0;" added. > } > } > > @@ -924,11 +926,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 > buf[SD_BUF_SIZE]) > return 0; > } > > - /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ > - sdkp->device->use_16_for_rw = 1; > - sdkp->device->use_10_for_rw = 0; > - sdkp->device->use_16_for_sync = 1; > - > /* Check zoned block device characteristics (unconstrained reads) */ > ret = sd_zbc_check_zoned_characteristics(sdkp, buf); > if (ret) >
On 2/29/24 14:16, Damien Le Moal wrote: > On 2024/02/29 14:07, Bart Van Assche wrote: >> --- a/drivers/scsi/sd_zbc.c >> +++ b/drivers/scsi/sd_zbc.c >> @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk >> *sdkp, unsigned char *buf, >> (unsigned long long)sdkp->capacity, >> (unsigned long long)max_lba + 1); >> sdkp->capacity = max_lba + 1; >> + if (sdkp->capacity > 0xffffffff) >> + sdkp->device->use_16_for_rw = 1; > > ZBC makes 16B RW mandatory, regardless of the device capacity. So I am not very > keen on playing games like this and think that it is safer to always use RW-16, > regardless of the device capacity. I prefer your first patch, with the added > "use_10_for_rw = 0;" added. Hi Damien, I think that we need to combine the two patches otherwise we risk breaking support for zoned devices that are neither ATA devices nor UFS devices. How about the patch below? Thanks, Bart. diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 0a0f483124c3..6d55a01f0263 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -986,8 +986,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) void ata_scsi_sdev_config(struct scsi_device *sdev) { - sdev->use_10_for_rw = 1; - sdev->use_10_for_ms = 1; + if (sdev->type == TYPE_ZBC) { + /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ + sdev->use_16_for_rw = 1; + sdev->use_10_for_rw = 0; + sdev->use_16_for_sync = 1; + } else { + sdev->use_10_for_rw = 1; + sdev->use_10_for_ms = 1; + } sdev->no_write_same = 1; /* Schedule policy is determined by ->qc_defer() callback and diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 997de4daa8c4..8d3020486095 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2798,6 +2798,13 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer) sdkp->physical_block_size); sdkp->device->sector_size = sector_size; + /* + * For zoned block devices, if RC BASIS = 1 in the READ CAPACITY + * response, sdkp->capacity does not represent the device capacity but + * the highest LBA of the conventional zones at the start of the LBA + * space. sd_zbc_check_capacity() sets sdkp->capacity correctly for + * zoned devices. + */ if (sdkp->capacity > 0xffffffff) sdp->use_16_for_rw = 1; diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index 26af5ab7d7c1..d4d6b3056410 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf, (unsigned long long)sdkp->capacity, (unsigned long long)max_lba + 1); sdkp->capacity = max_lba + 1; + if (sdkp->capacity > 0xffffffff) + sdkp->device->use_16_for_rw = 1; } } @@ -924,11 +926,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]) return 0; } - /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ - sdkp->device->use_16_for_rw = 1; - sdkp->device->use_10_for_rw = 0; - sdkp->device->use_16_for_sync = 1; - /* Check zoned block device characteristics (unconstrained reads) */ ret = sd_zbc_check_zoned_characteristics(sdkp, buf); if (ret)
On 2024/02/29 14:39, Bart Van Assche wrote: > On 2/29/24 14:16, Damien Le Moal wrote: >> On 2024/02/29 14:07, Bart Van Assche wrote: >>> --- a/drivers/scsi/sd_zbc.c >>> +++ b/drivers/scsi/sd_zbc.c >>> @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk >>> *sdkp, unsigned char *buf, >>> (unsigned long long)sdkp->capacity, >>> (unsigned long long)max_lba + 1); >>> sdkp->capacity = max_lba + 1; >>> + if (sdkp->capacity > 0xffffffff) >>> + sdkp->device->use_16_for_rw = 1; >> >> ZBC makes 16B RW mandatory, regardless of the device capacity. So I am not very >> keen on playing games like this and think that it is safer to always use RW-16, >> regardless of the device capacity. I prefer your first patch, with the added >> "use_10_for_rw = 0;" added. > > Hi Damien, > > I think that we need to combine the two patches otherwise we risk > breaking support for zoned devices that are neither ATA devices nor > UFS devices. How about the patch below? > > Thanks, > > Bart. > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 0a0f483124c3..6d55a01f0263 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -986,8 +986,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd > *qc) > > void ata_scsi_sdev_config(struct scsi_device *sdev) > { > - sdev->use_10_for_rw = 1; > - sdev->use_10_for_ms = 1; > + if (sdev->type == TYPE_ZBC) { > + /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ > + sdev->use_16_for_rw = 1; > + sdev->use_10_for_rw = 0; > + sdev->use_16_for_sync = 1; > + } else { > + sdev->use_10_for_rw = 1; > + sdev->use_10_for_ms = 1; > + } > sdev->no_write_same = 1; > > /* Schedule policy is determined by ->qc_defer() callback and > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 997de4daa8c4..8d3020486095 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2798,6 +2798,13 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned > char *buffer) > sdkp->physical_block_size); > sdkp->device->sector_size = sector_size; > > + /* > + * For zoned block devices, if RC BASIS = 1 in the READ CAPACITY > + * response, sdkp->capacity does not represent the device capacity but > + * the highest LBA of the conventional zones at the start of the LBA > + * space. sd_zbc_check_capacity() sets sdkp->capacity correctly for > + * zoned devices. > + */ > if (sdkp->capacity > 0xffffffff) > sdp->use_16_for_rw = 1; > > diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c > index 26af5ab7d7c1..d4d6b3056410 100644 > --- a/drivers/scsi/sd_zbc.c > +++ b/drivers/scsi/sd_zbc.c > @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk > *sdkp, unsigned char *buf, > (unsigned long long)sdkp->capacity, > (unsigned long long)max_lba + 1); > sdkp->capacity = max_lba + 1; > + if (sdkp->capacity > 0xffffffff) > + sdkp->device->use_16_for_rw = 1; While correct, I do not think that this change is needed. With the first hunk in place, a TYPE_ZBC device will be forced to use RW-16, regardless of the device capacity. > } > } > > @@ -924,11 +926,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 > buf[SD_BUF_SIZE]) > return 0; > } > > - /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ > - sdkp->device->use_16_for_rw = 1; > - sdkp->device->use_10_for_rw = 0; > - sdkp->device->use_16_for_sync = 1; > - > /* Check zoned block device characteristics (unconstrained reads) */ > ret = sd_zbc_check_zoned_characteristics(sdkp, buf); > if (ret) >
On 2/29/24 14:45, Damien Le Moal wrote: > On 2024/02/29 14:39, Bart Van Assche wrote: >> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >> index 26af5ab7d7c1..d4d6b3056410 100644 >> --- a/drivers/scsi/sd_zbc.c >> +++ b/drivers/scsi/sd_zbc.c >> @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk >> *sdkp, unsigned char *buf, >> (unsigned long long)sdkp->capacity, >> (unsigned long long)max_lba + 1); >> sdkp->capacity = max_lba + 1; >> + if (sdkp->capacity > 0xffffffff) >> + sdkp->device->use_16_for_rw = 1; > > While correct, I do not think that this change is needed. With the first hunk in > place, a TYPE_ZBC device will be forced to use RW-16, regardless of the device > capacity. The first hunk of this patch is for ATA devices only. I want to make sure that .use_16_for_rw is set for non-ATA devices if there are LBAs that need more than 32 bits. Thanks, Bart.
On 2024/02/29 15:25, Bart Van Assche wrote: > On 2/29/24 14:45, Damien Le Moal wrote: >> On 2024/02/29 14:39, Bart Van Assche wrote: >>> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >>> index 26af5ab7d7c1..d4d6b3056410 100644 >>> --- a/drivers/scsi/sd_zbc.c >>> +++ b/drivers/scsi/sd_zbc.c >>> @@ -734,6 +734,8 @@ static int sd_zbc_check_capacity(struct scsi_disk >>> *sdkp, unsigned char *buf, >>> (unsigned long long)sdkp->capacity, >>> (unsigned long long)max_lba + 1); >>> sdkp->capacity = max_lba + 1; >>> + if (sdkp->capacity > 0xffffffff) >>> + sdkp->device->use_16_for_rw = 1; >> >> While correct, I do not think that this change is needed. With the first hunk in >> place, a TYPE_ZBC device will be forced to use RW-16, regardless of the device >> capacity. > > The first hunk of this patch is for ATA devices only. I want to make sure that > .use_16_for_rw is set for non-ATA devices if there are LBAs that need more than > 32 bits. I missed that ! OK. So, you want to keep RW-16 forced for HDDs, but only have RW-10 for zoned UFS devices, is this correct ? If it is, then question: what is the device type reported by zoned UFS drives ? If it is TYPE_ZBC, then we should follow SBC specs and thus have RW-16 mandatory. Is UFS, again, not following completely the scsi specifications ? I would really prefer to not spread this setting between scsi sd and libata-scsi. I also do not like that your change for the scsi side depends on the capacity instead on the device type. Small capacity ZBC drives can be created with scsi_debug or tcmu-runner zbc handler, and these should use RW-16 as per the specifications too. > > Thanks, > > Bart.
On Thu, Feb 29, 2024 at 09:23:32AM -0800, Bart Van Assche wrote: > READ(10) and WRITE(10) are sufficient for zoned UFS devices. UFS device > manufacturers prefer to minimize the size of their firmware and hence But ZBS rquires the 16byte formats. It UFS vendors keep ignoring SCSI they should not claim to implement it.
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 0a0f483124c3..a196dce4bbc3 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -986,8 +986,14 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) void ata_scsi_sdev_config(struct scsi_device *sdev) { - sdev->use_10_for_rw = 1; - sdev->use_10_for_ms = 1; + if (sdev->type == TYPE_ZBC) { + /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ + sdev->use_16_for_rw = 1; + sdev->use_16_for_sync = 1; + } else { + sdev->use_10_for_rw = 1; + sdev->use_10_for_ms = 1; + } sdev->no_write_same = 1; /* Schedule policy is determined by ->qc_defer() callback and diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index 26af5ab7d7c1..bcdb21706d3f 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -924,11 +924,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]) return 0; } - /* READ16/WRITE16/SYNC16 is mandatory for ZBC devices */ - sdkp->device->use_16_for_rw = 1; - sdkp->device->use_10_for_rw = 0; - sdkp->device->use_16_for_sync = 1; - /* Check zoned block device characteristics (unconstrained reads) */ ret = sd_zbc_check_zoned_characteristics(sdkp, buf); if (ret)
READ(10) and WRITE(10) are sufficient for zoned UFS devices. UFS device manufacturers prefer to minimize the size of their firmware and hence also the number of SCSI commands that are supported. Hence this patch that switches from READ(16)/WRITE(16)/SYNCHRONIZE CACHE(16) to READ(10)/ WRITE(10)/SYNCHRONIZE CACHE(10) for zoned UFS devices. The 16-byte commands are still used for zoned devices with more than 2**32 logical blocks because of the following code in sd_read_capacity(): if (sdkp->capacity > 0xffffffff) sdp->use_16_for_rw = 1; Cc: Damien Le Moal <dlemoal@kernel.org> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ata/libata-scsi.c | 10 ++++++++-- drivers/scsi/sd_zbc.c | 5 ----- 2 files changed, 8 insertions(+), 7 deletions(-)