* [patch] SGIIOC4 limit request size @ 2006-02-01 8:59 Jes Sorensen 2006-02-01 10:34 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Jes Sorensen @ 2006-02-01 8:59 UTC (permalink / raw) To: Alan Cox; +Cc: Linus Torvalds, linux-kernel, Jeremy Higdon 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 <jes@sgi.com> ---- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 8:59 [patch] SGIIOC4 limit request size Jes Sorensen @ 2006-02-01 10:34 ` Bartlomiej Zolnierkiewicz 2006-02-01 10:41 ` Jes Sorensen 2006-02-01 10:49 ` Jeremy Higdon 0 siblings, 2 replies; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2006-02-01 10:34 UTC (permalink / raw) To: Jes Sorensen; +Cc: Alan Cox, Linus Torvalds, linux-kernel, Jeremy Higdon On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <jes@sgi.com> 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 <jes@sgi.com> > > ---- > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 10:34 ` Bartlomiej Zolnierkiewicz @ 2006-02-01 10:41 ` Jes Sorensen 2006-02-01 10:49 ` Jeremy Higdon 1 sibling, 0 replies; 13+ messages in thread From: Jes Sorensen @ 2006-02-01 10:41 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Alan Cox, Linus Torvalds, linux-kernel, Jeremy Higdon Bartlomiej Zolnierkiewicz wrote: > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <jes@sgi.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 10:34 ` Bartlomiej Zolnierkiewicz 2006-02-01 10:41 ` Jes Sorensen @ 2006-02-01 10:49 ` Jeremy Higdon 2006-02-01 11:08 ` Bartlomiej Zolnierkiewicz 1 sibling, 1 reply; 13+ messages in thread From: Jeremy Higdon @ 2006-02-01 10:49 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote: > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <jes@sgi.com> 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 <jes@sgi.com> > > > > ---- > > > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 10:49 ` Jeremy Higdon @ 2006-02-01 11:08 ` Bartlomiej Zolnierkiewicz 2006-02-01 11:17 ` Jeremy Higdon 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2006-02-01 11:08 UTC (permalink / raw) To: Jeremy Higdon; +Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel On 2/1/06, Jeremy Higdon <jeremy@sgi.com> wrote: > On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote: > > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <jes@sgi.com> 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 <jes@sgi.com> > > > > > > ---- > > > > > > 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 11:08 ` Bartlomiej Zolnierkiewicz @ 2006-02-01 11:17 ` Jeremy Higdon 2006-02-01 11:26 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Jeremy Higdon @ 2006-02-01 11:17 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel On Wed, Feb 01, 2006 at 12:08:30PM +0100, Bartlomiej Zolnierkiewicz wrote: > On 2/1/06, Jeremy Higdon <jeremy@sgi.com> wrote: > > On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote: > > > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <jes@sgi.com> 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++; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 11:17 ` Jeremy Higdon @ 2006-02-01 11:26 ` Bartlomiej Zolnierkiewicz 2006-02-01 11:36 ` Jeremy Higdon 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2006-02-01 11:26 UTC (permalink / raw) To: Jeremy Higdon; +Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel On 2/1/06, Jeremy Higdon <jeremy@sgi.com> wrote: > On Wed, Feb 01, 2006 at 12:08:30PM +0100, Bartlomiej Zolnierkiewicz wrote: > > On 2/1/06, Jeremy Higdon <jeremy@sgi.com> wrote: > > > On Wed, Feb 01, 2006 at 11:34:18AM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > On 01 Feb 2006 03:59:16 -0500, Jes Sorensen <jes@sgi.com> 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++; > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 11:26 ` Bartlomiej Zolnierkiewicz @ 2006-02-01 11:36 ` Jeremy Higdon 2006-02-01 12:44 ` Bartlomiej Zolnierkiewicz 2006-02-01 13:39 ` [patch] SGIIOC4 limit request size Alan Cox 0 siblings, 2 replies; 13+ messages in thread From: Jeremy Higdon @ 2006-02-01 11:36 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 11:36 ` Jeremy Higdon @ 2006-02-01 12:44 ` Bartlomiej Zolnierkiewicz 2006-02-02 8:00 ` [patch] Fix DMA timeouts with sgiioc4 Jeremy Higdon 2006-02-01 13:39 ` [patch] SGIIOC4 limit request size Alan Cox 1 sibling, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2006-02-01 12:44 UTC (permalink / raw) To: Jeremy Higdon; +Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel On 2/1/06, Jeremy Higdon <jeremy@sgi.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [patch] Fix DMA timeouts with sgiioc4 2006-02-01 12:44 ` Bartlomiej Zolnierkiewicz @ 2006-02-02 8:00 ` Jeremy Higdon 2006-02-02 8:45 ` Jes Sorensen 0 siblings, 1 reply; 13+ messages in thread From: Jeremy Higdon @ 2006-02-02 8:00 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel, linux-ide On Wed, Feb 01, 2006 at 01:44:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > On 2/1/06, Jeremy Higdon <jeremy@sgi.com> 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 <jeremy@sgi.com> 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; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] Fix DMA timeouts with sgiioc4 2006-02-02 8:00 ` [patch] Fix DMA timeouts with sgiioc4 Jeremy Higdon @ 2006-02-02 8:45 ` Jes Sorensen 0 siblings, 0 replies; 13+ messages in thread From: Jes Sorensen @ 2006-02-02 8:45 UTC (permalink / raw) To: Jeremy Higdon Cc: Bartlomiej Zolnierkiewicz, Alan Cox, Linus Torvalds, linux-kernel, linux-ide 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 <jeremy@sgi.com> Acked-by: Jes Sorensen <jes@sgi.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 11:36 ` Jeremy Higdon 2006-02-01 12:44 ` Bartlomiej Zolnierkiewicz @ 2006-02-01 13:39 ` Alan Cox 2006-02-01 14:53 ` Bartlomiej Zolnierkiewicz 1 sibling, 1 reply; 13+ messages in thread From: Alan Cox @ 2006-02-01 13:39 UTC (permalink / raw) To: Jeremy Higdon Cc: Bartlomiej Zolnierkiewicz, Jes Sorensen, Alan Cox, Linus Torvalds, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch] SGIIOC4 limit request size 2006-02-01 13:39 ` [patch] SGIIOC4 limit request size Alan Cox @ 2006-02-01 14:53 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2006-02-01 14:53 UTC (permalink / raw) To: Alan Cox; +Cc: Jeremy Higdon, Jes Sorensen, Linus Torvalds, linux-kernel On 2/1/06, Alan Cox <alan@redhat.com> 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2006-02-02 8:45 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2006-02-01 8:59 [patch] SGIIOC4 limit request size Jes Sorensen 2006-02-01 10:34 ` Bartlomiej Zolnierkiewicz 2006-02-01 10:41 ` Jes Sorensen 2006-02-01 10:49 ` Jeremy Higdon 2006-02-01 11:08 ` Bartlomiej Zolnierkiewicz 2006-02-01 11:17 ` Jeremy Higdon 2006-02-01 11:26 ` Bartlomiej Zolnierkiewicz 2006-02-01 11:36 ` Jeremy Higdon 2006-02-01 12:44 ` Bartlomiej Zolnierkiewicz 2006-02-02 8:00 ` [patch] Fix DMA timeouts with sgiioc4 Jeremy Higdon 2006-02-02 8:45 ` Jes Sorensen 2006-02-01 13:39 ` [patch] SGIIOC4 limit request size Alan Cox 2006-02-01 14:53 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).