RE: [PATCH V3 04/25] smartpqi: add support for raid5 and raid6 writes

[<Don.Brace@microchip.com>]

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