On 2021/07/23 1:14, Hannes Reinecke wrote:
> On 7/21/21 12:42 PM, Damien Le Moal wrote:
>> Add the sd_read_cpr() function to the sd scsi disk driver to discover
>> if a device has multiple concurrent positioning ranges (i.e. multiple
>> actuators on an HDD). This new function is called from
>> sd_revalidate_disk() and uses the block layer functions
>> blk_alloc_cranges() and blk_queue_set_cranges() to set a device
>> cranges according to the information retrieved from log page B9h,
>> if the device supports it.
>>
>> The format of the Concurrent Positioning Ranges VPD page B9h is defined
>> in section 6.6.6 of SBC-5.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
>> ---
>> drivers/scsi/sd.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/scsi/sd.h | 1 +
>> 2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index b8d55af763f9..b1e767a01b9f 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -3125,6 +3125,85 @@ static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
>> sdkp->security = 1;
>> }
>>
>> +static inline sector_t sd64_to_sectors(struct scsi_disk *sdkp, u8 *buf)
>> +{
>> + return logical_to_sectors(sdkp->device, get_unaligned_be64(buf));
>> +}
>> +
>> +/**
>> + * sd_read_cpr - Query concurrent positioning ranges
>> + * @sdkp: disk to query
>> + */
>> +static void sd_read_cpr(struct scsi_disk *sdkp)
>> +{
>> + unsigned char *buffer, *desc;
>> + struct blk_cranges *cr = NULL;
>> + unsigned int nr_cpr = 0;
>> + int i, vpd_len, buf_len = SD_BUF_SIZE;
>> +
>> + /*
>> + * We need to have the capacity set first for the block layer to be
>> + * able to check the ranges.
>> + */
>> + if (sdkp->first_scan)
>> + return;
>> +
>> + if (!sdkp->capacity)
>> + goto out;
>> +
>> + /*
>> + * Concurrent Positioning Ranges VPD: there can be at most 256 ranges,
>> + * leading to a maximum page size of 64 + 256*32 bytes.
>> + */
>> + buf_len = 64 + 256*32;
>> + buffer = kmalloc(buf_len, GFP_KERNEL);
>> + if (!buffer || scsi_get_vpd_page(sdkp->device, 0xb9, buffer, buf_len))
>> + goto out;
>> +
>> + /* We must have at least a 64B header and one 32B range descriptor */
>> + vpd_len = get_unaligned_be16(&buffer[2]) + 3;
>> + if (vpd_len > buf_len || vpd_len < 64 + 32 || (vpd_len & 31)) {
>> + sd_printk(KERN_ERR, sdkp,
>> + "Invalid Concurrent Positioning Ranges VPD page\n");
>> + goto out;
>> + }
>> +
>> + nr_cpr = (vpd_len - 64) / 32;
>> + if (nr_cpr == 1) {
>> + nr_cpr = 0;
>> + goto out;
>> + }
>> +
>> + cr = blk_alloc_cranges(sdkp->disk, nr_cpr);
>> + if (!cr) {
>> + nr_cpr = 0;
>> + goto out;
>> + }
>> +
>> + desc = &buffer[64];
>> + for (i = 0; i < nr_cpr; i++, desc += 32) {
>> + if (desc[0] != i) {
>> + sd_printk(KERN_ERR, sdkp,
>> + "Invalid Concurrent Positioning Range number\n");
>> + nr_cpr = 0;
>> + break;
>> + }
>> +
>> + cr->ranges[i].sector = sd64_to_sectors(sdkp, desc + 8);
>> + cr->ranges[i].nr_sectors = sd64_to_sectors(sdkp, desc + 16);
>> + }
>> +
>> +out:
>> + blk_queue_set_cranges(sdkp->disk, cr);
>
> See? We are _are_ creating a new set of ranges.
> So why bother updating the old ones?
See my reply to my comments on patch 1. We do not update the old ones. The cases
are:
1) This is the first call, q->cranges is NULL: cr will become q->cranges.
2) q->cranges is already set and cr has the same info as q->cranges (no change):
cr will be freed by blk_queue_set_cranges().
3) q->cranges is already set and cr is different from q-cranges: q->cranges will
be completely dropped and cr become the new q->cranges.
So we never directly update anything inside q->cranges. We just swap in a new
full structure in case of change.
>> + if (nr_cpr && sdkp->nr_actuators != nr_cpr) {
>> + sd_printk(KERN_NOTICE, sdkp,
>> + "%u concurrent positioning ranges\n", nr_cpr);
>> + sdkp->nr_actuators = nr_cpr;
>> + }
>> +
>> + kfree(buffer);
>> +}
>> +
>> /*
>> * Determine the device's preferred I/O size for reads and writes
>> * unless the reported value is unreasonably small, large, not a
>> @@ -3240,6 +3319,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>> sd_read_app_tag_own(sdkp, buffer);
>> sd_read_write_same(sdkp, buffer);
>> sd_read_security(sdkp, buffer);
>> + sd_read_cpr(sdkp);
>> }
>>
>> /*
>> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
>> index b59136c4125b..2e5932bde43d 100644
>> --- a/drivers/scsi/sd.h
>> +++ b/drivers/scsi/sd.h
>> @@ -106,6 +106,7 @@ struct scsi_disk {
>> u8 protection_type;/* Data Integrity Field */
>> u8 provisioning_mode;
>> u8 zeroing_mode;
>> + u8 nr_actuators; /* Number of actuators */
>> unsigned ATO : 1; /* state of disk ATO bit */
>> unsigned cache_override : 1; /* temp override of WCE,RCD */
>> unsigned WCE : 1; /* state of disk WCE bit */
>>
> Otherwise:
>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
>
> Cheers,
>
> Hannes
>
--
Damien Le Moal
Western Digital Research
