On Thu, Mar 17 2005, [EMAIL PROTECTED] wrote:
> Hi
>
> On Thu, 17 Mar 2005, Jens Axboe wrote:
>
> > On Wed, Mar 16 2005, Guennadi Liakhovetski wrote:
> > >
> > > On Wed, 16 Mar 2005, Jens Axboe wrote:
> > > > The kmap() isn't just inefficient, it's a problem to iterate over the sg
> > > > list and kmap all the pages. That is illegal.
> > >
> > > Hm, what do you mean "illegal"? Could you explain why?
> >
> > You risk deadlocking.
>
> Emn, I am curious, would be nice to know details:-)
See the kmap implementation, mm/highmem.c:map_new_virtual() to be
precise. If you run out of entries while processing your sglist, you
will end up waiting on a free kmap entry for an sg entry that will not
become available before your previous kmaps are released => deadlock.
> > > One more fragment in the driver I wasn't sure about is this:
> > >
> > > unsigned long mask =
> > > ~((unsigned long)sg->length - 1) & PAGE_MASK;
> > > if ((sg_dma_address(sg) & mask) == (psge->address & mask)) {
> > >
> > > Is sg->length guaranteed to be something like a power of 2 or smaller
> > > than page? I thought about replacing the above with
> >
> > No, it's not guaranteed to be a power-of-2.
> >
> > > + if (sg_dma_address(sg) <= psge->address && sg_dma_address(sg) +
> > > psge->length > psge->address) {
> >
> > What is it trying to accomplish?
>
> So, I was not sure if the old check was correct, was it? The test is
> supposed to find the current sg-element.
When this happens, do you know how many bytes of io already completed?
Most sane drives do, so I would use that to find the current sg entry.
> >
> > > @@ -1020,11 +1022,11 @@
> > > reqlen, cmd->request_buffer, cmd->use_sg,
> > > srb->sg_count);
> > >
> > > - srb->virt_addr = page_address(sl->page);
> > > + srb->page = sl->page;
> > > + srb->offset = sl->offset;
> > > for (i = 0; i < srb->sg_count; i++) {
> > > - u32 busaddr = (u32)sg_dma_address(&sl[i]);
> > > - u32 seglen = (u32)sl[i].length;
> > > - sgp[i].address = busaddr;
> > > + u32 seglen = (u32)sg_dma_len(sl + i);
> > > + sgp[i].address = cpu_to_le32(0xffffffff &
> > > sg_dma_address(sl +i));
> > > sgp[i].length = seglen;
> > > srb->total_xfer_length += seglen;
> > > }
> >
> > I don't understand this change, why the cpu_to_le32?
>
> Well, I copied it from tmscsim:
>
> pSRB->SGBusAddr = cpu_to_le32(pci_dma_lo32(sg_dma_address(psgl)));
>
> I am somewhat confused. In both cases these are bus addresses, right? and
> sg_dma_address() gives already the bus address, so, both are wrong?
The only reason that would make sense is if ->SGBusAddr is passed to the
hardware and it requires LE encoding. If it is just driver internal use,
it doesn't make any sense.
> BTW, looking at tmscsim again, isn't this
>
> if( residual )
> {
> bval = DC390_read8 (ScsiFifo); /* get one residual byte */
> ptr = (u8 *) bus_to_virt( pSRB->SGBusAddr );
> *ptr = bval;
> pSRB->SGBusAddr++; xferCnt++;
> pSRB->TotalXferredLen++;
> pSRB->SGToBeXferLen--;
> }
>
> also a case for dma_map_atomic?
It breaks for iommu, for sure.
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html