Re: [PATCH v2] add bidi support for block pc requests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 2007-07-07 at 11:27 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > On Sun, 2007-06-03 at 10:45 +0300, Boaz Harrosh wrote:
> >> Jeff Garzik wrote:
> >>> Boaz Harrosh wrote:
> >>>> FUJITA Tomonori wrote:
> >>>>> From: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
> >>>>> Subject: Re: [PATCH v2] add bidi support for block pc requests
> >>>>> Date: Thu, 17 May 2007 17:00:21 +0300
> >>>>>
> >>>>>> Yes Tomo found it at ata_scsi_slave_config(). Attached below the way I
> >>>>>> fixed it. Now it works with 127.
> >>>>> I think that we can just remove blk_queue_max_phys_segments since the
> >>>>> ata drivers seem to set sg_tablesize to LIBATA_MAX_PRD.
> >>>>>
> >>>> Who should send a patch upstream? (I cc'ed Jeff Garzik)
> >>>>
> >>>> Boaz
> >>>>
> >>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> >>>> index dd81fa7..3660f3e 100644
> >>>> --- a/drivers/ata/libata-scsi.c
> >>>> +++ b/drivers/ata/libata-scsi.c
> >>>> @@ -800,8 +800,6 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
> >>>>
> >>>>  	ata_scsi_sdev_config(sdev);
> >>>>
> >>>> -	blk_queue_max_phys_segments(sdev->request_queue, LIBATA_MAX_PRD);
> >>>> -
> >>>>  	sdev->manage_start_stop = 1;
> >>> I don't mind the patch, but could someone refresh me as to the context?
> >>>
> >>> Is there something wrong with the above code, or is it simply redundant 
> >>> to the scsi_host_template settings in each LLDD?
> >>>
> >>> 	Jeff
> >>>
> >>>
> >>>
> >> Hi Jeff
> >> What happens is that if SCSI-ml sets an higher value than LIBATA_MAX_PRD (=128)
> >> than every thing is OK and libata-scsi will only see its LIBATA_MAX_PRD. But what
> >> happens if SCSI-ml sets a lower value? It will than crash on unexpected high sg
> >> count. My first Patch was an "if >" but Tomo said that it is redundant since
> >> drivers do that already. So I guess it is your call. Can it be removed or we need
> >> a: if ( sdev->request_queue->max_phys_segments > LIBATA_MAX_PRD)
> > 
> > Ordinarily, in SCSI, phys_segments is the wrong thing for a driver to
> > set because what a driver wants to see is what comes out the other side
> > of dma_map_sg() (which is controlled by hw_segments) not what goes in.
> > However, I could see libata having an interest in the phys_segments if
> > the physical region descriptors are going to the device via PIO ... is
> > that what this is for?
> 
> LIBATA_MAX_PRD is the maximum number of DMA scatter/gather elements 
> permitted by the HBA's DMA engine, for a single ATA command.

Then it's the wrong parameter you're setting:  phys_segments is what you
have going into a dma_map_sg() but hw_segments is what you end up after
it (mathemtaically, the mapping never splits segments, so hw_segments <=
phys_segments always, but it's still the wrong way to bound this); so if
you want to limit the SG elements in the HBA, you should set hw_segments
not phys_segments (which is what the sg_tablesize parameter of the host
structure corresponds to).

However, I suspect this has to do with our iommu problem, doesn't it?
The IOMMU may merge the segments beyond your 64k DMA boundary, so you
need to split them again ... in that case you need phys_segments
bounded, not hw_segments?  I really wish we could get this iommu
parameter problem fixed so we didn't have to have all of this confusing
code.

> The PIO code doesn't really care about segments.

OK ... I just thought it was PIO because the only time you should worry
about phys_segments is if you're not going to do a dma_map_sg() on the
scatterlist.

James


-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux