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

[Jens Axboe <axboe@suse.de>]

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