[PATCH] fix dma_unmap_sg misuse

[PATCH] fix dma_unmap_sg misuse

[FUJITA Tomonori]

Looks like that libata misuses dma_unmap_sg though I'm not familiar
with libata so I might completely misunderstand something.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] fix dma_unmap_sg misuse

DMA-mapping.txt says:

To unmap a scatterlist, just call:

	pci_unmap_sg(pdev, sglist, nents, direction);

Again, make sure DMA activity has already finished.

PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
              the _same_ one you passed into the pci_map_sg call,
	      it should _NOT_ be the 'count' value _returned_ from the
              pci_map_sg call.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/ata/libata-core.c |    4 ++--
 include/linux/libata.h    |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9fbf059..5e324ce 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4612,7 +4612,7 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
 	VPRINTK("unmapping %u sg elements\n", qc->n_elem);
 
 	if (qc->n_elem)
-		dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
+		dma_unmap_sg(ap->dev, sg, qc->orig_n_elem, dir);
 
 	qc->flags &= ~ATA_QCFLAG_DMAMAP;
 	qc->sg = NULL;
@@ -4727,7 +4727,7 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
 		return -1;
 
 	DPRINTK("%d sg elements mapped\n", n_elem);
-
+	qc->orig_n_elem = qc->n_elem;
 	qc->n_elem = n_elem;
 	qc->flags |= ATA_QCFLAG_DMAMAP;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5577ea8..2de827d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -530,6 +530,7 @@ struct ata_queued_cmd {
 	unsigned long		flags;		/* ATA_QCFLAG_xxx */
 	unsigned int		tag;
 	unsigned int		n_elem;
+	unsigned int		orig_n_elem;
 
 	int			dma_dir;
 
-- 
1.6.0.6

Re: [PATCH] fix dma_unmap_sg misuse

[Bartlomiej Zolnierkiewicz]

On Tuesday 17 February 2009, FUJITA Tomonori wrote:
> Looks like that libata misuses dma_unmap_sg though I'm not familiar
> with libata so I might completely misunderstand something.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] fix dma_unmap_sg misuse
> 
> DMA-mapping.txt says:
> 
> To unmap a scatterlist, just call:
> 
> 	pci_unmap_sg(pdev, sglist, nents, direction);
> 
> Again, make sure DMA activity has already finished.
> 
> PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
>               the _same_ one you passed into the pci_map_sg call,
> 	      it should _NOT_ be the 'count' value _returned_ from the
>               pci_map_sg call.

This is correct and other subsystems (SCSI, IDE) are compliant with it.

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

> ---
>  drivers/ata/libata-core.c |    4 ++--
>  include/linux/libata.h    |    1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9fbf059..5e324ce 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4612,7 +4612,7 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
>  	VPRINTK("unmapping %u sg elements\n", qc->n_elem);
>  
>  	if (qc->n_elem)
> -		dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
> +		dma_unmap_sg(ap->dev, sg, qc->orig_n_elem, dir);
>  
>  	qc->flags &= ~ATA_QCFLAG_DMAMAP;
>  	qc->sg = NULL;
> @@ -4727,7 +4727,7 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
>  		return -1;
>  
>  	DPRINTK("%d sg elements mapped\n", n_elem);
> -
> +	qc->orig_n_elem = qc->n_elem;
>  	qc->n_elem = n_elem;
>  	qc->flags |= ATA_QCFLAG_DMAMAP;
>  
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 5577ea8..2de827d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -530,6 +530,7 @@ struct ata_queued_cmd {
>  	unsigned long		flags;		/* ATA_QCFLAG_xxx */
>  	unsigned int		tag;
>  	unsigned int		n_elem;
> +	unsigned int		orig_n_elem;
>  
>  	int			dma_dir;

Re: [PATCH] fix dma_unmap_sg misuse

[Chuck Ebbert]

On Tue, 17 Feb 2009 13:35:27 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] fix dma_unmap_sg misuse
> 
> DMA-mapping.txt says:
> 
> To unmap a scatterlist, just call:
> 
> 	pci_unmap_sg(pdev, sglist, nents, direction);
> 
> Again, make sure DMA activity has already finished.
> 
> PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
>               the _same_ one you passed into the pci_map_sg call,
> 	      it should _NOT_ be the 'count' value _returned_ from the
>               pci_map_sg call.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/ata/libata-core.c |    4 ++--
>  include/linux/libata.h    |    1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9fbf059..5e324ce 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4612,7 +4612,7 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
>  	VPRINTK("unmapping %u sg elements\n", qc->n_elem);
>  
>  	if (qc->n_elem)

	if (qc->orig_n_elem)
?

> -		dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
> +		dma_unmap_sg(ap->dev, sg, qc->orig_n_elem, dir);
>  
>  	qc->flags &= ~ATA_QCFLAG_DMAMAP;
>  	qc->sg = NULL;

Re: [PATCH] fix dma_unmap_sg misuse

[Tejun Heo]

FUJITA Tomonori wrote:
> Looks like that libata misuses dma_unmap_sg though I'm not familiar
> with libata so I might completely misunderstand something.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] fix dma_unmap_sg misuse
> 
> DMA-mapping.txt says:
> 
> To unmap a scatterlist, just call:
> 
> 	pci_unmap_sg(pdev, sglist, nents, direction);
> 
> Again, make sure DMA activity has already finished.
> 
> PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
>               the _same_ one you passed into the pci_map_sg call,
> 	      it should _NOT_ be the 'count' value _returned_ from the
>               pci_map_sg call.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Right, nice spotting.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] fix dma_unmap_sg misuse

[FUJITA Tomonori]

On Wed, 18 Feb 2009 21:59:49 +0100
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> On Tuesday 17 February 2009, FUJITA Tomonori wrote:
> > Looks like that libata misuses dma_unmap_sg though I'm not familiar
> > with libata so I might completely misunderstand something.
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: [PATCH] fix dma_unmap_sg misuse
> > 
> > DMA-mapping.txt says:
> > 
> > To unmap a scatterlist, just call:
> > 
> > 	pci_unmap_sg(pdev, sglist, nents, direction);
> > 
> > Again, make sure DMA activity has already finished.
> > 
> > PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
> >               the _same_ one you passed into the pci_map_sg call,
> > 	      it should _NOT_ be the 'count' value _returned_ from the
> >               pci_map_sg call.
> 
> This is correct and other subsystems (SCSI, IDE) are compliant with it.
> 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Thanks,

I guess that the ide stack has the same problem (I guess that you
already knew that though).

Looks like that the ide stack in linux-next has more serious problem
about handling dma_map_sg?

In linux-next, the ide doesn't save the returned value of dma_map_sg
and assumes that dma_map_sg returns the same value to the 'nents'
argument of dma_map_sg?

Again I'm not familiar with the ide stack at all so I might completely
misunderstand something.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Thu, 19 Feb 2009 21:51:17 +0900
Subject: [PATCH] ide: save the returned value of dma_map_sg

dma_map_sg could return a value different to 'nents' argument of
dma_map_sg so the ide stack needs to save it for the later usage
(e.g. for_each_sg).

The ide stack also needs to save the original sg_nents value for
pci_unmap_sg.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/ide/ide-dma.c |    6 +++++-
 include/linux/ide.h   |    1 +
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
index b4fa861..3dbf80c 100644
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -143,6 +143,10 @@ int ide_build_sglist(ide_drive_t *drive, struct ide_cmd *cmd)
 	i = dma_map_sg(hwif->dev, sg, cmd->sg_nents, cmd->sg_dma_direction);
 	if (i == 0)
 		ide_map_sg(drive, cmd);
+	else {
+		cmd->orig_sg_nents = cmd->sg_nents;
+		cmd->sg_nents = i;
+	}
 
 	return i;
 }
@@ -163,7 +167,7 @@ void ide_destroy_dmatable(ide_drive_t *drive)
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_cmd *cmd = &hwif->cmd;
 
-	dma_unmap_sg(hwif->dev, hwif->sg_table, cmd->sg_nents,
+	dma_unmap_sg(hwif->dev, hwif->sg_table, cmd->orig_sg_nents,
 		     cmd->sg_dma_direction);
 }
 EXPORT_SYMBOL_GPL(ide_destroy_dmatable);
diff --git a/include/linux/ide.h b/include/linux/ide.h
index c75631c..d3465d1 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -347,6 +347,7 @@ struct ide_cmd {
 	int			protocol;
 
 	int			sg_nents;	  /* number of sg entries */
+	int			orig_sg_nents;
 	int			sg_dma_direction; /* DMA transfer direction */
 
 	unsigned int		nbytes;
-- 
1.6.0.6

Re: [PATCH] fix dma_unmap_sg misuse

[FUJITA Tomonori]

On Wed, 18 Feb 2009 17:12:35 -0500
Chuck Ebbert <cebbert@redhat.com> wrote:

> On Tue, 17 Feb 2009 13:35:27 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: [PATCH] fix dma_unmap_sg misuse
> > 
> > DMA-mapping.txt says:
> > 
> > To unmap a scatterlist, just call:
> > 
> > 	pci_unmap_sg(pdev, sglist, nents, direction);
> > 
> > Again, make sure DMA activity has already finished.
> > 
> > PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
> >               the _same_ one you passed into the pci_map_sg call,
> > 	      it should _NOT_ be the 'count' value _returned_ from the
> >               pci_map_sg call.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  drivers/ata/libata-core.c |    4 ++--
> >  include/linux/libata.h    |    1 +
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 9fbf059..5e324ce 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -4612,7 +4612,7 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
> >  	VPRINTK("unmapping %u sg elements\n", qc->n_elem);
> >  
> >  	if (qc->n_elem)
> 
> 	if (qc->orig_n_elem)
> ?

Either is fine because when n_elem is not zero, qc->orig_n_elem is not
zero.

> 
> > -		dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
> > +		dma_unmap_sg(ap->dev, sg, qc->orig_n_elem, dir);
> >  
> >  	qc->flags &= ~ATA_QCFLAG_DMAMAP;
> >  	qc->sg = NULL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix dma_unmap_sg misuse

[Bartlomiej Zolnierkiewicz]

On Thursday 19 February 2009, FUJITA Tomonori wrote:
> On Wed, 18 Feb 2009 21:59:49 +0100
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > On Tuesday 17 February 2009, FUJITA Tomonori wrote:
> > > Looks like that libata misuses dma_unmap_sg though I'm not familiar
> > > with libata so I might completely misunderstand something.
> > > 
> > > =
> > > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > Subject: [PATCH] fix dma_unmap_sg misuse
> > > 
> > > DMA-mapping.txt says:
> > > 
> > > To unmap a scatterlist, just call:
> > > 
> > > 	pci_unmap_sg(pdev, sglist, nents, direction);
> > > 
> > > Again, make sure DMA activity has already finished.
> > > 
> > > PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
> > >               the _same_ one you passed into the pci_map_sg call,
> > > 	      it should _NOT_ be the 'count' value _returned_ from the
> > >               pci_map_sg call.
> > 
> > This is correct and other subsystems (SCSI, IDE) are compliant with it.
> > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > 
> > Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> Thanks,
> 
> I guess that the ide stack has the same problem (I guess that you
> already knew that though).
> 
> Looks like that the ide stack in linux-next has more serious problem
> about handling dma_map_sg?
> 
> In linux-next, the ide doesn't save the returned value of dma_map_sg
> and assumes that dma_map_sg returns the same value to the 'nents'
> argument of dma_map_sg?
> 
> Again I'm not familiar with the ide stack at all so I might completely
> misunderstand something.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Date: Thu, 19 Feb 2009 21:51:17 +0900
> Subject: [PATCH] ide: save the returned value of dma_map_sg
> 
> dma_map_sg could return a value different to 'nents' argument of
> dma_map_sg so the ide stack needs to save it for the later usage
> (e.g. for_each_sg).
> 
> The ide stack also needs to save the original sg_nents value for
> pci_unmap_sg.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

applied, thanks!