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)
>
--
Damien Le Moal
Western Digital Research
