| Message ID | 160763248354.26927.303366932249508542.stgit@brunhilda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | smartpqi updates | expand |
On Thu, 2020-12-10 at 14:34 -0600, Don Brace wrote: > * Add in new IU definition. > * Add in support raid5 and raid6 writes. > > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Reviewed-by: Scott Teel <scott.teel@microchip.com> > Reviewed-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi.h | 39 +++++ > drivers/scsi/smartpqi/smartpqi_init.c | 247 > ++++++++++++++++++++++++++++++++- > 2 files changed, 278 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/smartpqi/smartpqi.h > b/drivers/scsi/smartpqi/smartpqi.h > index d486a2ec3045..e9844210c4a0 100644 > --- a/drivers/scsi/smartpqi/smartpqi.h > +++ b/drivers/scsi/smartpqi/smartpqi.h > @@ -257,6 +257,7 @@ struct pqi_device_capability { > }; > > #define PQI_MAX_EMBEDDED_SG_DESCRIPTORS 4 > +#define PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS 3 > > struct pqi_raid_path_request { > struct pqi_iu_header header; > @@ -312,6 +313,39 @@ struct pqi_aio_path_request { > sg_descriptors[PQI_MAX_EMBEDDED_SG_DESCRIPTORS]; > }; > > +#define PQI_RAID56_XFER_LIMIT_4K 0x1000 /* 4Kib */ > +#define PQI_RAID56_XFER_LIMIT_8K 0x2000 /* 8Kib */ You don't seem to use these, and you'll remove them again in patch 06/25. > +struct pqi_aio_r56_path_request { > + struct pqi_iu_header header; > + __le16 request_id; > + __le16 volume_id; /* ID of the RAID volume */ > + __le32 data_it_nexus; /* IT nexus for the data > drive */ > + __le32 p_parity_it_nexus; /* IT nexus for the P parity > drive */ > + __le32 q_parity_it_nexus; /* IT nexus for the Q parity > drive */ > + __le32 data_length; /* total bytes to read/write > */ > + u8 data_direction : 2; > + u8 partial : 1; > + u8 mem_type : 1; /* 0b: PCIe, 1b: DDR */ > + u8 fence : 1; > + u8 encryption_enable : 1; > + u8 reserved : 2; > + u8 task_attribute : 3; > + u8 command_priority : 4; > + u8 reserved1 : 1; > + __le16 data_encryption_key_index; > + u8 cdb[16]; > + __le16 error_index; > + u8 num_sg_descriptors; > + u8 cdb_length; > + u8 xor_multiplier; > + u8 reserved2[3]; > + __le32 encrypt_tweak_lower; > + __le32 encrypt_tweak_upper; > + u8 row; /* row = logical lba/blocks > per row */ > + u8 reserved3[8]; > + struct pqi_sg_descriptor > sg_descriptors[PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS]; > +}; > + > struct pqi_io_response { > struct pqi_iu_header header; > __le16 request_id; > @@ -484,6 +518,8 @@ struct pqi_raid_error_info { > #define PQI_REQUEST_IU_TASK_MANAGEMENT 0x13 > #define PQI_REQUEST_IU_RAID_PATH_IO 0x14 > #define PQI_REQUEST_IU_AIO_PATH_IO 0x15 > +#define PQI_REQUEST_IU_AIO_PATH_RAID5_IO 0x18 > +#define PQI_REQUEST_IU_AIO_PATH_RAID6_IO 0x19 > #define PQI_REQUEST_IU_GENERAL_ADMIN 0x60 > #define PQI_REQUEST_IU_REPORT_VENDOR_EVENT_CONFIG 0x72 > #define PQI_REQUEST_IU_SET_VENDOR_EVENT_CONFIG 0x73 > @@ -1179,6 +1215,7 @@ struct pqi_ctrl_info { > u16 max_inbound_iu_length_per_firmware; > u16 max_inbound_iu_length; > unsigned int max_sg_per_iu; > + unsigned int max_sg_per_r56_iu; > void *admin_queue_memory_base; > u32 admin_queue_memory_length; > dma_addr_t admin_queue_memory_base_dma_handle; > @@ -1210,6 +1247,8 @@ struct pqi_ctrl_info { > u8 soft_reset_handshake_supported : 1; > u8 raid_iu_timeout_supported: 1; > u8 tmf_iu_timeout_supported: 1; > + u8 enable_r5_writes : 1; > + u8 enable_r6_writes : 1; > > struct list_head scsi_device_list; > spinlock_t scsi_device_list_lock; > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index 6bcb037ae9d7..c813cec10003 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -67,6 +67,10 @@ static int pqi_aio_submit_io(struct pqi_ctrl_info > *ctrl_info, > struct scsi_cmnd *scmd, u32 aio_handle, u8 *cdb, > unsigned int cdb_length, struct pqi_queue_group *queue_group, > struct pqi_encryption_info *encryption_info, bool > raid_bypass); > +static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info > *ctrl_info, > + struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group, > + struct pqi_encryption_info *encryption_info, struct > pqi_scsi_dev *device, > + struct pqi_scsi_dev_raid_map_data *rmd); > static void pqi_ofa_ctrl_quiesce(struct pqi_ctrl_info *ctrl_info); > static void pqi_ofa_ctrl_unquiesce(struct pqi_ctrl_info *ctrl_info); > static int pqi_ofa_ctrl_restart(struct pqi_ctrl_info *ctrl_info); > @@ -2237,7 +2241,8 @@ static inline void pqi_set_encryption_info( > * Attempt to perform RAID bypass mapping for a logical volume I/O. > */ > > -static bool pqi_aio_raid_level_supported(struct > pqi_scsi_dev_raid_map_data *rmd) > +static bool pqi_aio_raid_level_supported(struct pqi_ctrl_info > *ctrl_info, > + struct pqi_scsi_dev_raid_map_data *rmd) > { > bool is_supported = true; > > @@ -2245,13 +2250,14 @@ static bool > pqi_aio_raid_level_supported(struct pqi_scsi_dev_raid_map_data *rmd) > case SA_RAID_0: > break; > case SA_RAID_1: > - if (rmd->is_write) > - is_supported = false; > + is_supported = false; You disable RAID1 READs with this patch. I can see you fix it again in 05/25, still it looks wrong. > break; > case SA_RAID_5: > - fallthrough; > + if (rmd->is_write && !ctrl_info->enable_r5_writes) > + is_supported = false; > + break; > case SA_RAID_6: > - if (rmd->is_write) > + if (rmd->is_write && !ctrl_info->enable_r6_writes) > is_supported = false; > break; > case SA_RAID_ADM: > @@ -2526,6 +2532,26 @@ static int pqi_calc_aio_r5_or_r6(struct > pqi_scsi_dev_raid_map_data *rmd, > rmd->total_disks_per_row)) + > (rmd->map_row * rmd->total_disks_per_row) + rmd- > > first_column; > > + if (rmd->is_write) { > + rmd->p_index = (rmd->map_row * rmd- > > total_disks_per_row) + rmd->data_disks_per_row; > + rmd->p_parity_it_nexus = raid_map->disk_data[rmd- > > p_index].aio_handle; I suppose you have made sure rmd->p_index can't be larger than the size of raid_map->disk_data. A comment explaining that would be helpful for the reader though. > + if (rmd->raid_level == SA_RAID_6) { > + rmd->q_index = (rmd->map_row * rmd- > > total_disks_per_row) + > + (rmd->data_disks_per_row + 1); > + rmd->q_parity_it_nexus = raid_map- > > disk_data[rmd->q_index].aio_handle; > + rmd->xor_mult = raid_map->disk_data[rmd- > > map_index].xor_mult[1]; See above. > + } > + if (rmd->blocks_per_row == 0) > + return PQI_RAID_BYPASS_INELIGIBLE; > +#if BITS_PER_LONG == 32 > + tmpdiv = rmd->first_block; > + do_div(tmpdiv, rmd->blocks_per_row); > + rmd->row = tmpdiv; > +#else > + rmd->row = rmd->first_block / rmd->blocks_per_row; > +#endif Why not always use do_div()? > + } > + > return 0; > } > > @@ -2567,7 +2593,7 @@ static int > pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, > > rmd.raid_level = device->raid_level; > > - if (!pqi_aio_raid_level_supported(&rmd)) > + if (!pqi_aio_raid_level_supported(ctrl_info, &rmd)) > return PQI_RAID_BYPASS_INELIGIBLE; > > if (unlikely(rmd.block_cnt == 0)) > @@ -2587,7 +2613,8 @@ static int > pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, > } else if (device->raid_level == SA_RAID_ADM) { > rc = pqi_calc_aio_raid_adm(&rmd, device); > } else if ((device->raid_level == SA_RAID_5 || > - device->raid_level == SA_RAID_6) && > rmd.layout_map_count > 1) { > + device->raid_level == SA_RAID_6) && > + (rmd.layout_map_count > 1 || rmd.is_write)) { > rc = pqi_calc_aio_r5_or_r6(&rmd, raid_map); > if (rc) > return PQI_RAID_BYPASS_INELIGIBLE; > @@ -2622,9 +2649,27 @@ static int > pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, > encryption_info_ptr = NULL; > } > > - return pqi_aio_submit_io(ctrl_info, scmd, rmd.aio_handle, > + if (rmd.is_write) { > + switch (device->raid_level) { > + case SA_RAID_0: > + return pqi_aio_submit_io(ctrl_info, scmd, > rmd.aio_handle, > rmd.cdb, rmd.cdb_length, queue_group, > encryption_info_ptr, true); > + case SA_RAID_5: > + case SA_RAID_6: > + return pqi_aio_submit_r56_write_io(ctrl_info, > scmd, queue_group, > + encryption_info_ptr, device, > &rmd); > + default: > + return pqi_aio_submit_io(ctrl_info, scmd, > rmd.aio_handle, > + rmd.cdb, rmd.cdb_length, queue_group, > + encryption_info_ptr, true); > + } > + } else { > + return pqi_aio_submit_io(ctrl_info, scmd, > rmd.aio_handle, > + rmd.cdb, rmd.cdb_length, queue_group, > + encryption_info_ptr, true); > + } > + > } > > #define PQI_STATUS_IDLE 0x0 > @@ -4844,6 +4889,12 @@ static void > pqi_calculate_queue_resources(struct pqi_ctrl_info *ctrl_info) > PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) / > sizeof(struct pqi_sg_descriptor)) + > PQI_MAX_EMBEDDED_SG_DESCRIPTORS; > + > + ctrl_info->max_sg_per_r56_iu = > + ((ctrl_info->max_inbound_iu_length - > + PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) / > + sizeof(struct pqi_sg_descriptor)) + > + PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS; > } > > static inline void pqi_set_sg_descriptor( > @@ -4931,6 +4982,44 @@ static int pqi_build_raid_sg_list(struct > pqi_ctrl_info *ctrl_info, > return 0; > } > > +static int pqi_build_aio_r56_sg_list(struct pqi_ctrl_info > *ctrl_info, > + struct pqi_aio_r56_path_request *request, struct scsi_cmnd > *scmd, > + struct pqi_io_request *io_request) > +{ > + u16 iu_length; > + int sg_count; > + bool chained; > + unsigned int num_sg_in_iu; > + struct scatterlist *sg; > + struct pqi_sg_descriptor *sg_descriptor; > + > + sg_count = scsi_dma_map(scmd); > + if (sg_count < 0) > + return sg_count; > + > + iu_length = offsetof(struct pqi_aio_r56_path_request, > sg_descriptors) - > + PQI_REQUEST_HEADER_LENGTH; > + num_sg_in_iu = 0; > + > + if (sg_count == 0) > + goto out; An if {} block would be better readable here. > + > + sg = scsi_sglist(scmd); > + sg_descriptor = request->sg_descriptors; > + > + num_sg_in_iu = pqi_build_sg_list(sg_descriptor, sg, sg_count, > io_request, > + ctrl_info->max_sg_per_r56_iu, &chained); > + > + request->partial = chained; > + iu_length += num_sg_in_iu * sizeof(*sg_descriptor); > + > +out: > + put_unaligned_le16(iu_length, &request->header.iu_length); > + request->num_sg_descriptors = num_sg_in_iu; > + > + return 0; > +} > + > static int pqi_build_aio_sg_list(struct pqi_ctrl_info *ctrl_info, > struct pqi_aio_path_request *request, struct scsi_cmnd *scmd, > struct pqi_io_request *io_request) > @@ -5335,6 +5424,88 @@ static int pqi_aio_submit_io(struct > pqi_ctrl_info *ctrl_info, > return 0; > } > > +static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info > *ctrl_info, > + struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group, > + struct pqi_encryption_info *encryption_info, struct > pqi_scsi_dev *device, > + struct pqi_scsi_dev_raid_map_data *rmd) > +{ > + int rc; > + struct pqi_io_request *io_request; > + struct pqi_aio_r56_path_request *r56_request; > + > + io_request = pqi_alloc_io_request(ctrl_info); > + io_request->io_complete_callback = pqi_aio_io_complete; > + io_request->scmd = scmd; > + io_request->raid_bypass = true; > + > + r56_request = io_request->iu; > + memset(r56_request, 0, offsetof(struct > pqi_aio_r56_path_request, sg_descriptors)); > + > + if (device->raid_level == SA_RAID_5 || device->raid_level == > SA_RAID_51) > + r56_request->header.iu_type = > PQI_REQUEST_IU_AIO_PATH_RAID5_IO; > + else > + r56_request->header.iu_type = > PQI_REQUEST_IU_AIO_PATH_RAID6_IO; > + > + put_unaligned_le16(*(u16 *)device->scsi3addr & 0x3fff, > &r56_request->volume_id); > + put_unaligned_le32(rmd->aio_handle, &r56_request- > > data_it_nexus); > + put_unaligned_le32(rmd->p_parity_it_nexus, &r56_request- > > p_parity_it_nexus); > + if (rmd->raid_level == SA_RAID_6) { > + put_unaligned_le32(rmd->q_parity_it_nexus, > &r56_request->q_parity_it_nexus); > + r56_request->xor_multiplier = rmd->xor_mult; > + } > + put_unaligned_le32(scsi_bufflen(scmd), &r56_request- > > data_length); > + r56_request->task_attribute = SOP_TASK_ATTRIBUTE_SIMPLE; > + put_unaligned_le64(rmd->row, &r56_request->row); > + > + put_unaligned_le16(io_request->index, &r56_request- > > request_id); > + r56_request->error_index = r56_request->request_id; > + > + if (rmd->cdb_length > sizeof(r56_request->cdb)) > + rmd->cdb_length = sizeof(r56_request->cdb); > + r56_request->cdb_length = rmd->cdb_length; > + memcpy(r56_request->cdb, rmd->cdb, rmd->cdb_length); > + > + switch (scmd->sc_data_direction) { > + case DMA_TO_DEVICE: > + r56_request->data_direction = SOP_READ_FLAG; > + break; I wonder how it would be possible that sc_data_direction is anything else but DMA_TO_DEVICE here. AFAICS we will only reach this code for WRITE commands. Add a comment, please. > + case DMA_FROM_DEVICE: > + r56_request->data_direction = SOP_WRITE_FLAG; > + break; > + case DMA_NONE: > + r56_request->data_direction = SOP_NO_DIRECTION_FLAG; > + break; > + case DMA_BIDIRECTIONAL: > + r56_request->data_direction = SOP_BIDIRECTIONAL; > + break; > + default: > + dev_err(&ctrl_info->pci_dev->dev, > + "unknown data direction: %d\n", > + scmd->sc_data_direction); > + break; > + } > + > + if (encryption_info) { > + r56_request->encryption_enable = true; > + put_unaligned_le16(encryption_info- > > data_encryption_key_index, > + &r56_request- > > data_encryption_key_index); > + put_unaligned_le32(encryption_info- > > encrypt_tweak_lower, > + &r56_request->encrypt_tweak_lower); > + put_unaligned_le32(encryption_info- > > encrypt_tweak_upper, > + &r56_request->encrypt_tweak_upper); > + } > + > + rc = pqi_build_aio_r56_sg_list(ctrl_info, r56_request, scmd, > io_request); > + if (rc) { > + pqi_free_io_request(io_request); > + return SCSI_MLQUEUE_HOST_BUSY; > + } > + > + pqi_start_io(ctrl_info, queue_group, AIO_PATH, io_request); > + > + return 0; > +} > + > static inline u16 pqi_get_hw_queue(struct pqi_ctrl_info *ctrl_info, > struct scsi_cmnd *scmd) > { > @@ -6298,6 +6469,60 @@ static ssize_t pqi_lockup_action_store(struct > device *dev, > return -EINVAL; > } > > +static ssize_t pqi_host_enable_r5_writes_show(struct device *dev, > + struct device_attribute *attr, char *buffer) > +{ > + struct Scsi_Host *shost = class_to_shost(dev); > + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); > + > + return scnprintf(buffer, 10, "%hhx\n", ctrl_info- > > enable_r5_writes); "%hhx" is deprecated, see https://lore.kernel.org/lkml/20190914015858.7c76e036@lwn.net/T/ > +} > + > +static ssize_t pqi_host_enable_r5_writes_store(struct device *dev, > + struct device_attribute *attr, const char *buffer, size_t > count) > +{ > + struct Scsi_Host *shost = class_to_shost(dev); > + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); > + u8 set_r5_writes = 0; > + > + if (kstrtou8(buffer, 0, &set_r5_writes)) > + return -EINVAL; > + > + if (set_r5_writes > 0) > + set_r5_writes = 1; > + > + ctrl_info->enable_r5_writes = set_r5_writes; > + > + return count; > +} > + > +static ssize_t pqi_host_enable_r6_writes_show(struct device *dev, > + struct device_attribute *attr, char *buffer) > +{ > + struct Scsi_Host *shost = class_to_shost(dev); > + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); > + > + return scnprintf(buffer, 10, "%hhx\n", ctrl_info- > > enable_r6_writes); See above > +} > + > +static ssize_t pqi_host_enable_r6_writes_store(struct device *dev, > + struct device_attribute *attr, const char *buffer, size_t > count) > +{ > + struct Scsi_Host *shost = class_to_shost(dev); > + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); > + u8 set_r6_writes = 0; > + > + if (kstrtou8(buffer, 0, &set_r6_writes)) > + return -EINVAL; > + > + if (set_r6_writes > 0) > + set_r6_writes = 1; > + > + ctrl_info->enable_r6_writes = set_r6_writes; > + > + return count; > +} > + > static DEVICE_ATTR(driver_version, 0444, pqi_driver_version_show, > NULL); > static DEVICE_ATTR(firmware_version, 0444, > pqi_firmware_version_show, NULL); > static DEVICE_ATTR(model, 0444, pqi_model_show, NULL); > @@ -6306,6 +6531,10 @@ static DEVICE_ATTR(vendor, 0444, > pqi_vendor_show, NULL); > static DEVICE_ATTR(rescan, 0200, NULL, pqi_host_rescan_store); > static DEVICE_ATTR(lockup_action, 0644, pqi_lockup_action_show, > pqi_lockup_action_store); > +static DEVICE_ATTR(enable_r5_writes, 0644, > + pqi_host_enable_r5_writes_show, > pqi_host_enable_r5_writes_store); > +static DEVICE_ATTR(enable_r6_writes, 0644, > + pqi_host_enable_r6_writes_show, > pqi_host_enable_r6_writes_store); > > static struct device_attribute *pqi_shost_attrs[] = { > &dev_attr_driver_version, > @@ -6315,6 +6544,8 @@ static struct device_attribute > *pqi_shost_attrs[] = { > &dev_attr_vendor, > &dev_attr_rescan, > &dev_attr_lockup_action, > + &dev_attr_enable_r5_writes, > + &dev_attr_enable_r6_writes, > NULL > }; > >
Subject: Re: [PATCH V3 04/25] smartpqi: add support for raid5 and raid6 writes Stg s> > struct pqi_raid_path_request { > struct pqi_iu_header header; > @@ -312,6 +313,39 @@ struct pqi_aio_path_request { > sg_descriptors[PQI_MAX_EMBEDDED_SG_DESCRIPTORS]; > }; > > +#define PQI_RAID56_XFER_LIMIT_4K 0x1000 /* 4Kib */ > +#define PQI_RAID56_XFER_LIMIT_8K 0x2000 /* 8Kib */ You don't seem to use these, and you'll remove them again in patch 06/25. Don: Removed these definitions. > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index 6bcb037ae9d7..c813cec10003 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -2245,13 +2250,14 @@ static bool > pqi_aio_raid_level_supported(struct pqi_scsi_dev_raid_map_data *rmd) > case SA_RAID_0: > break; > case SA_RAID_1: > - if (rmd->is_write) > - is_supported = false; > + is_supported = false; You disable RAID1 READs with this patch. I can see you fix it again in 05/25, still it looks wrong. Don: Corrected > break; > case SA_RAID_5: > - fallthrough; > + if (rmd->is_write && !ctrl_info->enable_r5_writes) > + is_supported = false; > + break; > case SA_RAID_6: > - if (rmd->is_write) > + if (rmd->is_write && !ctrl_info->enable_r6_writes) > is_supported = false; > break; > case SA_RAID_ADM: > @@ -2526,6 +2532,26 @@ static int pqi_calc_aio_r5_or_r6(struct > pqi_scsi_dev_raid_map_data *rmd, > rmd->total_disks_per_row)) + > (rmd->map_row * rmd->total_disks_per_row) + rmd- > > first_column; > > + if (rmd->is_write) { > + rmd->p_index = (rmd->map_row * rmd- > > total_disks_per_row) + rmd->data_disks_per_row; > + rmd->p_parity_it_nexus = raid_map->disk_data[rmd- > > p_index].aio_handle; I suppose you have made sure rmd->p_index can't be larger than the size of raid_map->disk_data. A comment explaining that would be helpful for the reader though. Don: Added a comment for p_index. > + if (rmd->raid_level == SA_RAID_6) { > + rmd->q_index = (rmd->map_row * rmd- > > total_disks_per_row) + > + (rmd->data_disks_per_row + 1); > + rmd->q_parity_it_nexus = raid_map- > > disk_data[rmd->q_index].aio_handle; > + rmd->xor_mult = raid_map->disk_data[rmd- > > map_index].xor_mult[1]; See above. Don: Comment updated to include q_index. > + } > + if (rmd->blocks_per_row == 0) > + return PQI_RAID_BYPASS_INELIGIBLE; #if > +BITS_PER_LONG == 32 > + tmpdiv = rmd->first_block; > + do_div(tmpdiv, rmd->blocks_per_row); > + rmd->row = tmpdiv; > +#else > + rmd->row = rmd->first_block / rmd->blocks_per_row; > +#endif Why not always use do_div()? Don: I had removed the BITS_PER_LONG check, was an attempt to clean up the code, but forgot we still need to support 32bit and I just re-added BITS_PER_LONG HUNKS. These HUNKS were there before I refactored the code so it predates me. Any chance I can leave this in? It's been through a lot of regression testing already... > @@ -4844,6 +4889,12 @@ static void > pqi_calculate_queue_resources(struct pqi_ctrl_info *ctrl_info) > PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) / > sizeof(struct pqi_sg_descriptor)) + > PQI_MAX_EMBEDDED_SG_DESCRIPTORS; > + > + ctrl_info->max_sg_per_r56_iu = > + ((ctrl_info->max_inbound_iu_length - > + PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) / > + sizeof(struct pqi_sg_descriptor)) + > + PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS; > } > > static inline void pqi_set_sg_descriptor( @@ -4931,6 +4982,44 @@ > static int pqi_build_raid_sg_list(struct pqi_ctrl_info *ctrl_info, > return 0; > } > > +static int pqi_build_aio_r56_sg_list(struct pqi_ctrl_info > *ctrl_info, > + struct pqi_aio_r56_path_request *request, struct scsi_cmnd > *scmd, > + struct pqi_io_request *io_request) { > + u16 iu_length; > + int sg_count; > + bool chained; > + unsigned int num_sg_in_iu; > + struct scatterlist *sg; > + struct pqi_sg_descriptor *sg_descriptor; > + > + sg_count = scsi_dma_map(scmd); > + if (sg_count < 0) > + return sg_count; > + > + iu_length = offsetof(struct pqi_aio_r56_path_request, > sg_descriptors) - > + PQI_REQUEST_HEADER_LENGTH; > + num_sg_in_iu = 0; > + > + if (sg_count == 0) > + goto out; An if {} block would be better readable here. Don> done. > } > > +static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info > *ctrl_info, > + struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group, > + struct pqi_encryption_info *encryption_info, struct > pqi_scsi_dev *device, > + struct pqi_scsi_dev_raid_map_data *rmd) { > + > + switch (scmd->sc_data_direction) { > + case DMA_TO_DEVICE: > + r56_request->data_direction = SOP_READ_FLAG; > + break; I wonder how it would be possible that sc_data_direction is anything else but DMA_TO_DEVICE here. AFAICS we will only reach this code for WRITE commands. Add a comment, please. Don: Great observation, removed switch block and added a comment. Set direction to write. > +static ssize_t pqi_host_enable_r5_writes_show(struct device *dev, > + struct device_attribute *attr, char *buffer) { > + struct Scsi_Host *shost = class_to_shost(dev); > + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); > + > + return scnprintf(buffer, 10, "%hhx\n", ctrl_info- > > enable_r5_writes); "%hhx" is deprecated, see https://lore.kernel.org/lkml/20190914015858.7c76e036@lwn.net/T/ Don: done > +static ssize_t pqi_host_enable_r6_writes_show(struct device *dev, > + struct device_attribute *attr, char *buffer) { > + struct Scsi_Host *shost = class_to_shost(dev); > + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); > + > + return scnprintf(buffer, 10, "%hhx\n", ctrl_info- > > enable_r6_writes); See above Don: done Don: Thanks for your all of your great effort on this patch. I'll upload a V4 with updates to this patch and the rest of your reviews soon. Thanks, Don Brace
On Fri, 2021-01-08 at 22:56 +0000, Don.Brace@microchip.com wrote: > > > + } > > + if (rmd->blocks_per_row == 0) > > + return PQI_RAID_BYPASS_INELIGIBLE; #if > > +BITS_PER_LONG == 32 > > + tmpdiv = rmd->first_block; > > + do_div(tmpdiv, rmd->blocks_per_row); > > + rmd->row = tmpdiv; > > +#else > > + rmd->row = rmd->first_block / rmd->blocks_per_row; > > +#endif > > Why not always use do_div()? > > Don: I had removed the BITS_PER_LONG check, was an attempt to clean > up the code, but forgot we still need to support 32bit and I just re- > added BITS_PER_LONG HUNKS. These HUNKS were there before I refactored > the code so it predates me. Any chance I can leave this in? It's been > through a lot of regression testing already... My suggestion was to rather do the opposite, use the 32bit code (with do_div()) for both 32bit and 64bit. AFAIK, this would work just fine (but not vice-versa). You can leave this in. It was just a suggestion how to improve readability. Perhaps consider cleaning it up sometime later. Regards, Martin
diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h index d486a2ec3045..e9844210c4a0 100644 --- a/drivers/scsi/smartpqi/smartpqi.h +++ b/drivers/scsi/smartpqi/smartpqi.h @@ -257,6 +257,7 @@ struct pqi_device_capability { }; #define PQI_MAX_EMBEDDED_SG_DESCRIPTORS 4 +#define PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS 3 struct pqi_raid_path_request { struct pqi_iu_header header; @@ -312,6 +313,39 @@ struct pqi_aio_path_request { sg_descriptors[PQI_MAX_EMBEDDED_SG_DESCRIPTORS]; }; +#define PQI_RAID56_XFER_LIMIT_4K 0x1000 /* 4Kib */ +#define PQI_RAID56_XFER_LIMIT_8K 0x2000 /* 8Kib */ +struct pqi_aio_r56_path_request { + struct pqi_iu_header header; + __le16 request_id; + __le16 volume_id; /* ID of the RAID volume */ + __le32 data_it_nexus; /* IT nexus for the data drive */ + __le32 p_parity_it_nexus; /* IT nexus for the P parity drive */ + __le32 q_parity_it_nexus; /* IT nexus for the Q parity drive */ + __le32 data_length; /* total bytes to read/write */ + u8 data_direction : 2; + u8 partial : 1; + u8 mem_type : 1; /* 0b: PCIe, 1b: DDR */ + u8 fence : 1; + u8 encryption_enable : 1; + u8 reserved : 2; + u8 task_attribute : 3; + u8 command_priority : 4; + u8 reserved1 : 1; + __le16 data_encryption_key_index; + u8 cdb[16]; + __le16 error_index; + u8 num_sg_descriptors; + u8 cdb_length; + u8 xor_multiplier; + u8 reserved2[3]; + __le32 encrypt_tweak_lower; + __le32 encrypt_tweak_upper; + u8 row; /* row = logical lba/blocks per row */ + u8 reserved3[8]; + struct pqi_sg_descriptor sg_descriptors[PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS]; +}; + struct pqi_io_response { struct pqi_iu_header header; __le16 request_id; @@ -484,6 +518,8 @@ struct pqi_raid_error_info { #define PQI_REQUEST_IU_TASK_MANAGEMENT 0x13 #define PQI_REQUEST_IU_RAID_PATH_IO 0x14 #define PQI_REQUEST_IU_AIO_PATH_IO 0x15 +#define PQI_REQUEST_IU_AIO_PATH_RAID5_IO 0x18 +#define PQI_REQUEST_IU_AIO_PATH_RAID6_IO 0x19 #define PQI_REQUEST_IU_GENERAL_ADMIN 0x60 #define PQI_REQUEST_IU_REPORT_VENDOR_EVENT_CONFIG 0x72 #define PQI_REQUEST_IU_SET_VENDOR_EVENT_CONFIG 0x73 @@ -1179,6 +1215,7 @@ struct pqi_ctrl_info { u16 max_inbound_iu_length_per_firmware; u16 max_inbound_iu_length; unsigned int max_sg_per_iu; + unsigned int max_sg_per_r56_iu; void *admin_queue_memory_base; u32 admin_queue_memory_length; dma_addr_t admin_queue_memory_base_dma_handle; @@ -1210,6 +1247,8 @@ struct pqi_ctrl_info { u8 soft_reset_handshake_supported : 1; u8 raid_iu_timeout_supported: 1; u8 tmf_iu_timeout_supported: 1; + u8 enable_r5_writes : 1; + u8 enable_r6_writes : 1; struct list_head scsi_device_list; spinlock_t scsi_device_list_lock; diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index 6bcb037ae9d7..c813cec10003 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -67,6 +67,10 @@ static int pqi_aio_submit_io(struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd, u32 aio_handle, u8 *cdb, unsigned int cdb_length, struct pqi_queue_group *queue_group, struct pqi_encryption_info *encryption_info, bool raid_bypass); +static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info *ctrl_info, + struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group, + struct pqi_encryption_info *encryption_info, struct pqi_scsi_dev *device, + struct pqi_scsi_dev_raid_map_data *rmd); static void pqi_ofa_ctrl_quiesce(struct pqi_ctrl_info *ctrl_info); static void pqi_ofa_ctrl_unquiesce(struct pqi_ctrl_info *ctrl_info); static int pqi_ofa_ctrl_restart(struct pqi_ctrl_info *ctrl_info); @@ -2237,7 +2241,8 @@ static inline void pqi_set_encryption_info( * Attempt to perform RAID bypass mapping for a logical volume I/O. */ -static bool pqi_aio_raid_level_supported(struct pqi_scsi_dev_raid_map_data *rmd) +static bool pqi_aio_raid_level_supported(struct pqi_ctrl_info *ctrl_info, + struct pqi_scsi_dev_raid_map_data *rmd) { bool is_supported = true; @@ -2245,13 +2250,14 @@ static bool pqi_aio_raid_level_supported(struct pqi_scsi_dev_raid_map_data *rmd) case SA_RAID_0: break; case SA_RAID_1: - if (rmd->is_write) - is_supported = false; + is_supported = false; break; case SA_RAID_5: - fallthrough; + if (rmd->is_write && !ctrl_info->enable_r5_writes) + is_supported = false; + break; case SA_RAID_6: - if (rmd->is_write) + if (rmd->is_write && !ctrl_info->enable_r6_writes) is_supported = false; break; case SA_RAID_ADM: @@ -2526,6 +2532,26 @@ static int pqi_calc_aio_r5_or_r6(struct pqi_scsi_dev_raid_map_data *rmd, rmd->total_disks_per_row)) + (rmd->map_row * rmd->total_disks_per_row) + rmd->first_column; + if (rmd->is_write) { + rmd->p_index = (rmd->map_row * rmd->total_disks_per_row) + rmd->data_disks_per_row; + rmd->p_parity_it_nexus = raid_map->disk_data[rmd->p_index].aio_handle; + if (rmd->raid_level == SA_RAID_6) { + rmd->q_index = (rmd->map_row * rmd->total_disks_per_row) + + (rmd->data_disks_per_row + 1); + rmd->q_parity_it_nexus = raid_map->disk_data[rmd->q_index].aio_handle; + rmd->xor_mult = raid_map->disk_data[rmd->map_index].xor_mult[1]; + } + if (rmd->blocks_per_row == 0) + return PQI_RAID_BYPASS_INELIGIBLE; +#if BITS_PER_LONG == 32 + tmpdiv = rmd->first_block; + do_div(tmpdiv, rmd->blocks_per_row); + rmd->row = tmpdiv; +#else + rmd->row = rmd->first_block / rmd->blocks_per_row; +#endif + } + return 0; } @@ -2567,7 +2593,7 @@ static int pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, rmd.raid_level = device->raid_level; - if (!pqi_aio_raid_level_supported(&rmd)) + if (!pqi_aio_raid_level_supported(ctrl_info, &rmd)) return PQI_RAID_BYPASS_INELIGIBLE; if (unlikely(rmd.block_cnt == 0)) @@ -2587,7 +2613,8 @@ static int pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, } else if (device->raid_level == SA_RAID_ADM) { rc = pqi_calc_aio_raid_adm(&rmd, device); } else if ((device->raid_level == SA_RAID_5 || - device->raid_level == SA_RAID_6) && rmd.layout_map_count > 1) { + device->raid_level == SA_RAID_6) && + (rmd.layout_map_count > 1 || rmd.is_write)) { rc = pqi_calc_aio_r5_or_r6(&rmd, raid_map); if (rc) return PQI_RAID_BYPASS_INELIGIBLE; @@ -2622,9 +2649,27 @@ static int pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, encryption_info_ptr = NULL; } - return pqi_aio_submit_io(ctrl_info, scmd, rmd.aio_handle, + if (rmd.is_write) { + switch (device->raid_level) { + case SA_RAID_0: + return pqi_aio_submit_io(ctrl_info, scmd, rmd.aio_handle, rmd.cdb, rmd.cdb_length, queue_group, encryption_info_ptr, true); + case SA_RAID_5: + case SA_RAID_6: + return pqi_aio_submit_r56_write_io(ctrl_info, scmd, queue_group, + encryption_info_ptr, device, &rmd); + default: + return pqi_aio_submit_io(ctrl_info, scmd, rmd.aio_handle, + rmd.cdb, rmd.cdb_length, queue_group, + encryption_info_ptr, true); + } + } else { + return pqi_aio_submit_io(ctrl_info, scmd, rmd.aio_handle, + rmd.cdb, rmd.cdb_length, queue_group, + encryption_info_ptr, true); + } + } #define PQI_STATUS_IDLE 0x0 @@ -4844,6 +4889,12 @@ static void pqi_calculate_queue_resources(struct pqi_ctrl_info *ctrl_info) PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) / sizeof(struct pqi_sg_descriptor)) + PQI_MAX_EMBEDDED_SG_DESCRIPTORS; + + ctrl_info->max_sg_per_r56_iu = + ((ctrl_info->max_inbound_iu_length - + PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) / + sizeof(struct pqi_sg_descriptor)) + + PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS; } static inline void pqi_set_sg_descriptor( @@ -4931,6 +4982,44 @@ static int pqi_build_raid_sg_list(struct pqi_ctrl_info *ctrl_info, return 0; } +static int pqi_build_aio_r56_sg_list(struct pqi_ctrl_info *ctrl_info, + struct pqi_aio_r56_path_request *request, struct scsi_cmnd *scmd, + struct pqi_io_request *io_request) +{ + u16 iu_length; + int sg_count; + bool chained; + unsigned int num_sg_in_iu; + struct scatterlist *sg; + struct pqi_sg_descriptor *sg_descriptor; + + sg_count = scsi_dma_map(scmd); + if (sg_count < 0) + return sg_count; + + iu_length = offsetof(struct pqi_aio_r56_path_request, sg_descriptors) - + PQI_REQUEST_HEADER_LENGTH; + num_sg_in_iu = 0; + + if (sg_count == 0) + goto out; + + sg = scsi_sglist(scmd); + sg_descriptor = request->sg_descriptors; + + num_sg_in_iu = pqi_build_sg_list(sg_descriptor, sg, sg_count, io_request, + ctrl_info->max_sg_per_r56_iu, &chained); + + request->partial = chained; + iu_length += num_sg_in_iu * sizeof(*sg_descriptor); + +out: + put_unaligned_le16(iu_length, &request->header.iu_length); + request->num_sg_descriptors = num_sg_in_iu; + + return 0; +} + static int pqi_build_aio_sg_list(struct pqi_ctrl_info *ctrl_info, struct pqi_aio_path_request *request, struct scsi_cmnd *scmd, struct pqi_io_request *io_request) @@ -5335,6 +5424,88 @@ static int pqi_aio_submit_io(struct pqi_ctrl_info *ctrl_info, return 0; } +static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info *ctrl_info, + struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group, + struct pqi_encryption_info *encryption_info, struct pqi_scsi_dev *device, + struct pqi_scsi_dev_raid_map_data *rmd) +{ + int rc; + struct pqi_io_request *io_request; + struct pqi_aio_r56_path_request *r56_request; + + io_request = pqi_alloc_io_request(ctrl_info); + io_request->io_complete_callback = pqi_aio_io_complete; + io_request->scmd = scmd; + io_request->raid_bypass = true; + + r56_request = io_request->iu; + memset(r56_request, 0, offsetof(struct pqi_aio_r56_path_request, sg_descriptors)); + + if (device->raid_level == SA_RAID_5 || device->raid_level == SA_RAID_51) + r56_request->header.iu_type = PQI_REQUEST_IU_AIO_PATH_RAID5_IO; + else + r56_request->header.iu_type = PQI_REQUEST_IU_AIO_PATH_RAID6_IO; + + put_unaligned_le16(*(u16 *)device->scsi3addr & 0x3fff, &r56_request->volume_id); + put_unaligned_le32(rmd->aio_handle, &r56_request->data_it_nexus); + put_unaligned_le32(rmd->p_parity_it_nexus, &r56_request->p_parity_it_nexus); + if (rmd->raid_level == SA_RAID_6) { + put_unaligned_le32(rmd->q_parity_it_nexus, &r56_request->q_parity_it_nexus); + r56_request->xor_multiplier = rmd->xor_mult; + } + put_unaligned_le32(scsi_bufflen(scmd), &r56_request->data_length); + r56_request->task_attribute = SOP_TASK_ATTRIBUTE_SIMPLE; + put_unaligned_le64(rmd->row, &r56_request->row); + + put_unaligned_le16(io_request->index, &r56_request->request_id); + r56_request->error_index = r56_request->request_id; + + if (rmd->cdb_length > sizeof(r56_request->cdb)) + rmd->cdb_length = sizeof(r56_request->cdb); + r56_request->cdb_length = rmd->cdb_length; + memcpy(r56_request->cdb, rmd->cdb, rmd->cdb_length); + + switch (scmd->sc_data_direction) { + case DMA_TO_DEVICE: + r56_request->data_direction = SOP_READ_FLAG; + break; + case DMA_FROM_DEVICE: + r56_request->data_direction = SOP_WRITE_FLAG; + break; + case DMA_NONE: + r56_request->data_direction = SOP_NO_DIRECTION_FLAG; + break; + case DMA_BIDIRECTIONAL: + r56_request->data_direction = SOP_BIDIRECTIONAL; + break; + default: + dev_err(&ctrl_info->pci_dev->dev, + "unknown data direction: %d\n", + scmd->sc_data_direction); + break; + } + + if (encryption_info) { + r56_request->encryption_enable = true; + put_unaligned_le16(encryption_info->data_encryption_key_index, + &r56_request->data_encryption_key_index); + put_unaligned_le32(encryption_info->encrypt_tweak_lower, + &r56_request->encrypt_tweak_lower); + put_unaligned_le32(encryption_info->encrypt_tweak_upper, + &r56_request->encrypt_tweak_upper); + } + + rc = pqi_build_aio_r56_sg_list(ctrl_info, r56_request, scmd, io_request); + if (rc) { + pqi_free_io_request(io_request); + return SCSI_MLQUEUE_HOST_BUSY; + } + + pqi_start_io(ctrl_info, queue_group, AIO_PATH, io_request); + + return 0; +} + static inline u16 pqi_get_hw_queue(struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd) { @@ -6298,6 +6469,60 @@ static ssize_t pqi_lockup_action_store(struct device *dev, return -EINVAL; } +static ssize_t pqi_host_enable_r5_writes_show(struct device *dev, + struct device_attribute *attr, char *buffer) +{ + struct Scsi_Host *shost = class_to_shost(dev); + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); + + return scnprintf(buffer, 10, "%hhx\n", ctrl_info->enable_r5_writes); +} + +static ssize_t pqi_host_enable_r5_writes_store(struct device *dev, + struct device_attribute *attr, const char *buffer, size_t count) +{ + struct Scsi_Host *shost = class_to_shost(dev); + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); + u8 set_r5_writes = 0; + + if (kstrtou8(buffer, 0, &set_r5_writes)) + return -EINVAL; + + if (set_r5_writes > 0) + set_r5_writes = 1; + + ctrl_info->enable_r5_writes = set_r5_writes; + + return count; +} + +static ssize_t pqi_host_enable_r6_writes_show(struct device *dev, + struct device_attribute *attr, char *buffer) +{ + struct Scsi_Host *shost = class_to_shost(dev); + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); + + return scnprintf(buffer, 10, "%hhx\n", ctrl_info->enable_r6_writes); +} + +static ssize_t pqi_host_enable_r6_writes_store(struct device *dev, + struct device_attribute *attr, const char *buffer, size_t count) +{ + struct Scsi_Host *shost = class_to_shost(dev); + struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost); + u8 set_r6_writes = 0; + + if (kstrtou8(buffer, 0, &set_r6_writes)) + return -EINVAL; + + if (set_r6_writes > 0) + set_r6_writes = 1; + + ctrl_info->enable_r6_writes = set_r6_writes; + + return count; +} + static DEVICE_ATTR(driver_version, 0444, pqi_driver_version_show, NULL); static DEVICE_ATTR(firmware_version, 0444, pqi_firmware_version_show, NULL); static DEVICE_ATTR(model, 0444, pqi_model_show, NULL); @@ -6306,6 +6531,10 @@ static DEVICE_ATTR(vendor, 0444, pqi_vendor_show, NULL); static DEVICE_ATTR(rescan, 0200, NULL, pqi_host_rescan_store); static DEVICE_ATTR(lockup_action, 0644, pqi_lockup_action_show, pqi_lockup_action_store); +static DEVICE_ATTR(enable_r5_writes, 0644, + pqi_host_enable_r5_writes_show, pqi_host_enable_r5_writes_store); +static DEVICE_ATTR(enable_r6_writes, 0644, + pqi_host_enable_r6_writes_show, pqi_host_enable_r6_writes_store); static struct device_attribute *pqi_shost_attrs[] = { &dev_attr_driver_version, @@ -6315,6 +6544,8 @@ static struct device_attribute *pqi_shost_attrs[] = { &dev_attr_vendor, &dev_attr_rescan, &dev_attr_lockup_action, + &dev_attr_enable_r5_writes, + &dev_attr_enable_r6_writes, NULL };