On Tue, Jun 4, 2019 at 5:22 PM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
Good point, will do it in V2.
Thanks,
Ming Lei
