On Tue, 4 Jun 2019, Ming Lei wrote:
> The driver supporses that there isn't sg chain, and itereate the
> list one by one. This way is obviously wrong.
>
> Fixes it by sgl helper.
>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Bart Van Assche <bvanassche@xxxxxxx>
> Cc: Ewan D. Milne <emilne@xxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
> drivers/scsi/esp_scsi.c | 32 +++++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 76e7ca864d6a..58b4e059dcfb 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -371,6 +371,7 @@ static void esp_map_dma(struct esp *esp, struct scsi_cmnd *cmd)
> struct esp_cmd_priv *spriv = ESP_CMD_PRIV(cmd);
> struct scatterlist *sg = scsi_sglist(cmd);
> int total = 0, i;
> + struct scatterlist *sgt;
>
> if (cmd->sc_data_direction == DMA_NONE)
> return;
> @@ -381,14 +382,15 @@ static void esp_map_dma(struct esp *esp, struct scsi_cmnd *cmd)
> * a dma address, so perform an identity mapping.
> */
> spriv->num_sg = scsi_sg_count(cmd);
> - for (i = 0; i < spriv->num_sg; i++) {
> - sg[i].dma_address = (uintptr_t)sg_virt(&sg[i]);
> - total += sg_dma_len(&sg[i]);
> +
> + scsi_for_each_sg(cmd, sgt, spriv->num_sg, i) {
> + sgt->dma_address = (uintptr_t)sg_virt(sgt);
> + total += sg_dma_len(sgt);
> }
> } else {
> spriv->num_sg = scsi_dma_map(cmd);
> - for (i = 0; i < spriv->num_sg; i++)
> - total += sg_dma_len(&sg[i]);
> + scsi_for_each_sg(cmd, sgt, spriv->num_sg, i)
> + total += sg_dma_len(sgt);
> }
> spriv->cur_residue = sg_dma_len(sg);
> spriv->cur_sg = sg;
> @@ -444,7 +446,7 @@ static void esp_advance_dma(struct esp *esp, struct esp_cmd_entry *ent,
> p->tot_residue = 0;
> }
> if (!p->cur_residue && p->tot_residue) {
> - p->cur_sg++;
> + p->cur_sg = sg_next(p->cur_sg);
> p->cur_residue = sg_dma_len(p->cur_sg);
> }
> }
> @@ -1610,6 +1612,22 @@ static void esp_msgin_extended(struct esp *esp)
> scsi_esp_cmd(esp, ESP_CMD_SATN);
> }
>
> +static struct scatterlist *esp_sg_prev(struct scsi_cmnd *cmd,
> + struct scatterlist *sg)
> +{
> + int i;
> + struct scatterlist *tmp;
> + struct scatterlist *prev = NULL;
> +
> + scsi_for_each_sg(cmd, tmp, scsi_sg_count(cmd), i) {
> + if (tmp == sg)
> + break;
> + prev = tmp;
> + }
> +
> + return prev;
> +}
> +
Did you consider recording the previous scatterlist pointer using an
additional member in struct esp_cmd_priv, to be assigned in
esp_advance_dma()? I think it would execute faster and require less code.
--
> /* Analyze msgin bytes received from target so far. Return non-zero
> * if there are more bytes needed to complete the message.
> */
> @@ -1647,7 +1665,7 @@ static int esp_msgin_process(struct esp *esp)
> spriv = ESP_CMD_PRIV(ent->cmd);
>
> if (spriv->cur_residue == sg_dma_len(spriv->cur_sg)) {
> - spriv->cur_sg--;
> + spriv->cur_sg = esp_sg_prev(ent->cmd, spriv->cur_sg);
> spriv->cur_residue = 1;
> } else
> spriv->cur_residue++;
>
