[PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]


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


Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>


--- linux-2.6.11.orig/drivers/scsi/ide-scsi.c	2005-03-09
14:13:48.000000000 -0500
+++ linux-2.6.11.new/drivers/scsi/ide-scsi.c	2005-03-09
13:56:38.000000000 -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++;

[-- Attachment #2: ide-scsi.2.6.11.try3.patch --]
[-- Type: application/octet-stream, Size: 1658 bytes --]

--- linux-2.6.11.orig/drivers/scsi/ide-scsi.c	2005-03-09 14:13:48.000000000 -0500
+++ linux-2.6.11.new/drivers/scsi/ide-scsi.c	2005-03-09 13:56:38.000000000 -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++;

RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

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 <stuart_hayes@dell.com>
> 
> 
> --- linux-2.6.11.orig/drivers/scsi/ide-scsi.c	2005-03-09
> 14:13:48.000000000 -0500
> +++ linux-2.6.11.new/drivers/scsi/ide-scsi.c	2005-03-09
> 13:56:38.000000000 -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++;

Re: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Jens Axboe]

On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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.

-- 
Jens Axboe

RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

Jens Axboe wrote:
> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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

Re: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Jens Axboe]

On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
> Jens Axboe wrote:
> > On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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!

-- 
Jens Axboe

RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

Jens Axboe wrote:
> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
>> Jens Axboe wrote:
>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff (so that the maximum physical address of any sg
page won't be over 4G (0xffffffff)), there are apparently high 
memory pages that are below physical address of 4G (0xffffffff), 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?

Re: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Jens Axboe]

On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
> Jens Axboe wrote:
> > On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
> >> Jens Axboe wrote:
> >>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff (so that the maximum physical address of any sg
> page won't be over 4G (0xffffffff)), there are apparently high 
> memory pages that are below physical address of 4G (0xffffffff), 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.

-- 
Jens Axboe

Re: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Jens Axboe]

On Thu, Mar 17 2005, Jens Axboe wrote:
> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
> > Jens Axboe wrote:
> > > On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
> > >> Jens Axboe wrote:
> > >>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff (so that the maximum physical address of any sg
> > page won't be over 4G (0xffffffff)), there are apparently high 
> > memory pages that are below physical address of 4G (0xffffffff), 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()

-- 
Jens Axboe

RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

Jens Axboe wrote:
> On Thu, Mar 17 2005, Jens Axboe wrote:
>> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
>>> Jens Axboe wrote:
>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
>>>>> Jens Axboe wrote:
>>>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff (so that the maximum physical
>>> address of any sg page won't be over 4G (0xffffffff)), there are
>>> apparently high memory pages that are below physical address of 4G
>>> (0xffffffff), 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.

I'll try this out and send in a patch.

Thanks!

Re: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Jens Axboe]

On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
> Jens Axboe wrote:
> > On Thu, Mar 17 2005, Jens Axboe wrote:
> >> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
> >>> Jens Axboe wrote:
> >>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
> >>>>> Jens Axboe wrote:
> >>>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff (so that the maximum physical
> >>> address of any sg page won't be over 4G (0xffffffff)), there are
> >>> apparently high memory pages that are below physical address of 4G
> >>> (0xffffffff), 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 :)

-- 
Jens Axboe

RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

[-- Attachment #1: Type: text/plain, Size: 7338 bytes --]

Jens Axboe wrote:
> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
>> Jens Axboe wrote:
>>> On Thu, Mar 17 2005, Jens Axboe wrote:
>>>> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
>>>>> Jens Axboe wrote:
>>>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
>>>>>>> Jens Axboe wrote:
>>>>>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff (so that the
>>>>> maximum physical 
>>>>> address of any sg page won't be over 4G (0xffffffff)), there are
>>>>> apparently high memory pages that are below physical address of 4G
>>>>> (0xffffffff), 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 here?

I'll also attach as a file, though I'm optimistic that this one won't
be wrapped.

Thanks!
Stuart


Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>

diff -purN a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-15 19:09:08.000000000 -0500
+++ b/drivers/scsi/ide-scsi.c	2005-03-23 17:26:06.000000000 -0500
@@ -1048,6 +1048,7 @@ static int idescsi_attach(ide_drive_t *d
 		return 1;
 
 	host->max_id = 1;
+	//host->bounce_high = 1;
 
 #if IDESCSI_DEBUG_LOG
 	if (drive->id->last_lun)
diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	2005-03-15 19:09:06.000000000 -0500
+++ b/drivers/scsi/scsi_lib.c	2005-03-23 17:26:04.000000000 -0500
@@ -1333,6 +1333,9 @@ u64 scsi_calculate_bounce_limit(struct S
 
 	if (shost->unchecked_isa_dma)
 		return BLK_BOUNCE_ISA;
+
+	if (shost->bounce_high)
+		return BLK_BOUNCE_HIGH;
 	/*
 	 * Platforms with virtual-DMA translation
 	 * hardware have no practical limit.
diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	2005-03-15 19:09:01.000000000 -0500
+++ b/include/scsi/scsi_host.h	2005-03-23 17:26:54.000000000 -0500
@@ -502,6 +502,12 @@ struct Scsi_Host {
 	unsigned reverse_ordering:1;
 
 	/*
+	 * Host needs all high memory bounced (useful for ide-scsi,
+	 * so it doesn't have to kmap when doing PIO)
+	 */
+	unsigned bounce_high:1;
+
+	/*
 	 * Host has rejected a command because it was busy.
 	 */
 	unsigned int host_blocked;



[-- Attachment #2: ide-scsi-bouncehighmem-2.6.11.5.patch --]
[-- Type: application/octet-stream, Size: 1348 bytes --]

diff -purN a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-15 19:09:08.000000000 -0500
+++ b/drivers/scsi/ide-scsi.c	2005-03-23 17:26:06.000000000 -0500
@@ -1048,6 +1048,7 @@ static int idescsi_attach(ide_drive_t *d
 		return 1;
 
 	host->max_id = 1;
+	//host->bounce_high = 1;
 
 #if IDESCSI_DEBUG_LOG
 	if (drive->id->last_lun)
diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	2005-03-15 19:09:06.000000000 -0500
+++ b/drivers/scsi/scsi_lib.c	2005-03-23 17:26:04.000000000 -0500
@@ -1333,6 +1333,9 @@ u64 scsi_calculate_bounce_limit(struct S
 
 	if (shost->unchecked_isa_dma)
 		return BLK_BOUNCE_ISA;
+
+	if (shost->bounce_high)
+		return BLK_BOUNCE_HIGH;
 	/*
 	 * Platforms with virtual-DMA translation
 	 * hardware have no practical limit.
diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	2005-03-15 19:09:01.000000000 -0500
+++ b/include/scsi/scsi_host.h	2005-03-23 17:26:54.000000000 -0500
@@ -502,6 +502,12 @@ struct Scsi_Host {
 	unsigned reverse_ordering:1;
 
 	/*
+	 * Host needs all high memory bounced (useful for ide-scsi,
+	 * so it doesn't have to kmap when doing PIO)
+	 */
+	unsigned bounce_high:1;
+
+	/*
 	 * Host has rejected a command because it was busy.
 	 */
 	unsigned int host_blocked;

RE: [PATCH 2.6.11] ide-scsi: map sg buffers in idescsi_input/output_buffers() to kernel virtual address

[Stuart_Hayes]

Hayes, Stuart wrote:
> Jens Axboe wrote:
>> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
>>> Jens Axboe wrote:
>>>> On Thu, Mar 17 2005, Jens Axboe wrote:
>>>>> On Thu, Mar 17 2005, Stuart_Hayes@Dell.com wrote:
>>>>>> Jens Axboe wrote:
>>>>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com wrote:
>>>>>>>> Jens Axboe wrote:
>>>>>>>>> On Wed, Mar 16 2005, Stuart_Hayes@Dell.com 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 0xfffff
>>>>>> (so that the maximum physical address of any sg page won't be
>>>>>> over 4G (0xffffffff)), there are apparently high memory pages
>>>>>> that are below physical address of 4G (0xffffffff), 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 here? 
> 
> I'll also attach as a file, though I'm optimistic that this one won't
> be wrapped. 
> 
> Thanks!
> Stuart
> 
> 
> Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>
> 
> diff -purN a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
> --- a/drivers/scsi/ide-scsi.c	2005-03-15 19:09:08.000000000 -0500
> +++ b/drivers/scsi/ide-scsi.c	2005-03-23 17:26:06.000000000 -0500
> @@ -1048,6 +1048,7 @@ static int idescsi_attach(ide_drive_t *d
>  		return 1;
> 
>  	host->max_id = 1;
> +	//host->bounce_high = 1;
> 
>  #if IDESCSI_DEBUG_LOG
>  	if (drive->id->last_lun)
> diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> --- a/drivers/scsi/scsi_lib.c	2005-03-15 19:09:06.000000000 -0500
> +++ b/drivers/scsi/scsi_lib.c	2005-03-23 17:26:04.000000000 -0500
> @@ -1333,6 +1333,9 @@ u64 scsi_calculate_bounce_limit(struct S
> 
>  	if (shost->unchecked_isa_dma)
>  		return BLK_BOUNCE_ISA;
> +
> +	if (shost->bounce_high)
> +		return BLK_BOUNCE_HIGH;
>  	/*
>  	 * Platforms with virtual-DMA translation
>  	 * hardware have no practical limit.
> diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> --- a/include/scsi/scsi_host.h	2005-03-15 19:09:01.000000000
-0500
> +++ b/include/scsi/scsi_host.h	2005-03-23 17:26:54.000000000
-0500
> @@ -502,6 +502,12 @@ struct Scsi_Host {
>  	unsigned reverse_ordering:1;
> 
>  	/*
> +	 * Host needs all high memory bounced (useful for ide-scsi,
> +	 * so it doesn't have to kmap when doing PIO)
> +	 */
> +	unsigned bounce_high:1;
> +
> +	/*
>  	 * Host has rejected a command because it was busy.
>  	 */
>  	unsigned int host_blocked;


It occurred to me that it might be possible for the device to have a 
greater restriction on the DMA memory than BLK_BOUNCE_HIGH, so I changed

the scsi_calculate_bounce_limit to make shost->bounce_high an upper 
limit on the bounce limit.

I also uncommented the "host->bounce_high=1" in ide-scsi.c... :)

I'm personally still partial to my original patch 
(http://marc.theaimsgroup.com/?l=linux-scsi&m=111040058219417) to fix 
this problem, but I guess it's six of one...

And, feel free to tell me if there's still something wrong with this!


Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>

diff -purN a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-25 22:28:23.000000000 -0500
+++ b/drivers/scsi/ide-scsi.c	2005-03-31 15:17:38.000000000 -0500
@@ -1048,6 +1048,7 @@ static int idescsi_attach(ide_drive_t *d
 		return 1;
 
 	host->max_id = 1;
+	host->bounce_high = 1;
 
 #if IDESCSI_DEBUG_LOG
 	if (drive->id->last_lun)
diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	2005-03-25 22:28:21.000000000 -0500
+++ b/drivers/scsi/scsi_lib.c	2005-03-31 15:21:35.000000000 -0500
@@ -1344,6 +1344,9 @@ u64 scsi_calculate_bounce_limit(struct S
 	if (host_dev && host_dev->dma_mask)
 		bounce_limit = *host_dev->dma_mask;
 
+	if (shost->bounce_high && (bounce_limit > BLK_BOUNCE_HIGH))
+		return BLK_BOUNCE_HIGH;
+
 	return bounce_limit;
 }
 EXPORT_SYMBOL(scsi_calculate_bounce_limit);
diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	2005-03-25 22:28:16.000000000 -0500
+++ b/include/scsi/scsi_host.h	2005-03-31 15:16:54.000000000 -0500
@@ -502,6 +502,12 @@ struct Scsi_Host {
 	unsigned reverse_ordering:1;
 
 	/*
+	 * Host needs all high memory bounced (useful for ide-scsi,
+	 * so it doesn't have to kmap when doing PIO)
+	 */
+	unsigned bounce_high:1;
+
+	/*
 	 * Host has rejected a command because it was busy.
 	 */
 	unsigned int host_blocked;