RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address
Hayes, Stuart wrote: Jens Axboe wrote: On Thu, Mar 17 2005, [EMAIL PROTECTED] wrote: Jens Axboe wrote: On Thu, Mar 17 2005, Jens Axboe wrote: On Thu, Mar 17 2005, [EMAIL PROTECTED] wrote: Jens Axboe wrote: On Wed, Mar 16 2005, [EMAIL PROTECTED] wrote: Jens Axboe wrote: On Wed, Mar 16 2005, [EMAIL PROTECTED] wrote: Hayes, Stuart wrote: This patch will map the sg buffers to kernel virtual memory space in the functions idescsi_input_buffers() and idescsi_output_buffers(). Without this patch, idescsi passes a null pointer to atapi_input_bytes() and atapi_output_bytes() when sg pages are in high memory (i686 architecture). I'm attaching as a file, too, as the text will certainly be wrapped. (Sorry for the subject rename--I'm trying to use the correct format for patch emails.) Thanks Stuart And, while there's another high memory/kmap patch question on this list... Is there some reason nobody seems interested in this patch (except Jens--thanks for the help!)? I'm kind of new to sending in patches, and I'm not sure if I'm just not waiting long enough, or if there is a problem with this patch... But really, we're getting a null pointer dereference oops when using ATAPI tape drives (with ide-scsi) without this patch... Sorry, that did seem to get dropped on the floor. Actually I'm wondering why you are seeing highmem pages there in the first place, it would be easier/better just to limit ide-scsi to non-highmem pages. That would remove the need to add any work arounds in the driver. I think we're seeing highmem pages in the sg list because that's where the user memory was when st_write() was called. The sg list is set up when st_write(), which calls setup_buffering(), which calls st_map_user_pages()... this just sets up the sg pages to point directly to the user memory. So, by the time ide-scsi comes into the picture, the sg list is already set up to point to high memory pages. Are you suggesting that ide-scsi should change the dma_mask for the device so that st_map_user_pages() won't let sg pages point to high memory? Or is there something else I'm missing? I think that is a bug, this effectively bypasses whatever restrictions the scsi host adapter has said about memory limits. It is a problem because the request doesn't come from the block layer (which handles all of this). I would much prefer fixing that real issue! By whatever restrictions the scsi host adapter has said about memory limits, are you referring to the dma_boundary or the dma_mask? I'm don't know any other ways a host adapter can specify memory limits. I wasn't complete in my statement above, though--st_map_user_pages() does prevent the sg list from pointing to pages that are above the max_pfn for the device (it gets max_pfn from scsi_calculate_bounce_ limit()), and that appears to be working ok. But, even though max_pfn is 0xf (so that the maximum physical address of any sg page won't be over 4G (0x)), there are apparently high memory pages that are below physical address of 4G (0x), but are still considered high memory. So the entire first 4G of memory isn't low memory... for example, on my box, pfn 0xfbc3c, with physical address 0xfbc3c000, has the PG_highmem bit set in (page)-flags. Because of this, when ide-scsi needs to do PIO, it needs to do a kmap_atomic(). Am I completely missing your point? Ok good, so st isn't broken at least. You are not missing my point, but maybe you don't realize that highmem pages doesn't refer to any specific address in memory - it refers to pages that don't have a virtual mapping, so depending on the kernel config that can be anywhere between ~900MB and 2GB (typicall). ide-scsi should just use blk_max_low_pfn as the bounce limit, that will work all the time. IOW, one way to solve this would be to add an shost-bounce_high flag and add something ala if (shost-bounce_high) return BLK_BOUNCE_HIGH; to scsi_calculate_bounce_limit() Yes, I could easily implement that. I had been trying to figure out how to go about implementing what you said--I was looking at making idescsi_attach change the dma_mask that scsi_calculate_bounce_limit() currently looks at, but it wasn't looking very pretty. Unfortunately, I won't be able to do that fix with a DKMS driver on an existing kernel... but that's not really relevant to this list. For your existing kernel fix, just use the one you sent last using kmap_atomic, it should work for you as well. I'll try this out and send in a patch. Thanks! No problem, thanks for being persistent in getting this fixed :) OK, sorry for the delay. Here's the new patch to handle this as you suggested, against the 2.6.11.5 kernel. I have tested it, and it works. Do I need to re-send this with a new subject line, since the patch doesn't match the description in the subject line, or is it OK
RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address
Jens Axboe wrote: On Wed, Mar 16 2005, [EMAIL PROTECTED] wrote: Jens Axboe wrote: On Wed, Mar 16 2005, [EMAIL PROTECTED] wrote: Hayes, Stuart wrote: This patch will map the sg buffers to kernel virtual memory space in the functions idescsi_input_buffers() and idescsi_output_buffers(). Without this patch, idescsi passes a null pointer to atapi_input_bytes() and atapi_output_bytes() when sg pages are in high memory (i686 architecture). I'm attaching as a file, too, as the text will certainly be wrapped. (Sorry for the subject rename--I'm trying to use the correct format for patch emails.) Thanks Stuart And, while there's another high memory/kmap patch question on this list... Is there some reason nobody seems interested in this patch (except Jens--thanks for the help!)? I'm kind of new to sending in patches, and I'm not sure if I'm just not waiting long enough, or if there is a problem with this patch... But really, we're getting a null pointer dereference oops when using ATAPI tape drives (with ide-scsi) without this patch... Sorry, that did seem to get dropped on the floor. Actually I'm wondering why you are seeing highmem pages there in the first place, it would be easier/better just to limit ide-scsi to non-highmem pages. That would remove the need to add any work arounds in the driver. I think we're seeing highmem pages in the sg list because that's where the user memory was when st_write() was called. The sg list is set up when st_write(), which calls setup_buffering(), which calls st_map_user_pages()... this just sets up the sg pages to point directly to the user memory. So, by the time ide-scsi comes into the picture, the sg list is already set up to point to high memory pages. Are you suggesting that ide-scsi should change the dma_mask for the device so that st_map_user_pages() won't let sg pages point to high memory? Or is there something else I'm missing? I think that is a bug, this effectively bypasses whatever restrictions the scsi host adapter has said about memory limits. It is a problem because the request doesn't come from the block layer (which handles all of this). I would much prefer fixing that real issue! By whatever restrictions the scsi host adapter has said about memory limits, are you referring to the dma_boundary or the dma_mask? I'm don't know any other ways a host adapter can specify memory limits. I wasn't complete in my statement above, though--st_map_user_pages() does prevent the sg list from pointing to pages that are above the max_pfn for the device (it gets max_pfn from scsi_calculate_bounce_ limit()), and that appears to be working ok. But, even though max_pfn is 0xf (so that the maximum physical address of any sg page won't be over 4G (0x)), there are apparently high memory pages that are below physical address of 4G (0x), but are still considered high memory. So the entire first 4G of memory isn't low memory... for example, on my box, pfn 0xfbc3c, with physical address 0xfbc3c000, has the PG_highmem bit set in (page)-flags. Because of this, when ide-scsi needs to do PIO, it needs to do a kmap_atomic(). Am I completely missing your point? - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address
Hayes, Stuart wrote:
This patch will map the sg buffers to kernel virtual memory space in
the functions idescsi_input_buffers() and idescsi_output_buffers().
Without this patch, idescsi passes a null pointer to
atapi_input_bytes() and atapi_output_bytes() when sg pages are in
high memory (i686 architecture).
I'm attaching as a file, too, as the text will certainly be wrapped.
(Sorry for the subject rename--I'm trying to use the correct format
for patch emails.)
Thanks
Stuart
And, while there's another high memory/kmap patch question on this
list...
Is there some reason nobody seems interested in this patch (except
Jens--thanks for the help!)? I'm kind of new to sending in patches,
and I'm not sure if I'm just not waiting long enough, or if there is
a problem with this patch...
But really, we're getting a null pointer dereference oops when using
ATAPI tape drives (with ide-scsi) without this patch...
Thanks!
Stuart
Signed-off-by: Stuart Hayes [EMAIL PROTECTED]
--- linux-2.6.11.orig/drivers/scsi/ide-scsi.c 2005-03-09
14:13:48.0 -0500
+++ linux-2.6.11.new/drivers/scsi/ide-scsi.c 2005-03-09
13:56:38.0 -0500
@@ -143,6 +143,7 @@ static void idescsi_input_buffers (ide_d {
int count;
char *buf;
+ unsigned long flags;
while (bcount) {
if (pc-sg - (struct scatterlist *)
pc-scsi_cmd-request_buffer pc-scsi_cmd-use_sg) {
@@ -151,8 +152,11 @@ static void idescsi_input_buffers (ide_d
return;
}
count = min(pc-sg-length - pc-b_count, bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ local_irq_save(flags);
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_input_bytes(drive, buf + pc-b_count,
count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
+ local_irq_restore(flags);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
@@ -165,6 +169,7 @@ static void idescsi_output_buffers (ide_ {
int count;
char *buf;
+ unsigned long flags;
while (bcount) {
if (pc-sg - (struct scatterlist *)
pc-scsi_cmd-request_buffer pc-scsi_cmd-use_sg) {
@@ -173,8 +178,11 @@ static void idescsi_output_buffers (ide_
return;
}
count = min(pc-sg-length - pc-b_count, bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ local_irq_save(flags);
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_output_bytes(drive, buf +
pc-b_count, count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
+ local_irq_restore(flags);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address
Jens Axboe wrote: On Wed, Mar 16 2005, [EMAIL PROTECTED] wrote: Hayes, Stuart wrote: This patch will map the sg buffers to kernel virtual memory space in the functions idescsi_input_buffers() and idescsi_output_buffers(). Without this patch, idescsi passes a null pointer to atapi_input_bytes() and atapi_output_bytes() when sg pages are in high memory (i686 architecture). I'm attaching as a file, too, as the text will certainly be wrapped. (Sorry for the subject rename--I'm trying to use the correct format for patch emails.) Thanks Stuart And, while there's another high memory/kmap patch question on this list... Is there some reason nobody seems interested in this patch (except Jens--thanks for the help!)? I'm kind of new to sending in patches, and I'm not sure if I'm just not waiting long enough, or if there is a problem with this patch... But really, we're getting a null pointer dereference oops when using ATAPI tape drives (with ide-scsi) without this patch... Sorry, that did seem to get dropped on the floor. Actually I'm wondering why you are seeing highmem pages there in the first place, it would be easier/better just to limit ide-scsi to non-highmem pages. That would remove the need to add any work arounds in the driver. I think we're seeing highmem pages in the sg list because that's where the user memory was when st_write() was called. The sg list is set up when st_write(), which calls setup_buffering(), which calls st_map_user_pages()... this just sets up the sg pages to point directly to the user memory. So, by the time ide-scsi comes into the picture, the sg list is already set up to point to high memory pages. Are you suggesting that ide-scsi should change the dma_mask for the device so that st_map_user_pages() won't let sg pages point to high memory? Or is there something else I'm missing? Thanks, Stuart - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: ide-scsi oops with ide tape drive
Jens Axboe wrote:
On Tue, Mar 08 2005, [EMAIL PROTECTED] wrote:
Hello!
We're seeing a null pointer dereference with certain IDE tape drives
on
2.6.11 when we use it with ide-scsi (i686 architecture). The
problem is that the scatter-gather pages aren't mapped to kernel
virtual address space in
idescsi_output_buffers()/idescsi_input_buffers(), so, if these pages
are in high memory, page_address() returns a null pointer.
This patch fixes the problem. I'll attach it as a file, too, just in
case it gets mangled. Please let me know if there are any problems
with or questions regarding this patch.
Again, this patch is against 2.6.11.
Thanks!
Stuart Hayes
[EMAIL PROTECTED]
--- ide-scsi.c.orig 2005-03-08 13:44:38.0 -0500
+++ ide-scsi.c 2005-03-08 14:02:43.0 -0500
@@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
return; }
count = min(pc-sg-length - pc-b_count, bcount);
-buf = page_address(pc-sg-page) + pc-sg-offset;
+buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_input_bytes(drive, buf + pc-b_count,
count);
+kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
You need a local_irq_save(flags); ... local_irq_restore(flags); around
the kmap(atomic), transfer, and kunmap_atomic() for this to be safe.
Interrupts may not be disabled at this point, depends on drive
settings.
Thanks for the quick response! I'll look into this more carefully.
--Stuart
@@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
return; }
count = min(pc-sg-length - pc-b_count, bcount);
-buf = page_address(pc-sg-page) + pc-sg-offset;
+buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_output_bytes(drive, buf +
pc-b_count, count);
+kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
Ditto.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: ide-scsi oops with ide tape drive
Hayes, Stuart wrote:
Jens Axboe wrote:
On Tue, Mar 08 2005, [EMAIL PROTECTED] wrote:
Hello!
We're seeing a null pointer dereference with certain IDE tape
drives on
2.6.11 when we use it with ide-scsi (i686 architecture). The
problem is that the scatter-gather pages aren't mapped to kernel
virtual address space in
idescsi_output_buffers()/idescsi_input_buffers(), so, if these
pages are in high memory, page_address() returns a null pointer.
This patch fixes the problem. I'll attach it as a file, too, just
in case it gets mangled. Please let me know if there are any
problems with or questions regarding this patch.
Again, this patch is against 2.6.11.
Thanks!
Stuart Hayes
[EMAIL PROTECTED]
--- ide-scsi.c.orig 2005-03-08 13:44:38.0 -0500
+++ ide-scsi.c 2005-03-08 14:02:43.0 -0500
@@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
return; } count = min(pc-sg-length - pc-b_count,
bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_input_bytes(drive, buf + pc-b_count,
count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
You need a local_irq_save(flags); ... local_irq_restore(flags);
around the kmap(atomic), transfer, and kunmap_atomic() for this to
be safe. Interrupts may not be disabled at this point, depends on
drive settings.
Thanks for the quick response! I'll look into this more carefully.
--Stuart
Oh, yeah, unmaskirq...
Do you see any problem with masking local IRQs during the transfer? If
unmask is set, it could be because the device is a slow PIO device, so
transferring a whole page could take over 2.5ms.
I could check, and if unmask is set the page is in high memory, I
could copy the page down to low memory and allow the transfer to the
drive to proceed with irqs unmasked. Do you think it would be worth the
extra code complexity?
Thanks
Stuart
@@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
return; } count = min(pc-sg-length - pc-b_count,
bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_output_bytes(drive, buf +
pc-b_count, count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
Ditto.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: ide-scsi oops with ide tape drive
Hayes, Stuart wrote:
Jens Axboe wrote:
On Tue, Mar 08 2005, [EMAIL PROTECTED] wrote:
Hello!
We're seeing a null pointer dereference with certain IDE tape
drives on
2.6.11 when we use it with ide-scsi (i686 architecture). The
problem is that the scatter-gather pages aren't mapped to kernel
virtual address space in
idescsi_output_buffers()/idescsi_input_buffers(), so, if these
pages are in high memory, page_address() returns a null pointer.
This patch fixes the problem. I'll attach it as a file, too, just
in case it gets mangled. Please let me know if there are any
problems with or questions regarding this patch.
Again, this patch is against 2.6.11.
Thanks!
Stuart Hayes
[EMAIL PROTECTED]
--- ide-scsi.c.orig2005-03-08 13:44:38.0 -0500
+++ ide-scsi.c 2005-03-08 14:02:43.0 -0500
@@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
return; } count = min(pc-sg-length - pc-b_count,
bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_input_bytes(drive, buf + pc-b_count,
count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
You need a local_irq_save(flags); ... local_irq_restore(flags);
around the kmap(atomic), transfer, and kunmap_atomic() for this to
be safe. Interrupts may not be disabled at this point, depends on
drive settings.
Thanks for the quick response! I'll look into this more carefully.
--Stuart
Oh, yeah, unmaskirq...
Do you see any problem with masking local IRQs during the transfer?
If unmask is set, it could be because the device is a slow PIO
device, so transferring a whole page could take over 2.5ms.
I could check, and if unmask is set the page is in high memory, I
could copy the page down to low memory and allow the transfer to the
drive to proceed with irqs unmasked. Do you think it would be worth
the extra code complexity?
Thanks
Stuart
Oops, it's a worst case of ~1.2ms, not 2.5ms. I guess that would be OK.
I'll rework the patch and send it again in a bit.
@@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
return; } count = min(pc-sg-length - pc-b_count,
bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_output_bytes(drive, buf +
pc-b_count, count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
Ditto.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
ide-scsi oops with ide tape drive
Hello!
We're seeing a null pointer dereference with certain IDE tape drives on
2.6.11 when we use it with ide-scsi (i686 architecture). The problem is
that the scatter-gather pages aren't mapped to kernel virtual address
space in idescsi_output_buffers()/idescsi_input_buffers(), so, if these
pages are in high memory, page_address() returns a null pointer.
This patch fixes the problem. I'll attach it as a file, too, just in
case it gets mangled. Please let me know if there are any problems with
or questions regarding this patch.
Again, this patch is against 2.6.11.
Thanks!
Stuart Hayes
[EMAIL PROTECTED]
--- ide-scsi.c.orig 2005-03-08 13:44:38.0 -0500
+++ ide-scsi.c 2005-03-08 14:02:43.0 -0500
@@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
return;
}
count = min(pc-sg-length - pc-b_count, bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_input_bytes(drive, buf + pc-b_count,
count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
@@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
return;
}
count = min(pc-sg-length - pc-b_count, bcount);
- buf = page_address(pc-sg-page) + pc-sg-offset;
+ buf = kmap_atomic(pc-sg-page, KM_USER0) +
pc-sg-offset;
drive-hwif-atapi_output_bytes(drive, buf +
pc-b_count, count);
+ kunmap_atomic(buf - pc-sg-offset, KM_USER0);
bcount -= count; pc-b_count += count;
if (pc-b_count == pc-sg-length) {
pc-sg++;
ide-scsi.2.6.11.patch
Description: ide-scsi.2.6.11.patch
