Random libata notes

Random libata notes

[Jeff Garzik]

These are small TODO bits that I didn't want to forget...  Patches welcome.

1) ATAPI edge case.  Due to adding the padding s/g entry, we must adjust 
sg_tablesize to include ' - 1' for each driver

2) ata_scsi_slave_config() hardcodes a call to
blk_queue_max_phys_segments(, LIBATA_MAX_PRD), when the value passed
should not be so limited on nice hardware like AHCI.

3) sata_mv: add back SCR_ERR clear?
         mv_scr_write(ap, SCR_ERROR, mv_scr_read(ap, SCR_ERROR));

4) sata_mv: set DMA mask

Re: Random libata notes

[Tejun Heo]

Jeff Garzik wrote:
> These are small TODO bits that I didn't want to forget...  Patches welcome.
> 
> 1) ATAPI edge case.  Due to adding the padding s/g entry, we must adjust 
> sg_tablesize to include ' - 1' for each driver

Isn't this handled by ata_scsi_slave_config()? If the attached device is 
ATAPI, ata_scsi_slave_config() reduces max_hw_segments by 1. We'll need 
to increment it back when detaching the device though.

> 
> 2) ata_scsi_slave_config() hardcodes a call to
> blk_queue_max_phys_segments(, LIBATA_MAX_PRD), when the value passed
> should not be so limited on nice hardware like AHCI.
> 
> 3) sata_mv: add back SCR_ERR clear?
>         mv_scr_write(ap, SCR_ERROR, mv_scr_read(ap, SCR_ERROR));
> 
> 4) sata_mv: set DMA mask
> 


-- 
tejun

Re: Random libata notes

[Tejun Heo]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> These are small TODO bits that I didn't want to forget...  Patches 
>> welcome.
>>
>> 1) ATAPI edge case.  Due to adding the padding s/g entry, we must 
>> adjust sg_tablesize to include ' - 1' for each driver
> 
> 
> Isn't this handled by ata_scsi_slave_config()? If the attached device is 
> ATAPI, ata_scsi_slave_config() reduces max_hw_segments by 1. We'll need 
> to increment it back when detaching the device though.
> 
>>
>> 2) ata_scsi_slave_config() hardcodes a call to
>> blk_queue_max_phys_segments(, LIBATA_MAX_PRD), when the value passed
>> should not be so limited on nice hardware like AHCI.

And, AFAICS, we can increase max_phys_segments all we want. This limits 
the number of segments before IOMMU mapping. The only part that's 
affected is the driver (software) and I don't see any limitation in 
libata PIO implementation that puts limitation on the number of sg 
entries. Simply changing LIBATA_MAX_PRD to 65535 should do it.

>>
>> 3) sata_mv: add back SCR_ERR clear?
>>         mv_scr_write(ap, SCR_ERROR, mv_scr_read(ap, SCR_ERROR));
>>
>> 4) sata_mv: set DMA mask
>>

-- 
tejun

Re: Random libata notes

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> These are small TODO bits that I didn't want to forget...  Patches 
>> welcome.
>>
>> 1) ATAPI edge case.  Due to adding the padding s/g entry, we must 
>> adjust sg_tablesize to include ' - 1' for each driver
> 
> 
> Isn't this handled by ata_scsi_slave_config()? If the attached device is 
> ATAPI, ata_scsi_slave_config() reduces max_hw_segments by 1. We'll need 
> to increment it back when detaching the device though.

Yes, you're right.

	Jeff

Re: Random libata notes

[Jeff Garzik]

Tejun Heo wrote:
> Tejun Heo wrote:
> 
>> Jeff Garzik wrote:
>>
>>> These are small TODO bits that I didn't want to forget...  Patches 
>>> welcome.
>>>
>>> 1) ATAPI edge case.  Due to adding the padding s/g entry, we must 
>>> adjust sg_tablesize to include ' - 1' for each driver
>>
>>
>>
>> Isn't this handled by ata_scsi_slave_config()? If the attached device 
>> is ATAPI, ata_scsi_slave_config() reduces max_hw_segments by 1. We'll 
>> need to increment it back when detaching the device though.
>>
>>>
>>> 2) ata_scsi_slave_config() hardcodes a call to
>>> blk_queue_max_phys_segments(, LIBATA_MAX_PRD), when the value passed
>>> should not be so limited on nice hardware like AHCI.
> 
> 
> And, AFAICS, we can increase max_phys_segments all we want. This limits 
> the number of segments before IOMMU mapping. The only part that's 
> affected is the driver (software) and I don't see any limitation in 
> libata PIO implementation that puts limitation on the number of sg 
> entries. Simply changing LIBATA_MAX_PRD to 65535 should do it.

We really want to accurately export the correct value for each piece of 
hardware.  It IMO isn't wise to tempt fate with a wide disparity between 
max_phys_segments, max_hw_segments, and real life...  :)

	Jeff

Re: Random libata notes

[Tejun Heo]

On Sat, Mar 11, 2006 at 08:37:45PM -0500, Jeff Garzik wrote:
> >>>
> >>>2) ata_scsi_slave_config() hardcodes a call to
> >>>blk_queue_max_phys_segments(, LIBATA_MAX_PRD), when the value passed
> >>>should not be so limited on nice hardware like AHCI.
> >
> >
> >And, AFAICS, we can increase max_phys_segments all we want. This limits 
> >the number of segments before IOMMU mapping. The only part that's 
> >affected is the driver (software) and I don't see any limitation in 
> >libata PIO implementation that puts limitation on the number of sg 
> >entries. Simply changing LIBATA_MAX_PRD to 65535 should do it.
> 
> We really want to accurately export the correct value for each piece of 
> hardware.  It IMO isn't wise to tempt fate with a wide disparity between 
> max_phys_segments, max_hw_segments, and real life...  :)
> 

Okay, I may have misunderstood, but max_phys_segments has *nothing* to
do with hardware limits.  max_phys_segments tells the block layer how
many segments the device driver (software) can handle while
max_hw_segments tells the block layer how many segments the hardware
can handle.

So, on machines without an IOMMU, unless max_phys_segments is smaller
than max_hw_segments, it is ignored.  And on machine with an IOMMU,
the number of sg entries may exceed max_hw_segments iff it's still
under max_phys_segments and the number of sg entries hardware sees is
under than max_hw_segments after IOMMU merging.

AFAICS, there is no limit in the current libata code which limits the
number of sg entries libata software can handle.  The only affected
codes would be sg mapping and PIO codes, both of which don't put any
limits on the number of segments they can handle.  So, there is no
reason to limit max_phys_segments to any number lower than its
maximum.  Limiting max_hw_segments is enough.

Hmmm.. Or is it that you are worried about block layer and/or IOMMU
acting weirdly?

Thanks.

-- 
tejun