RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

Jens Axboe wrote:
> On Fri, Apr 29 2005, Stuart_Hayes@Dell.com wrote:
>>> With more testing, I've discovered a problem with thin patch--I used
>>> the "KM_USER0" window for kmap_atomic(), but that's apparently not
>>> safe to use in an interrupt handler, because there are places in the
>>> kernel where KM_USER0 is used without masking interrupts (see
>>> include/linux/highmem.h for several examples).  This patch is
>>> identical to the previous one I sent in, except it uses the KM_IRQ0
>>> window, which I verified is not used anywhere without IRQs masked.
>>> 
>>> This patch is against 2.6.11.7... the latest kernel on kernel.org
>>> didn't yet have the previous patch I submitted.  If it would be more
>>> useful, I can make this patch against an ide-scsi.c that already has
>>> my previous patch applied--I wasn't sure which would be better.
> 
> (you should inline the patch so one can comment on it...)
> 
> The IO code typically uses the BIO defines for this.
> 
> diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c
> linux-2.6.11.7mods/drivers/scsi/ide-scsi.c ---
> linux-2.6.11.7/drivers/scsi/ide-scsi.c	2005-04-07
14:57:46.000000000
> -0400 +++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c	2005-04-29
>  	11:46:55.000000000 -0400 @@ -143,6 +143,9 @@ static void
>  	idescsi_input_buffers (ide_d  { int count; char *buf;
> +#ifdef CONFIG_HIGHMEM
> +	unsigned long flags;
> +#endif
> 
>  	while (bcount) {
>  		if (pc->sg - (struct scatterlist *)
pc->scsi_cmd->request_buffer >
>  			pc->scsi_cmd->use_sg) { @@ -151,8 +154,15 @@
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;
> +#ifdef CONFIG_HIGHMEM
> +		local_irq_save(flags);
> +#endif
> 
> This is just aweful, don't add tons of ifdefs. If you must
> differentiate, just do something ala: 
> 
>         if (PageHighMem(page)) {
>                 save interrupts
>                 buf = kmap
>         } else
>                 buf = page_address(xxx).
> 
> and so on. But why are you still doing it this way? Did we not agree
> that adding a host template no-high-mem defines was way better?? 

The ifdefs were at Bart's request.

The reason I went back to the kmap_atomic() fix instead of adding a  
no-high-mem flag in the host is that I couldn't get James to accept that

patch, while this one is ide-scsi only, and Bart did accept it.  And, to
be honest, I wasn't really sure why the kmap_atomic() fix was inferior.

But I'd certainly defer to your (or Bart's) judgment on those things--
you guys have a lot more experience than me with the kernel.  I am
hesitant to change the "ifdef"s to "if"s at this point, since Bart
already accepted it (previously) with the ifdefs, but I'd be more than 
happy to change it if he agrees.

Thanks for the comments,
Stuart

Re: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Bartlomiej Zolnierkiewicz]

Hi,

On 5/2/05, Stuart_Hayes@dell.com <Stuart_Hayes@dell.com> wrote:
> Jens Axboe wrote:
> > On Fri, Apr 29 2005, Stuart_Hayes@Dell.com wrote:
> >>> With more testing, I've discovered a problem with thin patch--I used
> >>> the "KM_USER0" window for kmap_atomic(), but that's apparently not
> >>> safe to use in an interrupt handler, because there are places in the
> >>> kernel where KM_USER0 is used without masking interrupts (see
> >>> include/linux/highmem.h for several examples).  This patch is
> >>> identical to the previous one I sent in, except it uses the KM_IRQ0
> >>> window, which I verified is not used anywhere without IRQs masked.
> >>>
> >>> This patch is against 2.6.11.7... the latest kernel on kernel.org
> >>> didn't yet have the previous patch I submitted.  If it would be more
> >>> useful, I can make this patch against an ide-scsi.c that already has
> >>> my previous patch applied--I wasn't sure which would be better.
> >
> > (you should inline the patch so one can comment on it...)
> >
> > The IO code typically uses the BIO defines for this.
> >
> > diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c
> > linux-2.6.11.7mods/drivers/scsi/ide-scsi.c ---
> > linux-2.6.11.7/drivers/scsi/ide-scsi.c        2005-04-07
> 14:57:46.000000000
> > -0400 +++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c  2005-04-29
> >       11:46:55.000000000 -0400 @@ -143,6 +143,9 @@ static void
> >       idescsi_input_buffers (ide_d  { int count; char *buf;
> > +#ifdef CONFIG_HIGHMEM
> > +     unsigned long flags;
> > +#endif
> >
> >       while (bcount) {
> >               if (pc->sg - (struct scatterlist *)
> pc->scsi_cmd->request_buffer >
> >                       pc->scsi_cmd->use_sg) { @@ -151,8 +154,15 @@
> 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;
> > +#ifdef CONFIG_HIGHMEM
> > +             local_irq_save(flags);
> > +#endif
> >
> > This is just aweful, don't add tons of ifdefs. If you must
> > differentiate, just do something ala:
> >
> >         if (PageHighMem(page)) {
> >                 save interrupts
> >                 buf = kmap
> >         } else
> >                 buf = page_address(xxx).
> >
> > and so on. But why are you still doing it this way? Did we not agree
> > that adding a host template no-high-mem defines was way better??
> 
> The ifdefs were at Bart's request.

Yep, ifdefs are my fault.

Thanks for PageHighMem() hint Jens, we should use it also for ide-taskfile.c.

> The reason I went back to the kmap_atomic() fix instead of adding a
> no-high-mem flag in the host is that I couldn't get James to accept that

What is no-high-mem flag?

> patch, while this one is ide-scsi only, and Bart did accept it.  And, to
> be honest, I wasn't really sure why the kmap_atomic() fix was inferior.
> 
> But I'd certainly defer to your (or Bart's) judgment on those things--
> you guys have a lot more experience than me with the kernel.  I am
> hesitant to change the "ifdef"s to "if"s at this point, since Bart
> already accepted it (previously) with the ifdefs, but I'd be more than
> happy to change it if he agrees.

I agree ;-)

Cheers,
Bartlomiej

Re: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Jens Axboe]

On Mon, May 02 2005, Bartlomiej Zolnierkiewicz wrote:
> > The reason I went back to the kmap_atomic() fix instead of adding a
> > no-high-mem flag in the host is that I couldn't get James to accept that
> 
> What is no-high-mem flag?

In the host_template, to force scsi_calculate_bounce_limit() to set the
bounce address at the max_low_pfn point.

-- 
Jens Axboe

Re: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Jens Axboe]

On Mon, May 02 2005, Stuart_Hayes@Dell.com wrote:
> >  		idescsi_input_buffers (ide_d return; }
> >  		count = min(pc->sg->length - pc->b_count, bcount);
> > -		buf = page_address(pc->sg->page) + pc->sg->offset;
> > +#ifdef CONFIG_HIGHMEM
> > +		local_irq_save(flags);
> > +#endif
> > 
> > This is just aweful, don't add tons of ifdefs. If you must
> > differentiate, just do something ala: 
> > 
> >         if (PageHighMem(page)) {
> >                 save interrupts
> >                 buf = kmap
> >         } else
> >                 buf = page_address(xxx).
> > 
> > and so on. But why are you still doing it this way? Did we not agree
> > that adding a host template no-high-mem defines was way better?? 
> 
> The ifdefs were at Bart's request.
> 
> The reason I went back to the kmap_atomic() fix instead of adding a  
> no-high-mem flag in the host is that I couldn't get James to accept that

But, why? The no_highmem flag doesn't make sense for regular host
adapters, they can just set pdev->dma_mask appropriately. But we cannot
do that for ide-scsi, so I think it would be acceptable to add it
because of that.

> patch, while this one is ide-scsi only, and Bart did accept it.  And, to
> be honest, I wasn't really sure why the kmap_atomic() fix was inferior.
> 
> But I'd certainly defer to your (or Bart's) judgment on those things--
> you guys have a lot more experience than me with the kernel.  I am
> hesitant to change the "ifdef"s to "if"s at this point, since Bart
> already accepted it (previously) with the ifdefs, but I'd be more than 
> happy to change it if he agrees.

The PageHighMem variant would be acceptable to me, although I still
prefer the highmem bounce approach.

-- 
Jens Axboe

RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

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

Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On 5/2/05, Stuart_Hayes@dell.com <Stuart_Hayes@dell.com> wrote:
>> Jens Axboe wrote:
>>> On Fri, Apr 29 2005, Stuart_Hayes@Dell.com wrote:
>>>>> With more testing, I've discovered a problem with thin patch--I
>>>>> used the "KM_USER0" window for kmap_atomic(), but that's
>>>>> apparently not safe to use in an interrupt handler, because there
>>>>> are places in the kernel where KM_USER0 is used without masking
>>>>> interrupts (see include/linux/highmem.h for several examples).
>>>>> This patch is identical to the previous one I sent in, except it
>>>>> uses the KM_IRQ0 window, which I verified is not used anywhere
>>>>> without IRQs masked. 
>>>>> 
>>>>> This patch is against 2.6.11.7... the latest kernel on kernel.org
>>>>> didn't yet have the previous patch I submitted.  If it would be
>>>>> more useful, I can make this patch against an ide-scsi.c that
>>>>> already has my previous patch applied--I wasn't sure which would
>>>>> be better. 
>>> 
>>> (you should inline the patch so one can comment on it...)
>>> 
>>> The IO code typically uses the BIO defines for this.
>>> 
>>> diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c
>>> linux-2.6.11.7mods/drivers/scsi/ide-scsi.c ---
>>> linux-2.6.11.7/drivers/scsi/ide-scsi.c        2005-04-07
>>> 14:57:46.000000000 -0400 +++
>>>       linux-2.6.11.7mods/drivers/scsi/ide-scsi.c  2005-04-29
>>>       11:46:55.000000000 -0400 @@ -143,6 +143,9 @@ static void
>>> idescsi_input_buffers (ide_d  { int count; char *buf; +#ifdef
>>> CONFIG_HIGHMEM +     unsigned long flags; +#endif
>>> 
>>>       while (bcount) {
>>>               if (pc->sg - (struct scatterlist *)
>>>                       pc->scsi_cmd->request_buffer >
>>>               pc->scsi_cmd->use_sg) { @@ -151,8 +154,15 @@ 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; +#ifdef CONFIG_HIGHMEM
>>> +             local_irq_save(flags); #endif 
>>> 
>>> This is just aweful, don't add tons of ifdefs. If you must
>>> differentiate, just do something ala:
>>> 
>>>         if (PageHighMem(page)) {
>>>                 save interrupts
>>>                 buf = kmap
>>>         } else
>>>                 buf = page_address(xxx).
>>> 
>>> and so on. But why are you still doing it this way? Did we not agree
>>> that adding a host template no-high-mem defines was way better??
>> 
>> The ifdefs were at Bart's request.
> 
> Yep, ifdefs are my fault.
> 
> Thanks for PageHighMem() hint Jens, we should use it also for
> ide-taskfile.c. 
> 
>> The reason I went back to the kmap_atomic() fix instead of adding a
>> no-high-mem flag in the host is that I couldn't get James to accept
>> that
> 
> What is no-high-mem flag?
> 
>> patch, while this one is ide-scsi only, and Bart did accept it.  And,
>> to be honest, I wasn't really sure why the kmap_atomic() fix was
>> inferior. 
>> 
>> But I'd certainly defer to your (or Bart's) judgment on those
>> things-- you guys have a lot more experience than me with the
>> kernel.  I am hesitant to change the "ifdef"s to "if"s at this
>> point, since Bart already accepted it (previously) with the ifdefs,
>> but I'd be more than happy to change it if he agrees.
> 
> I agree ;-)
> 
> Cheers,
> Bartlomiej
> -

How does this look?  I've tested it successfully.  (The attached
file should be the same as the code below, except for wrapping...)


diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c
linux-2.6.11.7mods/drivers/scsi/ide-scsi.c
--- linux-2.6.11.7/drivers/scsi/ide-scsi.c	2005-04-07
14:57:46.000000000 -0400
+++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c	2005-05-02
15:48:40.000000000 -0400
@@ -151,8 +151,17 @@ 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;
-		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
count);
+		if (PageHighMem(pc->sg->page)) {
+			unsigned long flags;
+			local_irq_save(flags);
+			buf = kmap_atomic(pc->sg->page, KM_IRQ0) +
pc->sg->offset;
+			drive->hwif->atapi_input_bytes(drive, buf +
pc->b_count, count);
+			kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+			local_irq_restore(flags);
+		} else {
+			buf = page_address(pc->sg->page) +
pc->sg->offset;
+			drive->hwif->atapi_input_bytes(drive, buf +
pc->b_count, count);
+		}
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -173,8 +182,17 @@ 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;
-		drive->hwif->atapi_output_bytes(drive, buf +
pc->b_count, count);
+		if (PageHighMem(pc->sg->page)) {
+			unsigned long flags;
+			local_irq_save(flags);
+			buf = kmap_atomic(pc->sg->page, KM_IRQ0) +
pc->sg->offset;
+			drive->hwif->atapi_output_bytes(drive, buf +
pc->b_count, count);
+			kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+			local_irq_restore(flags);
+		} else {
+			buf = page_address(pc->sg->page) +
pc->sg->offset;
+			drive->hwif->atapi_output_bytes(drive, buf +
pc->b_count, count);
+		}
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;


[-- Attachment #2: ide-scsi.2.6.11.7.try6.patch --]
[-- Type: application/octet-stream, Size: 1842 bytes --]

diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c linux-2.6.11.7mods/drivers/scsi/ide-scsi.c
--- linux-2.6.11.7/drivers/scsi/ide-scsi.c	2005-04-07 14:57:46.000000000 -0400
+++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c	2005-05-02 15:48:40.000000000 -0400
@@ -151,8 +151,17 @@ 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;
-		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count, count);
+		if (PageHighMem(pc->sg->page)) {
+			unsigned long flags;
+			local_irq_save(flags);
+			buf = kmap_atomic(pc->sg->page, KM_IRQ0) + pc->sg->offset;
+			drive->hwif->atapi_input_bytes(drive, buf + pc->b_count, count);
+			kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+			local_irq_restore(flags);
+		} else {
+			buf = page_address(pc->sg->page) + pc->sg->offset;
+			drive->hwif->atapi_input_bytes(drive, buf + pc->b_count, count);
+		}
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -173,8 +182,17 @@ 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;
-		drive->hwif->atapi_output_bytes(drive, buf + pc->b_count, count);
+		if (PageHighMem(pc->sg->page)) {
+			unsigned long flags;
+			local_irq_save(flags);
+			buf = kmap_atomic(pc->sg->page, KM_IRQ0) + pc->sg->offset;
+			drive->hwif->atapi_output_bytes(drive, buf + pc->b_count, count);
+			kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+			local_irq_restore(flags);
+		} else {
+			buf = page_address(pc->sg->page) + pc->sg->offset;
+			drive->hwif->atapi_output_bytes(drive, buf + pc->b_count, count);
+		}
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;

RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On 5/2/05, Stuart_Hayes@dell.com <Stuart_Hayes@dell.com> wrote:
>> Jens Axboe wrote:
>>> On Fri, Apr 29 2005, Stuart_Hayes@Dell.com wrote:
>>>>> With more testing, I've discovered a problem with thin patch--I
>>>>> used the "KM_USER0" window for kmap_atomic(), but that's
>>>>> apparently not safe to use in an interrupt handler, because there
>>>>> are places in the kernel where KM_USER0 is used without masking
>>>>> interrupts (see include/linux/highmem.h for several examples).
>>>>> This patch is identical to the previous one I sent in, except it
>>>>> uses the KM_IRQ0 window, which I verified is not used anywhere
>>>>> without IRQs masked. 
>>>>> 
>>>>> This patch is against 2.6.11.7... the latest kernel on kernel.org
>>>>> didn't yet have the previous patch I submitted.  If it would be
>>>>> more useful, I can make this patch against an ide-scsi.c that
>>>>> already has my previous patch applied--I wasn't sure which would
>>>>> be better. 
>>> 
>>> (you should inline the patch so one can comment on it...)
>>> 
>>> The IO code typically uses the BIO defines for this.
>>> 
>>> diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c
>>> linux-2.6.11.7mods/drivers/scsi/ide-scsi.c ---
>>> linux-2.6.11.7/drivers/scsi/ide-scsi.c        2005-04-07
>>> 14:57:46.000000000 -0400 +++
>>>       linux-2.6.11.7mods/drivers/scsi/ide-scsi.c  2005-04-29
>>>       11:46:55.000000000 -0400 @@ -143,6 +143,9 @@ static void
>>> idescsi_input_buffers (ide_d  { int count; char *buf; +#ifdef
>>> CONFIG_HIGHMEM +     unsigned long flags; +#endif
>>> 
>>>       while (bcount) {
>>>               if (pc->sg - (struct scatterlist *)
>>>                       pc->scsi_cmd->request_buffer >
>>>               pc->scsi_cmd->use_sg) { @@ -151,8 +154,15 @@ 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; +#ifdef CONFIG_HIGHMEM
>>> +             local_irq_save(flags); #endif 
>>> 
>>> This is just aweful, don't add tons of ifdefs. If you must
>>> differentiate, just do something ala:
>>> 
>>>         if (PageHighMem(page)) {
>>>                 save interrupts
>>>                 buf = kmap
>>>         } else
>>>                 buf = page_address(xxx).
>>> 
>>> and so on. But why are you still doing it this way? Did we not agree
>>> that adding a host template no-high-mem defines was way better??
>> 
>> The ifdefs were at Bart's request.
> 
> Yep, ifdefs are my fault.
> 
> Thanks for PageHighMem() hint Jens, we should use it also for
> ide-taskfile.c. 
> 
>> The reason I went back to the kmap_atomic() fix instead of adding a
>> no-high-mem flag in the host is that I couldn't get James to accept
>> that
> 
> What is no-high-mem flag?

When I first submitted this patch (to linux-scsi--I wasn't aware at the
time that ide-scsi was maintained as part of ide), Jens suggested a
different way of fixing this problem.  Instead of using kmap_atomic() in
the ide-scsi interrupt handler when doing PIO, he suggested adding a
"bounce_high" flag to the "struct Scsi_Host".  This flag would be set in
idescsi_attach() when the host was being set up.  Then,
scsi_calculate_bounce_limit() would be modified to return
BLK_BOUNCE_HIGH if this flag was set (and the bounce limit wasn't
already set lower than that), so that any scatter gather pages that were
in high memory would use bounce buffers.  That way, a kmap_atomic() in
the ide-scsi interrupt handler wouldn't be necessary when doing PIO.

I have a patch to do that, but it modifies the files ide-scsi.c,
scsi_lib.c, and scsi_host.h.  Here it is in case my description wasn't
clear:

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;

> 
>> patch, while this one is ide-scsi only, and Bart did accept it.  And,
>> to be honest, I wasn't really sure why the kmap_atomic() fix was
>> inferior. 
>> 
>> But I'd certainly defer to your (or Bart's) judgment on those
>> things-- you guys have a lot more experience than me with the
>> kernel.  I am hesitant to change the "ifdef"s to "if"s at this
>> point, since Bart already accepted it (previously) with the ifdefs,
>> but I'd be more than happy to change it if he agrees.
> 
> I agree ;-)
> 
> Cheers,
> Bartlomiej
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide"
> in the body of a message to majordomo@vger.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html  

RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

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

Hayes, Stuart wrote:
> Bartlomiej Zolnierkiewicz wrote:
>> On Apr 5, 2005 11:08 PM, Stuart_Hayes@dell.com
>> <Stuart_Hayes@dell.com> wrote: 
>> 
>>> How is this?  I already had the "flags" declarations at the
>>> beginning of the functions, but I wrapped them and the irq
>>> disable/enables in the
>> 
>> I mean at the very beginning but lets leave it as it is.
>> 
>>> HIGHMEM ifdef, like ide_pio_sector() in ide-taskfile.c.
>> 
>> thanks
>> 
>> Bartlomiej
> 
> Bart --
> 
> With more testing, I've discovered a problem with thin patch--I used
> the "KM_USER0" window for kmap_atomic(), but that's apparently not
> safe to use in an interrupt handler, because there are places in the
> kernel where KM_USER0 is used without masking interrupts (see
> include/linux/highmem.h for several examples).  This patch is
> identical to the previous one I sent in, except it uses the KM_IRQ0
> window, which I verified is not used anywhere without IRQs masked.   
> 
> This patch is against 2.6.11.7... the latest kernel on kernel.org
> didn't yet have the previous patch I submitted.  If it would be more
> useful, I can make this patch against an ide-scsi.c that already has
> my previous patch applied--I wasn't sure which would be better.   
> 
> Thanks!
> Stuart

My apologies--I just sent the wrong patch.  Here's the correct patch.

Stuart


[-- Attachment #2: idescsi-kmap-c-2.6.11.7.patch --]
[-- Type: application/octet-stream, Size: 1932 bytes --]

diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c linux-2.6.11.7mods/drivers/scsi/ide-scsi.c
--- linux-2.6.11.7/drivers/scsi/ide-scsi.c	2005-04-07 14:57:46.000000000 -0400
+++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c	2005-04-29 11:46:55.000000000 -0400
@@ -143,6 +143,9 @@ static void idescsi_input_buffers (ide_d
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -151,8 +154,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif
+		buf = kmap_atomic(pc->sg->page, KM_IRQ0) + pc->sg->offset;
 		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+#ifdef CONFIG_HIGHMEM
+		local_irq_restore(flags);
+#endif
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -165,6 +175,9 @@ static void idescsi_output_buffers (ide_
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -173,8 +186,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif
+		buf = kmap_atomic(pc->sg->page, KM_IRQ0) + pc->sg->offset;
 		drive->hwif->atapi_output_bytes(drive, buf + pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+#ifdef CONFIG_HIGHMEM
+		local_irq_restore(flags);
+#endif
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;

Re: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Jens Axboe]

On Fri, Apr 29 2005, Stuart_Hayes@Dell.com wrote:
> > With more testing, I've discovered a problem with thin patch--I used
> > the "KM_USER0" window for kmap_atomic(), but that's apparently not
> > safe to use in an interrupt handler, because there are places in the
> > kernel where KM_USER0 is used without masking interrupts (see
> > include/linux/highmem.h for several examples).  This patch is
> > identical to the previous one I sent in, except it uses the KM_IRQ0
> > window, which I verified is not used anywhere without IRQs masked.   
> > 
> > This patch is against 2.6.11.7... the latest kernel on kernel.org
> > didn't yet have the previous patch I submitted.  If it would be more
> > useful, I can make this patch against an ide-scsi.c that already has
> > my previous patch applied--I wasn't sure which would be better.   

(you should inline the patch so one can comment on it...)

The IO code typically uses the BIO defines for this.

diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c linux-2.6.11.7mods/drivers/scsi/ide-scsi.c
--- linux-2.6.11.7/drivers/scsi/ide-scsi.c	2005-04-07 14:57:46.000000000 -0400
+++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c	2005-04-29 11:46:55.000000000 -0400
@@ -143,6 +143,9 @@ static void idescsi_input_buffers (ide_d
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -151,8 +154,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif

This is just aweful, don't add tons of ifdefs. If you must
differentiate, just do something ala:

        if (PageHighMem(page)) {
                save interrupts
                buf = kmap
        } else
                buf = page_address(xxx).

and so on. But why are you still doing it this way? Did we not agree
that adding a host template no-high-mem defines was way better??

-- 
Jens Axboe

RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

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

Bartlomiej Zolnierkiewicz wrote:
> On Apr 5, 2005 11:08 PM, Stuart_Hayes@dell.com
> <Stuart_Hayes@dell.com> wrote: 
> 
>> How is this?  I already had the "flags" declarations at the beginning
>> of the functions, but I wrapped them and the irq disable/enables in
>> the
> 
> I mean at the very beginning but lets leave it as it is.
> 
>> HIGHMEM ifdef, like ide_pio_sector() in ide-taskfile.c.
> 
> thanks
> 
> Bartlomiej

Bart --

With more testing, I've discovered a problem with thin patch--I used the
"KM_USER0" window for kmap_atomic(), but that's apparently not safe to
use in an interrupt handler, because there are places in the kernel
where KM_USER0 is used without masking interrupts (see
include/linux/highmem.h for several examples).  This patch is identical
to the previous one I sent in, except it uses the KM_IRQ0 window, which 
I verified is not used anywhere without IRQs masked.

This patch is against 2.6.11.7... the latest kernel on kernel.org didn't
yet have the previous patch I submitted.  If it would be more useful, I
can make this patch against an ide-scsi.c that already has my previous
patch applied--I wasn't sure which would be better.

Thanks!
Stuart



[-- Attachment #2: ide-scsi.2.6.11.7.try5.patch --]
[-- Type: application/octet-stream, Size: 1746 bytes --]

diff -purN linux-2.6.11.7/drivers/scsi/ide-scsi.c linux-2.6.11.7mods/drivers/scsi/ide-scsi.c
--- linux-2.6.11.7/drivers/scsi/ide-scsi.c	2005-04-07 14:57:46.000000000 -0400
+++ linux-2.6.11.7mods/drivers/scsi/ide-scsi.c	2005-04-29 11:12:22.000000000 -0400
@@ -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_IRQ0) + pc->sg->offset;
 		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+		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_IRQ0) + pc->sg->offset;
 		drive->hwif->atapi_output_bytes(drive, buf + pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_IRQ0);
+		local_irq_restore(flags);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;

RE: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

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

> Hi,
> 
> On Apr 5, 2005 9:12 PM, Stuart_Hayes@dell.com <Stuart_Hayes@dell.com>
> wrote: 
>> 
>> The system can panic with a null pointer dereference using ide-scsi
>> if PIO is being done on scatter gather pages that are in high memory,
>> because page_address() returns 0.  We are actually seeing this using
>> a tape drive.  This patch will kmap_atomic() the pages before
>> performing PIO. 
>> 
>> I'm copying the text here, but I'm also attaching it as a file,
>> because the text will get garbled with extra line breaks.
>> 
>> Please let me know if there are any problems with the patch!
> 
> Looks fine, I have two small requests:
> * please don't disable/enable local IRQs for !HIGHMEM case
>   (see ide_pio_sector() in ide-taskfile.c)
> * declare 'unsigned long flags' at the beginning of the function
> 
>> (I sent this patch to linux-scsi a few weeks ago, and I was finally
>> told that it is maintained by the IDE folks (Bartlomiej
>> Zolnierkiewicz), not the SCSI folks...)
> 
> I'm not on linux-scsi, *sigh*, somebody could have simply forwarded
> it to me... 
> 
> Thanks!
> Bartlomiej

How is this?  I already had the "flags" declarations at the beginning of
the functions, but I wrapped them and the irq disable/enables in the
HIGHMEM ifdef, like ide_pio_sector() in ide-taskfile.c.

Thanks again,
Stuart


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

diff -purN a/drivers/scsi/ide-scsi.c b2/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-25 22:28:23.000000000 -0500
+++ b2/drivers/scsi/ide-scsi.c	2005-04-05 15:56:48.000000000 -0400
@@ -143,6 +143,9 @@ static void idescsi_input_buffers (ide_d
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *)
pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -151,8 +154,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif
+		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);
+#ifdef CONFIG_HIGHMEM
+		local_irq_restore(flags);
+#endif
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -165,6 +175,9 @@ static void idescsi_output_buffers (ide_
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *)
pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -173,8 +186,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif
+		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);
+#ifdef CONFIG_HIGHMEM
+		local_irq_restore(flags);
+#endif
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;



[-- Attachment #2: idescsi-kmap-b-2.6.11.6.patch --]
[-- Type: application/octet-stream, Size: 1878 bytes --]

diff -purN a/drivers/scsi/ide-scsi.c b2/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-25 22:28:23.000000000 -0500
+++ b2/drivers/scsi/ide-scsi.c	2005-04-05 15:56:48.000000000 -0400
@@ -143,6 +143,9 @@ static void idescsi_input_buffers (ide_d
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -151,8 +154,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif
+		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);
+#ifdef CONFIG_HIGHMEM
+		local_irq_restore(flags);
+#endif
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -165,6 +175,9 @@ static void idescsi_output_buffers (ide_
 {
 	int count;
 	char *buf;
+#ifdef CONFIG_HIGHMEM
+	unsigned long flags;
+#endif
 
 	while (bcount) {
 		if (pc->sg - (struct scatterlist *) pc->scsi_cmd->request_buffer > pc->scsi_cmd->use_sg) {
@@ -173,8 +186,15 @@ 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;
+#ifdef CONFIG_HIGHMEM
+		local_irq_save(flags);
+#endif
+		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);
+#ifdef CONFIG_HIGHMEM
+		local_irq_restore(flags);
+#endif
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;

Re: [PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Bartlomiej Zolnierkiewicz]

On Apr 5, 2005 11:08 PM, Stuart_Hayes@dell.com <Stuart_Hayes@dell.com> wrote:

> How is this?  I already had the "flags" declarations at the beginning of
> the functions, but I wrapped them and the irq disable/enables in the

I mean at the very beginning but lets leave it as it is.

> HIGHMEM ifdef, like ide_pio_sector() in ide-taskfile.c.

thanks

Bartlomiej

[PATCH 2.6.11.6] ide-scsi: kmap scatter/gather before doing PIO

[Stuart_Hayes]

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


The system can panic with a null pointer dereference using ide-scsi if
PIO is being done on scatter gather pages that are in high memory,
because page_address() returns 0.  We are actually seeing this using a
tape drive.  This patch will kmap_atomic() the pages before performing
PIO.

I'm copying the text here, but I'm also attaching it as a file, because
the text will get garbled with extra line breaks.

Please let me know if there are any problems with the patch!

(I sent this patch to linux-scsi a few weeks ago, and I was finally told
that it is maintained by the IDE folks (Bartlomiej Zolnierkiewicz), not
the SCSI folks...)

Thanks!
Stuart


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

diff -purN a/drivers/scsi/ide-scsi.c b2/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-25 22:28:23.000000000 -0500
+++ b2/drivers/scsi/ide-scsi.c	2005-04-05 13:50:27.000000000 -0400
@@ -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: idescsi-kmap-2.6.11.6.patch --]
[-- Type: application/octet-stream, Size: 1692 bytes --]

diff -purN a/drivers/scsi/ide-scsi.c b2/drivers/scsi/ide-scsi.c
--- a/drivers/scsi/ide-scsi.c	2005-03-25 22:28:23.000000000 -0500
+++ b2/drivers/scsi/ide-scsi.c	2005-04-05 13:50:27.000000000 -0400
@@ -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.6] ide-scsi: kmap scatter/gather before doing PIO

[Bartlomiej Zolnierkiewicz]

Hi,

On Apr 5, 2005 9:12 PM, Stuart_Hayes@dell.com <Stuart_Hayes@dell.com> wrote:
> 
> The system can panic with a null pointer dereference using ide-scsi if
> PIO is being done on scatter gather pages that are in high memory,
> because page_address() returns 0.  We are actually seeing this using a
> tape drive.  This patch will kmap_atomic() the pages before performing
> PIO.
> 
> I'm copying the text here, but I'm also attaching it as a file, because
> the text will get garbled with extra line breaks.
> 
> Please let me know if there are any problems with the patch!

Looks fine, I have two small requests:
* please don't disable/enable local IRQs for !HIGHMEM case
  (see ide_pio_sector() in ide-taskfile.c)
* declare 'unsigned long flags' at the beginning of the function

> (I sent this patch to linux-scsi a few weeks ago, and I was finally told
> that it is maintained by the IDE folks (Bartlomiej Zolnierkiewicz), not
> the SCSI folks...)

I'm not on linux-scsi, *sigh*, somebody could have simply 
forwarded it to me...

Thanks!
Bartlomiej