Why toggle_bounce only for disks?

Why toggle_bounce only for disks?

[Kyle McMartin]

Ahoi,

The following patch fixes a crash caused by attempting to bounce buffer
when an IDE CD-ROM is used on a machine with an IO-MMU. [At least, this
patch fixes things so I can use my IDE CD-ROM behind an ns87415 on a 
HP PA-RISC workstation.]

Why was this only allowed for disks?

--- a/drivers/ide/ide-lib.c   18 Jul 2005 18:46:30 -0000
+++ b/drivers/ide/ide-lib.c   6 Aug 2005 20:28:04 -0000
@@ -410,7 +410,7 @@ void ide_toggle_bounce(ide_drive_t *driv
 {
        u64 addr = BLK_BOUNCE_HIGH;     /* dma64_addr_t */
 
-       if (on && drive->media == ide_disk) {
+       if (on) {
                if (!PCI_DMA_BUS_IS_PHYS)
                        addr = BLK_BOUNCE_ANY;
                else if (HWIF(drive)->pci_dev)

Cheers,
-- 
Kyle McMartin

Re: Why toggle_bounce only for disks?

[Bartlomiej Zolnierkiewicz]

Hi,

http://www.ussg.iu.edu/hypermail/linux/kernel/0209.1/1074.html

Jens, is this still true??

Bartlomiej

On 8/7/05, Kyle McMartin <kyle@parisc-linux.org> wrote:
> Ahoi,
> 
> The following patch fixes a crash caused by attempting to bounce buffer
> when an IDE CD-ROM is used on a machine with an IO-MMU. [At least, this
> patch fixes things so I can use my IDE CD-ROM behind an ns87415 on a
> HP PA-RISC workstation.]
> 
> Why was this only allowed for disks?
> 
> --- a/drivers/ide/ide-lib.c   18 Jul 2005 18:46:30 -0000
> +++ b/drivers/ide/ide-lib.c   6 Aug 2005 20:28:04 -0000
> @@ -410,7 +410,7 @@ void ide_toggle_bounce(ide_drive_t *driv
>  {
>         u64 addr = BLK_BOUNCE_HIGH;     /* dma64_addr_t */
> 
> -       if (on && drive->media == ide_disk) {
> +       if (on) {
>                 if (!PCI_DMA_BUS_IS_PHYS)
>                         addr = BLK_BOUNCE_ANY;
>                 else if (HWIF(drive)->pci_dev)
> 
> Cheers,
> --
> Kyle McMartin

Re: Why toggle_bounce only for disks?

[Jens Axboe]

On Fri, Aug 19 2005, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0209.1/1074.html
> 
> Jens, is this still true??

It is, ide-cd would require changes to accept highmem pages.

> > Ahoi,
> > 
> > The following patch fixes a crash caused by attempting to bounce buffer
> > when an IDE CD-ROM is used on a machine with an IO-MMU. [At least, this
> > patch fixes things so I can use my IDE CD-ROM behind an ns87415 on a
> > HP PA-RISC workstation.]
> > 
> > Why was this only allowed for disks?

Because not bouncing is a performance optimization and I only did the
work on ide-cd to allow it. Your patch breaks ide-cd on highmem i386
machines, so it's not acceptable.

Tells us more about this crash instead, I'm pretty sure you are working
around another issue (your io-mmu code, is it hardware or software?)
somehwere with this patch.


> > --- a/drivers/ide/ide-lib.c   18 Jul 2005 18:46:30 -0000
> > +++ b/drivers/ide/ide-lib.c   6 Aug 2005 20:28:04 -0000
> > @@ -410,7 +410,7 @@ void ide_toggle_bounce(ide_drive_t *driv
> >  {
> >         u64 addr = BLK_BOUNCE_HIGH;     /* dma64_addr_t */
> > 
> > -       if (on && drive->media == ide_disk) {
> > +       if (on) {
> >                 if (!PCI_DMA_BUS_IS_PHYS)
> >                         addr = BLK_BOUNCE_ANY;
> >                 else if (HWIF(drive)->pci_dev)
> > 
> > Cheers,
> > --
> > Kyle McMartin
> 

-- 
Jens Axboe

Re: [parisc-linux] Re: Why toggle_bounce only for disks?

[James Bottomley]

On Fri, 2005-08-19 at 12:21 +0200, Jens Axboe wrote:
> Because not bouncing is a performance optimization and I only did the
> work on ide-cd to allow it. Your patch breaks ide-cd on highmem i386
> machines, so it's not acceptable.
> 
> Tells us more about this crash instead, I'm pretty sure you are working
> around another issue (your io-mmu code, is it hardware or software?)
> somehwere with this patch.

OK, so the particular fix is wrong; but the logic in ide_toggle_bounce()
is also incorrec.  Our problem is not that we don't want to bounce
highmem in ide-cd, it's that this is a parisc system with an IOMMU and
doesn't have any highmem to begin with.

we need ide_toggle_bounce to return BLK_BOUNCE_ANY always if
PCI_DMA_BUS_IS_PHYS is not set.

James

diff --git a/drivers/ide/ide-lib.c b/drivers/ide/ide-lib.c
--- a/drivers/ide/ide-lib.c
+++ b/drivers/ide/ide-lib.c
@@ -410,10 +410,10 @@ void ide_toggle_bounce(ide_drive_t *driv
 {
 	u64 addr = BLK_BOUNCE_HIGH;	/* dma64_addr_t */
 
-	if (on && drive->media == ide_disk) {
-		if (!PCI_DMA_BUS_IS_PHYS)
-			addr = BLK_BOUNCE_ANY;
-		else if (HWIF(drive)->pci_dev)
+	if (!PCI_DMA_BUS_IS_PHYS)
+		addr = BLK_BOUNCE_ANY;
+	else if (on && drive->media == ide_disk) {
+		if (HWIF(drive)->pci_dev)
 			addr = HWIF(drive)->pci_dev->dma_mask;
 	}
 

Re: [parisc-linux] Re: Why toggle_bounce only for disks?

[Jens Axboe]

On Fri, Aug 19 2005, James Bottomley wrote:
> On Fri, 2005-08-19 at 12:21 +0200, Jens Axboe wrote:
> > Because not bouncing is a performance optimization and I only did the
> > work on ide-cd to allow it. Your patch breaks ide-cd on highmem i386
> > machines, so it's not acceptable.
> > 
> > Tells us more about this crash instead, I'm pretty sure you are working
> > around another issue (your io-mmu code, is it hardware or software?)
> > somehwere with this patch.
> 
> OK, so the particular fix is wrong; but the logic in ide_toggle_bounce()
> is also incorrec.  Our problem is not that we don't want to bounce
> highmem in ide-cd, it's that this is a parisc system with an IOMMU and
> doesn't have any highmem to begin with.
> 
> we need ide_toggle_bounce to return BLK_BOUNCE_ANY always if
> PCI_DMA_BUS_IS_PHYS is not set.
> 
> James
> 
> diff --git a/drivers/ide/ide-lib.c b/drivers/ide/ide-lib.c
> --- a/drivers/ide/ide-lib.c
> +++ b/drivers/ide/ide-lib.c
> @@ -410,10 +410,10 @@ void ide_toggle_bounce(ide_drive_t *driv
>  {
>  	u64 addr = BLK_BOUNCE_HIGH;	/* dma64_addr_t */
>  
> -	if (on && drive->media == ide_disk) {
> -		if (!PCI_DMA_BUS_IS_PHYS)
> -			addr = BLK_BOUNCE_ANY;
> -		else if (HWIF(drive)->pci_dev)
> +	if (!PCI_DMA_BUS_IS_PHYS)
> +		addr = BLK_BOUNCE_ANY;
> +	else if (on && drive->media == ide_disk) {
> +		if (HWIF(drive)->pci_dev)
>  			addr = HWIF(drive)->pci_dev->dma_mask;
>  	}

That looks more correct, indeed. It's really too convoluted for drivers,
the defines/setup could do with a little work-over.

-- 
Jens Axboe