On 7/16/22 05:53, Bart Van Assche wrote:
> On 7/12/22 15:08, Damien Le Moal wrote:
>> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
>> index 6acc4f406eb8..32da54e7b68a 100644
>> --- a/drivers/scsi/sd_zbc.c
>> +++ b/drivers/scsi/sd_zbc.c
>> @@ -716,17 +716,15 @@ static int sd_zbc_check_capacity(struct scsi_disk
>> *sdkp, unsigned char *buf,
>> if (ret)
>> return ret;
>>
>> - if (sdkp->rc_basis == 0) {
>> - /* The max_lba field is the capacity of this device */
>> - max_lba = get_unaligned_be64(&buf[8]);
>> - if (sdkp->capacity != max_lba + 1) {
>> - if (sdkp->first_scan)
>> - sd_printk(KERN_WARNING, sdkp,
>> - "Changing capacity from %llu to
>> max LBA+1 %llu\n",
>> - (unsigned long long)sdkp->capacity,
>> - (unsigned long long)max_lba + 1);
>> - sdkp->capacity = max_lba + 1;
>> - }
>> + /* The max_lba field is the capacity of this device */
>> + max_lba = get_unaligned_be64(&buf[8]);
>> + if (sdkp->capacity != max_lba + 1) {
>> + if (sdkp->first_scan)
>> + sd_printk(KERN_WARNING, sdkp,
>> + "Changing capacity from %llu to max LBA+1 %llu\n",
>> + (unsigned long long)sdkp->capacity,
>> + (unsigned long long)max_lba + 1);
>> + sdkp->capacity = max_lba + 1;
>> }
>>
>> if (sdkp->zone_starting_lba_gran == 0) {
>>
>> That is, always check the reported capacity against max_lba of report
>> zones reply, regardless of rc_basis (and we can even then drop the
>> rc_basis field from struct scsi_disk).
>
> I like the above patch because it simplifies the existing code.
>
>> But I would argue that any problem with the current code would mean a
>> buggy device firmware. For such case, the device FW should be fixed or we
>> should add a quirk for it.
>
> My question was which approach should be followed for devices with a
> buggy firmware? Use the zones up to the LBA reported in the READ
> CAPACITY response or reject these devices entirely?
We should not try to write code for buggy devices. These should be handled
with quirks, or even better, ignored to give incentives to the device
vendor to fix their bugs.
Even the above code (removing the "sdkp->rc_basis == 0" test) is
borderline in my opinion. The code with the test is per specs, so correct.
If you really insist on changing the code as above, we should add
something like this under the "if (sdkp->capacity != max_lba + 1) {":
if (sdkp->rc_basis != 0)
sd_printk(KERN_WARNING, sdkp,
"ZBC device reported an invalid capacity\n");
To signal that the drive is wrong.
>
> Thanks,
>
> Bart.
--
Damien Le Moal
Western Digital Research
