Bartlomiej Zolnierkiewicz wrote:

Hi,

On 5/2/05, Stuart_Hayes@dell.com [off-list ref] wrote:

quoted

Jens Axboe wrote:

quoted

On Fri, Apr 29 2005, Stuart_Hayes@Dell.com wrote:

quoted

quoted

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. 

quoted

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?

quoted

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++;