2006-02-01 08:59:18

by Jes Sorensen

[permalink] [raw]
Subject: [patch] SGIIOC4 limit request size

Hi,

This one takes care of a problem with the SGI IOC4 driver where it
hits DMA problems if the request grows too large.

Cheers,
Jes

Avoid requests larger than the number of SG table entries, to avoid
DMA timeouts.

Signed-off-by: Jes Sorensen <[email protected]>

----

drivers/ide/pci/sgiioc4.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/ide/pci/sgiioc4.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/sgiioc4.c
+++ linux-2.6/drivers/ide/pci/sgiioc4.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003 Silicon Graphics, Inc. All Rights Reserved.
+ * Copyright (C) 2003, 2006 Silicon Graphics, Inc. All Rights Reserved.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License
@@ -613,6 +613,12 @@
hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
hwif->ide_dma_timeout = &__ide_dma_timeout;
hwif->INB = &sgiioc4_INB;
+
+ /*
+ * Limit the request size to avoid DMA timeouts when
+ * requesting more entries than goes in the sg table.
+ */
+ hwif->rqsize = 127;
}

static int __devinit


Subject: Re: [patch] SGIIOC4 limit request size

On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <[email protected]> wrote:
> Hi,

Hi,

> This one takes care of a problem with the SGI IOC4 driver where it
> hits DMA problems if the request grows too large.

Does this happen only for CONFIG_IA64_PAGE_SIZE_4KB=y
or CONFIG_IA64_PAGE_SIZE_8KB=y?

from sgiioc4.c:

/* Each Physical Region Descriptor Entry size is 16 bytes (2 * 64 bits) */
/* IOC4 has only 1 IDE channel */
#define IOC4_PRD_BYTES 16
#define IOC4_PRD_ENTRIES (PAGE_SIZE /(4*IOC4_PRD_BYTES))

As limiting request size to 127 sectors punishes performance
wouldn't it be better to define IOC4_PRD_ENTRIES to 256
if this is possible (would need 4 pages for PAGE_SIZE=4096
and 2 for PAGE_SIZE=8192)?

Cheers.
Bartlomiej

> Cheers,
> Jes
>
> Avoid requests larger than the number of SG table entries, to avoid
> DMA timeouts.
>
> Signed-off-by: Jes Sorensen <[email protected]>
>
> ----
>
> drivers/ide/pci/sgiioc4.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/drivers/ide/pci/sgiioc4.c
> ===================================================================
> --- linux-2.6.orig/drivers/ide/pci/sgiioc4.c
> +++ linux-2.6/drivers/ide/pci/sgiioc4.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2003 Silicon Graphics, Inc. All Rights Reserved.
> + * Copyright (C) 2003, 2006 Silicon Graphics, Inc. All Rights Reserved.
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms of version 2 of the GNU General Public License
> @@ -613,6 +613,12 @@
> hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
> hwif->ide_dma_timeout = &__ide_dma_timeout;
> hwif->INB = &sgiioc4_INB;
> +
> + /*
> + * Limit the request size to avoid DMA timeouts when
> + * requesting more entries than goes in the sg table.
> + */
> + hwif->rqsize = 127;
> }
>
> static int __devinit

2006-02-01 10:41:36

by Jes Sorensen

[permalink] [raw]
Subject: Re: [patch] SGIIOC4 limit request size

Bartlomiej Zolnierkiewicz wrote:
> On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <[email protected]> wrote:
>>This one takes care of a problem with the SGI IOC4 driver where it
>>hits DMA problems if the request grows too large.
>
>
> Does this happen only for CONFIG_IA64_PAGE_SIZE_4KB=y
> or CONFIG_IA64_PAGE_SIZE_8KB=y?
>
> from sgiioc4.c:
>
> /* Each Physical Region Descriptor Entry size is 16 bytes (2 * 64 bits) */
> /* IOC4 has only 1 IDE channel */
> #define IOC4_PRD_BYTES 16
> #define IOC4_PRD_ENTRIES (PAGE_SIZE /(4*IOC4_PRD_BYTES))
>
> As limiting request size to 127 sectors punishes performance
> wouldn't it be better to define IOC4_PRD_ENTRIES to 256
> if this is possible (would need 4 pages for PAGE_SIZE=4096
> and 2 for PAGE_SIZE=8192)?

This happens with the default page size which is 16KB, ie.
IOC4_PRD_ENTRIES=256, the problem is not due to the request
going beyond the number of PRD_ENTRIES. I haven't tried with smaller
page sizes but I would assume the problem would be the same.

Even with this patch performance seems very reasonable.

Jes

2006-02-01 10:49:26

by Jeremy Higdon

[permalink] [raw]
Subject: Re: [patch] SGIIOC4 limit request size

On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <[email protected]> wrote:
> > Hi,
>
> Hi,
>
> > This one takes care of a problem with the SGI IOC4 driver where it
> > hits DMA problems if the request grows too large.
>
> Does this happen only for CONFIG_IA64_PAGE_SIZE_4KB=y
> or CONFIG_IA64_PAGE_SIZE_8KB=y?

Actually, it happens with a 16KB page size.

> from sgiioc4.c:
>
> /* Each Physical Region Descriptor Entry size is 16 bytes (2 * 64 bits) */
> /* IOC4 has only 1 IDE channel */
> #define IOC4_PRD_BYTES 16
> #define IOC4_PRD_ENTRIES (PAGE_SIZE /(4*IOC4_PRD_BYTES))
>
> As limiting request size to 127 sectors punishes performance
> wouldn't it be better to define IOC4_PRD_ENTRIES to 256
> if this is possible (would need 4 pages for PAGE_SIZE=4096
> and 2 for PAGE_SIZE=8192)?

I may be misunderstanding something, but it looks to me as though
IOC4_PRD_ENTRIES may be ignored, since ide_init_queue() just uses
PRD_ENTRIES. Fortunately, with a 16KB page size, the arithmetic
works out to the same. In any case, it seems that the 64KB
limit is the problem. Whether that is due to too many s/g entries
or total byte count I cannot say. I do know that with a 2KB
physical sector size, the minimum size for a s/g entry should be
2KB, which would mean we're using at most 32 with 127 max sectors --
well below the 256 that we get from PRD_ENTRIES and IOC4_PRD_ENTRIES.

We're still looking for root cause of this problem. But with the
default 128KB max request size, we occasionally get timeouts on
DMA commands.

jeremy

> Cheers.
> Bartlomiej
>
> > Cheers,
> > Jes
> >
> > Avoid requests larger than the number of SG table entries, to avoid
> > DMA timeouts.
> >
> > Signed-off-by: Jes Sorensen <[email protected]>
> >
> > ----
> >
> > drivers/ide/pci/sgiioc4.c | 8 +++++++-
> > 1 files changed, 7 insertions(+), 1 deletion(-)
> >
> > Index: linux-2.6/drivers/ide/pci/sgiioc4.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/ide/pci/sgiioc4.c
> > +++ linux-2.6/drivers/ide/pci/sgiioc4.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2003 Silicon Graphics, Inc. All Rights Reserved.
> > + * Copyright (C) 2003, 2006 Silicon Graphics, Inc. All Rights Reserved.
> > *
> > * This program is free software; you can redistribute it and/or modify it
> > * under the terms of version 2 of the GNU General Public License
> > @@ -613,6 +613,12 @@
> > hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
> > hwif->ide_dma_timeout = &__ide_dma_timeout;
> > hwif->INB = &sgiioc4_INB;
> > +
> > + /*
> > + * Limit the request size to avoid DMA timeouts when
> > + * requesting more entries than goes in the sg table.
> > + */
> > + hwif->rqsize = 127;
> > }
> >
> > static int __devinit

Subject: Re: [patch] SGIIOC4 limit request size

On 2/1/06, Jeremy Higdon <[email protected]> wrote:
> On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <[email protected]> wrote:
> > > Hi,
> >
> > Hi,
> >
> > > This one takes care of a problem with the SGI IOC4 driver where it
> > > hits DMA problems if the request grows too large.
> >
> > Does this happen only for CONFIG_IA64_PAGE_SIZE_4KB=y
> > or CONFIG_IA64_PAGE_SIZE_8KB=y?
>
> Actually, it happens with a 16KB page size.
>
> > from sgiioc4.c:
> >
> > /* Each Physical Region Descriptor Entry size is 16 bytes (2 * 64 bits) */
> > /* IOC4 has only 1 IDE channel */
> > #define IOC4_PRD_BYTES 16
> > #define IOC4_PRD_ENTRIES (PAGE_SIZE /(4*IOC4_PRD_BYTES))
> >
> > As limiting request size to 127 sectors punishes performance
> > wouldn't it be better to define IOC4_PRD_ENTRIES to 256
> > if this is possible (would need 4 pages for PAGE_SIZE=4096
> > and 2 for PAGE_SIZE=8192)?
>
> I may be misunderstanding something, but it looks to me as though
> IOC4_PRD_ENTRIES may be ignored, since ide_init_queue() just uses
> PRD_ENTRIES. Fortunately, with a 16KB page size, the arithmetic
> works out to the same. In any case, it seems that the 64KB
> limit is the problem. Whether that is due to too many s/g entries

Indeed, seems that hwif->max_sg_nents is not respected when
setting queue ->max_hw_segments and ->max_phys_segments.

Does the logic really work the same?
Isn't PCI_DMA_BUS_IS_PHYS == 0 for SN2?

If so then the code sets ->max_{hw,phys}_segments
to IOC4_PRD_ENTRIES/2 which actually shouldn't hurt...

> or total byte count I cannot say. I do know that with a 2KB
> physical sector size, the minimum size for a s/g entry should be
> 2KB, which would mean we're using at most 32 with 127 max sectors --
> well below the 256 that we get from PRD_ENTRIES and IOC4_PRD_ENTRIES.
>
> We're still looking for root cause of this problem. But with the
> default 128KB max request size, we occasionally get timeouts on
> DMA commands.

I have no big problem with applying patch as it is but I think that
it just hides the real problem and/or makes it harder to hit...

Bartlomiej

> jeremy
>
> > Cheers.
> > Bartlomiej
> >
> > > Cheers,
> > > Jes
> > >
> > > Avoid requests larger than the number of SG table entries, to avoid
> > > DMA timeouts.
> > >
> > > Signed-off-by: Jes Sorensen <[email protected]>
> > >
> > > ----
> > >
> > > drivers/ide/pci/sgiioc4.c | 8 +++++++-
> > > 1 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-2.6/drivers/ide/pci/sgiioc4.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/ide/pci/sgiioc4.c
> > > +++ linux-2.6/drivers/ide/pci/sgiioc4.c
> > > @@ -1,5 +1,5 @@
> > > /*
> > > - * Copyright (c) 2003 Silicon Graphics, Inc. All Rights Reserved.
> > > + * Copyright (C) 2003, 2006 Silicon Graphics, Inc. All Rights Reserved.
> > > *
> > > * This program is free software; you can redistribute it and/or modify it
> > > * under the terms of version 2 of the GNU General Public License
> > > @@ -613,6 +613,12 @@
> > > hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
> > > hwif->ide_dma_timeout = &__ide_dma_timeout;
> > > hwif->INB = &sgiioc4_INB;
> > > +
> > > + /*
> > > + * Limit the request size to avoid DMA timeouts when
> > > + * requesting more entries than goes in the sg table.
> > > + */
> > > + hwif->rqsize = 127;
> > > }
> > >
> > > static int __devinit
>

2006-02-01 11:18:08

by Jeremy Higdon

[permalink] [raw]
Subject: Re: [patch] SGIIOC4 limit request size

On Wed, Feb 01, 2006 at 12:08:30PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 2/1/06, Jeremy Higdon <[email protected]> wrote:
> > On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <[email protected]> wrote:
> > > > Hi,
> > >
> > > Hi,
> > >
> > > > This one takes care of a problem with the SGI IOC4 driver where it
> > > > hits DMA problems if the request grows too large.
> > >
> > > Does this happen only for CONFIG_IA64_PAGE_SIZE_4KB=y
> > > or CONFIG_IA64_PAGE_SIZE_8KB=y?
> >
> > Actually, it happens with a 16KB page size.
> >
> > > from sgiioc4.c:
> > >
> > > /* Each Physical Region Descriptor Entry size is 16 bytes (2 * 64 bits) */
> > > /* IOC4 has only 1 IDE channel */
> > > #define IOC4_PRD_BYTES 16
> > > #define IOC4_PRD_ENTRIES (PAGE_SIZE /(4*IOC4_PRD_BYTES))
> > >
> > > As limiting request size to 127 sectors punishes performance
> > > wouldn't it be better to define IOC4_PRD_ENTRIES to 256
> > > if this is possible (would need 4 pages for PAGE_SIZE=4096
> > > and 2 for PAGE_SIZE=8192)?
> >
> > I may be misunderstanding something, but it looks to me as though
> > IOC4_PRD_ENTRIES may be ignored, since ide_init_queue() just uses
> > PRD_ENTRIES. Fortunately, with a 16KB page size, the arithmetic
> > works out to the same. In any case, it seems that the 64KB
> > limit is the problem. Whether that is due to too many s/g entries
>
> Indeed, seems that hwif->max_sg_nents is not respected when
> setting queue ->max_hw_segments and ->max_phys_segments.
>
> Does the logic really work the same?
> Isn't PCI_DMA_BUS_IS_PHYS == 0 for SN2?
>
> If so then the code sets ->max_{hw,phys}_segments
> to IOC4_PRD_ENTRIES/2 which actually shouldn't hurt...
>
> > or total byte count I cannot say. I do know that with a 2KB
> > physical sector size, the minimum size for a s/g entry should be
> > 2KB, which would mean we're using at most 32 with 127 max sectors --
> > well below the 256 that we get from PRD_ENTRIES and IOC4_PRD_ENTRIES.
> >
> > We're still looking for root cause of this problem. But with the
> > default 128KB max request size, we occasionally get timeouts on
> > DMA commands.
>
> I have no big problem with applying patch as it is but I think that
> it just hides the real problem and/or makes it harder to hit...
>
> Bartlomiej

I agree. I think I found the real problem.

I'm going to test it and sleep on it, but here is the correct patch,
I think. Hot off the press.

Give us 16 hours :-)

The problem was that the chip actually likes a count of 0x10000 for
a 64K s/g, but the original author programmed 0 instead of 0x10000
for that amount (I don't know why).

I'll send a better patch tomorrow. This one depends on a byte count
multiple of 2. Though according to the chip docs, it ignores bit 0
of the byte count anyway (and the address for that matter). So I
think this is functionally correct. But I think the xcount variable
is superfluous.

jeremy


--- a/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 03:13:40.000000000 -0800
+++ b/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 03:02:18.144450010 -0800
@@ -525,7 +525,7 @@
*table = 0x0;
table++;

- xcount = bcount & 0xffff;
+ xcount = ((bcount - 1) & 0xffff) + 1;
*table = cpu_to_be32(xcount);
table++;

Subject: Re: [patch] SGIIOC4 limit request size

On 2/1/06, Jeremy Higdon <[email protected]> wrote:
> On Wed, Feb 01, 2006 at 12:08:30PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 2/1/06, Jeremy Higdon <[email protected]> wrote:
> > > On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <[email protected]> wrote:
> > > > > Hi,
> > > >
> > > > Hi,
> > > >
> > > > > This one takes care of a problem with the SGI IOC4 driver where it
> > > > > hits DMA problems if the request grows too large.
> > > >
> > > > Does this happen only for CONFIG_IA64_PAGE_SIZE_4KB=y
> > > > or CONFIG_IA64_PAGE_SIZE_8KB=y?
> > >
> > > Actually, it happens with a 16KB page size.
> > >
> > > > from sgiioc4.c:
> > > >
> > > > /* Each Physical Region Descriptor Entry size is 16 bytes (2 * 64 bits) */
> > > > /* IOC4 has only 1 IDE channel */
> > > > #define IOC4_PRD_BYTES 16
> > > > #define IOC4_PRD_ENTRIES (PAGE_SIZE /(4*IOC4_PRD_BYTES))
> > > >
> > > > As limiting request size to 127 sectors punishes performance
> > > > wouldn't it be better to define IOC4_PRD_ENTRIES to 256
> > > > if this is possible (would need 4 pages for PAGE_SIZE=4096
> > > > and 2 for PAGE_SIZE=8192)?
> > >
> > > I may be misunderstanding something, but it looks to me as though
> > > IOC4_PRD_ENTRIES may be ignored, since ide_init_queue() just uses
> > > PRD_ENTRIES. Fortunately, with a 16KB page size, the arithmetic
> > > works out to the same. In any case, it seems that the 64KB
> > > limit is the problem. Whether that is due to too many s/g entries
> >
> > Indeed, seems that hwif->max_sg_nents is not respected when
> > setting queue ->max_hw_segments and ->max_phys_segments.
> >
> > Does the logic really work the same?
> > Isn't PCI_DMA_BUS_IS_PHYS == 0 for SN2?
> >
> > If so then the code sets ->max_{hw,phys}_segments
> > to IOC4_PRD_ENTRIES/2 which actually shouldn't hurt...
> >
> > > or total byte count I cannot say. I do know that with a 2KB
> > > physical sector size, the minimum size for a s/g entry should be
> > > 2KB, which would mean we're using at most 32 with 127 max sectors --
> > > well below the 256 that we get from PRD_ENTRIES and IOC4_PRD_ENTRIES.
> > >
> > > We're still looking for root cause of this problem. But with the
> > > default 128KB max request size, we occasionally get timeouts on
> > > DMA commands.
> >
> > I have no big problem with applying patch as it is but I think that
> > it just hides the real problem and/or makes it harder to hit...
> >
> > Bartlomiej
>
> I agree. I think I found the real problem.
>
> I'm going to test it and sleep on it, but here is the correct patch,
> I think. Hot off the press.
>
> Give us 16 hours :-)

:-)

> The problem was that the chip actually likes a count of 0x10000 for
> a 64K s/g, but the original author programmed 0 instead of 0x10000
> for that amount (I don't know why).

original BM-DMA interprets 0 as 64K since length field is limited to 16-bits

> I'll send a better patch tomorrow. This one depends on a byte count
> multiple of 2. Though according to the chip docs, it ignores bit 0
> of the byte count anyway (and the address for that matter). So I
> think this is functionally correct. But I think the xcount variable
> is superfluous.

it seems so

> jeremy
>
>
> --- a/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 03:13:40.000000000 -0800
> +++ b/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 03:02:18.144450010 -0800
> @@ -525,7 +525,7 @@
> *table = 0x0;
> table++;
>
> - xcount = bcount & 0xffff;
> + xcount = ((bcount - 1) & 0xffff) + 1;
> *table = cpu_to_be32(xcount);
> table++;
>
>

2006-02-01 11:36:37

by Jeremy Higdon

[permalink] [raw]
Subject: Re: [patch] SGIIOC4 limit request size

On Wed, Feb 01, 2006 at 12:26:26PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > I have no big problem with applying patch as it is but I think that
> > > it just hides the real problem and/or makes it harder to hit...
> > >
> > > Bartlomiej
> >
> > I agree. I think I found the real problem.
> >
> > I'm going to test it and sleep on it, but here is the correct patch,
> > I think. Hot off the press.
> >
> > Give us 16 hours :-)
>
> :-)
>
> > The problem was that the chip actually likes a count of 0x10000 for
> > a 64K s/g, but the original author programmed 0 instead of 0x10000
> > for that amount (I don't know why).
>
> original BM-DMA interprets 0 as 64K since length field is limited to 16-bits

That explains it.

> > I'll send a better patch tomorrow. This one depends on a byte count
> > multiple of 2. Though according to the chip docs, it ignores bit 0
> > of the byte count anyway (and the address for that matter). So I
> > think this is functionally correct. But I think the xcount variable
> > is superfluous.
>
> it seems so

Here's one that removes xcount. It seems to work too.
Should we set hwif->rqsize to 256, or are we pretty safe in
expecting that the default won't rise? The driver should be
able to handle more, but this ioc4 hardware is weird, and it
probably wouldn't get tested if a general change were made :-)

jeremy

--- a/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 03:29:34.000000000 -0800
+++ b/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 03:21:40.413245446 -0800
@@ -510,7 +510,7 @@
drive->name);
goto use_pio_instead;
} else {
- u32 xcount, bcount =
+ u32 bcount =
0x10000 - (cur_addr & 0xffff);

if (bcount > cur_len)
@@ -525,8 +525,7 @@
*table = 0x0;
table++;

- xcount = bcount & 0xffff;
- *table = cpu_to_be32(xcount);
+ *table = cpu_to_be32(bcount);
table++;

cur_addr += bcount;

Subject: Re: [patch] SGIIOC4 limit request size

On 2/1/06, Jeremy Higdon <[email protected]> wrote:
> On Wed, Feb 01, 2006 at 12:26:26PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > I'll send a better patch tomorrow. This one depends on a byte count
> > > multiple of 2. Though according to the chip docs, it ignores bit 0
> > > of the byte count anyway (and the address for that matter). So I
> > > think this is functionally correct. But I think the xcount variable
> > > is superfluous.
> >
> > it seems so
>
> Here's one that removes xcount. It seems to work too.
> Should we set hwif->rqsize to 256, or are we pretty safe in
> expecting that the default won't rise? The driver should be
> able to handle more, but this ioc4 hardware is weird, and it
> probably wouldn't get tested if a general change were made :-)

The current maximum request size is:
* 256 for LBA28 and ATAPI devices
* 1024 for LBA48 devices

The maximum request size allowed by IDE driver for
LBA48 devices will change to 65536 but block layer will
continue to use 1024 as a default maximum request size,
also IIRC sgiioc4 IDE is used only for ATAPI devices.
So I think that there is no need to worry about ->rqsize.

Bartlomiej

2006-02-01 13:39:50

by Alan Cox

[permalink] [raw]
Subject: Re: [patch] SGIIOC4 limit request size

On Wed, Feb 01, 2006 at 03:36:07AM -0800, Jeremy Higdon wrote:
> Here's one that removes xcount. It seems to work too.
> Should we set hwif->rqsize to 256, or are we pretty safe in
> expecting that the default won't rise? The driver should be

255 is the safest for LBA28 devices because a small number incorrectly
interpret 0 (meaning 256) as 0. And that can have unfortunate results

Subject: Re: [patch] SGIIOC4 limit request size

On 2/1/06, Alan Cox <[email protected]> wrote:
> On Wed, Feb 01, 2006 at 03:36:07AM -0800, Jeremy Higdon wrote:
> > Here's one that removes xcount. It seems to work too.
> > Should we set hwif->rqsize to 256, or are we pretty safe in
> > expecting that the default won't rise? The driver should be
>
> 255 is the safest for LBA28 devices because a small number incorrectly
> interpret 0 (meaning 256) as 0. And that can have unfortunate results

We can blacklist vulnerable devices if needed.
We have been using 256 for a long time now.

2006-02-02 08:01:03

by Jeremy Higdon

[permalink] [raw]
Subject: [patch] Fix DMA timeouts with sgiioc4

On Wed, Feb 01, 2006 at 01:44:01PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 2/1/06, Jeremy Higdon <[email protected]> wrote:
> >
> > Here's one that removes xcount. It seems to work too.
> > Should we set hwif->rqsize to 256, or are we pretty safe in
> > expecting that the default won't rise? The driver should be
> > able to handle more, but this ioc4 hardware is weird, and it
> > probably wouldn't get tested if a general change were made :-)
>
> The current maximum request size is:
> * 256 for LBA28 and ATAPI devices
> * 1024 for LBA48 devices
>
> The maximum request size allowed by IDE driver for
> LBA48 devices will change to 65536 but block layer will
> continue to use 1024 as a default maximum request size,
> also IIRC sgiioc4 IDE is used only for ATAPI devices.
> So I think that there is no need to worry about ->rqsize.


Thanks Bartlomiej. You're correct in that it is ATAPI only (and
read-only also).

In this case, this is the final patch (last night's with a copyright
update and removing spurious whitespace at end of line).

thanks

jeremy

Signed-off-by: Jeremy Higdon <[email protected]>

Fix sgiioc4 DMA timeout problem with 64KiB s/g elements.

--- a/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 23:57:08.000000000 -0800
+++ b/linux/drivers/ide/pci/sgiioc4.c 2006-02-01 23:56:47.169588392 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003 Silicon Graphics, Inc. All Rights Reserved.
+ * Copyright (c) 2003-2006 Silicon Graphics, Inc. All Rights Reserved.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License
@@ -510,7 +510,7 @@
drive->name);
goto use_pio_instead;
} else {
- u32 xcount, bcount =
+ u32 bcount =
0x10000 - (cur_addr & 0xffff);

if (bcount > cur_len)
@@ -525,8 +525,7 @@
*table = 0x0;
table++;

- xcount = bcount & 0xffff;
- *table = cpu_to_be32(xcount);
+ *table = cpu_to_be32(bcount);
table++;

cur_addr += bcount;
@@ -680,7 +679,7 @@
return -EIO;

/* Create /proc/ide entries */
- create_proc_ide_interfaces();
+ create_proc_ide_interfaces();

return 0;
}

2006-02-02 08:45:33

by Jes Sorensen

[permalink] [raw]
Subject: Re: [patch] Fix DMA timeouts with sgiioc4

Jeremy Higdon wrote:
> Thanks Bartlomiej. You're correct in that it is ATAPI only (and
> read-only also).
>
> In this case, this is the final patch (last night's with a copyright
> update and removing spurious whitespace at end of line).

Hi Jeremy,

Just ack'ing for completeness! Please discard my previous patch and use
this one instead.

Cheers,
Jes

>
> jeremy
>
> Signed-off-by: Jeremy Higdon <[email protected]>
Acked-by: Jes Sorensen <[email protected]>