Re: Organized summary of the pacthes

Re: Organized summary of the pacthes

[Jeff Garzik]

Albert Lee wrote:
> Summary of the pacthes for your review (as of 6/28/2005):
> 1. atapi_pio_bytes() fixes
>   => Three patches pending:
>      #1-2. __atapi_pio_bytes(): if condition fix
>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838913812842&w=2

I'll probably apply this


>      #1-3. __atapi_pio_bytes(): trailing data handling fix
>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838955410270&w=2
> 
>      #1-4. __atapi_pio_bytes(): odd-length data handling fix
>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838987630460&w=2
>      Patch #1-4 will overrun the odd-length buffer by one byte. :(
>      Maybe we can just ignore the last byte of odd-length buffer?

These two patches make me REALLY nervous.  This overrun business needs 
to be handled in a different way.

For DMA, we will want to copy 0-3 odd bytes into a 4-byte buffer, and 
then make that 4-byte buffer than final DMA segment.

For PIO, we might as well use the same buffer, and copy the last 0-3 (or 
0-1?) odd bytes.

Never overrun.


> 2. atapi_packet_task() assertion failed fix
>   => Revised patch submitted:
>      http://marc.theaimsgroup.com/?l=linux-ide&m=111830130223323&w=2
>   => Need more revision to remove the 'case ATA_PROT_ATAPI' handling in it.
>     (It conflicts with patch #3-2 below:
>      http://marc.theaimsgroup.com/?l=linux-ide&m=111951880730921&w=2)
> 
> 
> 3. ata_pio_task() race condition fixes.
>   => Two patches submitted. Waiting for review:
>    #3-1  http://marc.theaimsgroup.com/?l=linux-ide&m=111958527005103&w=2
>    #3-2  http://marc.theaimsgroup.com/?l=linux-ide&m=111951880730921&w=2

These are probably OK.

However, I am pondering scrapping all the polling code, since on SATA, 
interrupt-driven mode is much more desirable.

If I continue to use polling, I will (after further review) apply your 
patches.

Luckily PIO data paths are not used by users yet (disabled by default), 
so these problems are not urgent.


> =======
> 4. ata_qc_complete() race condition fix.
>   => In ata_qc_complete(),
>      "qc->flags &= ~ATA_QCFLAG_ACTIVE;" should be _before_
>      "rc = qc->complete_fn(qc, drv_stat);".

I agree.


>   => Otherwise ata_qc_complete() might race with atapi_request_sense().

If ata_qc_complete() is racing with atapi_request_sense(), more 
synchronization is needed.  ATAPI is violating the host state machine, 
that wants fixing.


> 5. pata_pdc2027x 0.62 update
>   => http://marc.theaimsgroup.com/?l=linux-ide&m=111474486330692&w=2
>   => Need more revision according to libata-dev tree in git.

I think this patch is not needed, once ATAPI DMA/PIO is fixed properly 
as described above.


> 6. IDENTIFY DEVICE detects phantom drive problem
>    http://marc.theaimsgroup.com/?l=linux-ide&m=111622547309019&w=2

To answer the question in your email here, we should separate hardware 
Status register value from a new 'force_error' parameter.  This would 
require a new test in the code

	int have_error = forced_error || (tf->status & ATA_ERR);

Eventually we need to separate out errors into separate classes (hi 
Alan, hi Tejun), such as dma-error, bus-error, device-error, etc.  When 
that is done, the code paths described in your email must change anyway.

	Jeff

Re: Organized summary of the pacthes

[Alan Cox]

On Maw, 2005-06-28 at 16:21, Jeff Garzik wrote:
> These two patches make me REALLY nervous.  This overrun business needs 
> to be handled in a different way.
> 
> For DMA, we will want to copy 0-3 odd bytes into a 4-byte buffer, and 
> then make that 4-byte buffer than final DMA segment.

You may need 16 or 32 byte alignment for some hardware on the host side.

> However, I am pondering scrapping all the polling code, since on SATA, 
> interrupt-driven mode is much more desirable.

You need both interrupt and polled command issue. ACPI needs to be able
to issue taskfiles to the IDE drive fairly early during resume. Right
now this isn't done at all. If polled is there it might also prove a
useful way to reissue a command when you think the IRQ routing is hosed
8)

There is a really nasty case here that the old IDE layer totally screwed
up: You may need to sequence speed changes into error recovery (BAD CRC)
which is coming from a timeout and irq context. In the sata layer at
least its from an eh thread.

Alan

Re: Organized summary of the pacthes

[Albert Lee]

Jeff,

> Albert Lee wrote:
> 
>> Summary of the pacthes for your review (as of 6/28/2005):
>> 1. atapi_pio_bytes() fixes
>>   => Three patches pending:
>>      #1-2. __atapi_pio_bytes(): if condition fix
>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838913812842&w=2
> 
> 
> I'll probably apply this
> 
> 
>>      #1-3. __atapi_pio_bytes(): trailing data handling fix
>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838955410270&w=2
>>
>>      #1-4. __atapi_pio_bytes(): odd-length data handling fix
>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838987630460&w=2
>>      Patch #1-4 will overrun the odd-length buffer by one byte. :(
>>      Maybe we can just ignore the last byte of odd-length buffer?
> 
> 
> These two patches make me REALLY nervous.  This overrun business needs 
> to be handled in a different way.
> 
> For DMA, we will want to copy 0-3 odd bytes into a 4-byte buffer, and 
> then make that 4-byte buffer than final DMA segment.
> 
> For PIO, we might as well use the same buffer, and copy the last 0-3 (or 
> 0-1?) odd bytes.
> 
> Never overrun.
> 

Thanks for the review and tolerance for the newbie blindness.
I'll revise/resubmit patch 1-3 and 1-4 per your advice.
I will also study the DMA padding/alignment advised by you and Alan.

> 
>> 2. atapi_packet_task() assertion failed fix
>>   => Revised patch submitted:
>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111830130223323&w=2
>>   => Need more revision to remove the 'case ATA_PROT_ATAPI' handling 
>> in it.
>>     (It conflicts with patch #3-2 below:
>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111951880730921&w=2)
>>
>>
>> 3. ata_pio_task() race condition fixes.
>>   => Two patches submitted. Waiting for review:
>>    #3-1  http://marc.theaimsgroup.com/?l=linux-ide&m=111958527005103&w=2
>>    #3-2  http://marc.theaimsgroup.com/?l=linux-ide&m=111951880730921&w=2
> 
> 
> These are probably OK.
> 
> However, I am pondering scrapping all the polling code, since on SATA, 
> interrupt-driven mode is much more desirable.

Is there any draft plan or design for the interrupt driven PIO code
available? Also as Alan said, PIO polling would be useful for some
situation. Any plan for libata to support both PIO polling mode and the
interrupt driven mode?

If permitted, I would like to take this opportunity to participate in
the interrupt driven PIO task, under your design guideline.

> 
> If I continue to use polling, I will (after further review) apply your 
> patches.
> 
> Luckily PIO data paths are not used by users yet (disabled by default), 
> so these problems are not urgent.
> 

The rare race conditions only occur under hours of heavy stress test.

Based on the testing result, the libata PIO polling code is quite robust.
It has been tested with harddisks, cd-rom drives and even an old
ATAPI Zip 100 floppy drive, which supports PIO only; and it works
fine. (Except high CPU usage due to the nature of polling.)

> 
>> =======
>> 4. ata_qc_complete() race condition fix.
>>   => In ata_qc_complete(),
>>      "qc->flags &= ~ATA_QCFLAG_ACTIVE;" should be _before_
>>      "rc = qc->complete_fn(qc, drv_stat);".
> 
> 
> I agree.

Thanks, will submit the patch.

> 
> 
>>   => Otherwise ata_qc_complete() might race with atapi_request_sense().
> 
> 
> If ata_qc_complete() is racing with atapi_request_sense(), more 
> synchronization is needed.  ATAPI is violating the host state machine, 
> that wants fixing.

It's false alarm.
After moving "qc->flags &= ~ATA_QCFLAG_ACTIVE" described above,
no race between ata_qc_complete() and atapi_request_sense()
is ever seen. Please ignore my false alarm.

> 
> 
>> 5. pata_pdc2027x 0.62 update
>>   => http://marc.theaimsgroup.com/?l=linux-ide&m=111474486330692&w=2
>>   => Need more revision according to libata-dev tree in git.
> 
> 
> I think this patch is not needed, once ATAPI DMA/PIO is fixed properly 
> as described above.

Mmm, quite possibly. Will defer this patch and re-test after
ATAPI DMA/PIO fixes are ready.

> 
> 
>> 6. IDENTIFY DEVICE detects phantom drive problem
>>    http://marc.theaimsgroup.com/?l=linux-ide&m=111622547309019&w=2
> 
> 
> To answer the question in your email here, we should separate hardware 
> Status register value from a new 'force_error' parameter.  This would 
> require a new test in the code
> 
>     int have_error = forced_error || (tf->status & ATA_ERR);
> 
> Eventually we need to separate out errors into separate classes (hi 
> Alan, hi Tejun), such as dma-error, bus-error, device-error, etc.  When 
> that is done, the code paths described in your email must change anyway.

Understood. Please discard this patch.

Albert

RE: Organized summary of the pacthes

[Al Boldi]

By the way,
Why is the ide-driver in so slow?
Hdparm -tT gives 28mb/s in 2.6.12 
Cat /dev/hda > /dev/null gives 2% user 25% sys 0% idle 73% IOWAIT

Hdparm -tT gives 38mb/s in 2.4.31
Cat /dev/hda > /dev/null gives 2% user 33% sys 65% idle

It feels like DMA is not being applied properly in 2.6.12.

Your comments please.

Thanks!

Re: Organized summary of the pacthes

[Olaf Hering]

 On Tue, Jun 28, Jeff Garzik wrote:

> >=======
> >4. ata_qc_complete() race condition fix.
> >  => In ata_qc_complete(),
> >     "qc->flags &= ~ATA_QCFLAG_ACTIVE;" should be _before_
> >     "rc = qc->complete_fn(qc, drv_stat);".
> 
> I agree.

Does it need a membarrier as well?

Re: Organized summary of the pacthes

[Tejun Heo]

Albert Lee wrote:
> 
> Jeff,
> 
>> Albert Lee wrote:
>>
>>> Summary of the pacthes for your review (as of 6/28/2005):
>>> 1. atapi_pio_bytes() fixes
>>>   => Three patches pending:
>>>      #1-2. __atapi_pio_bytes(): if condition fix
>>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838913812842&w=2
>>
>>
>>
>> I'll probably apply this
>>
>>
>>>      #1-3. __atapi_pio_bytes(): trailing data handling fix
>>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838955410270&w=2
>>>
>>>      #1-4. __atapi_pio_bytes(): odd-length data handling fix
>>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111838987630460&w=2
>>>      Patch #1-4 will overrun the odd-length buffer by one byte. :(
>>>      Maybe we can just ignore the last byte of odd-length buffer?
>>
>>
>>
>> These two patches make me REALLY nervous.  This overrun business needs 
>> to be handled in a different way.
>>
>> For DMA, we will want to copy 0-3 odd bytes into a 4-byte buffer, and 
>> then make that 4-byte buffer than final DMA segment.
>>
>> For PIO, we might as well use the same buffer, and copy the last 0-3 
>> (or 0-1?) odd bytes.
>>
>> Never overrun.
>>
> 
> Thanks for the review and tolerance for the newbie blindness.
> I'll revise/resubmit patch 1-3 and 1-4 per your advice.
> I will also study the DMA padding/alignment advised by you and Alan.
> 
>>
>>> 2. atapi_packet_task() assertion failed fix
>>>   => Revised patch submitted:
>>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111830130223323&w=2
>>>   => Need more revision to remove the 'case ATA_PROT_ATAPI' handling 
>>> in it.
>>>     (It conflicts with patch #3-2 below:
>>>      http://marc.theaimsgroup.com/?l=linux-ide&m=111951880730921&w=2)
>>>
>>>
>>> 3. ata_pio_task() race condition fixes.
>>>   => Two patches submitted. Waiting for review:
>>>    #3-1  http://marc.theaimsgroup.com/?l=linux-ide&m=111958527005103&w=2
>>>    #3-2  http://marc.theaimsgroup.com/?l=linux-ide&m=111951880730921&w=2
>>
>>
>>
>> These are probably OK.
>>
>> However, I am pondering scrapping all the polling code, since on SATA, 
>> interrupt-driven mode is much more desirable.
> 
> 
> Is there any draft plan or design for the interrupt driven PIO code
> available? Also as Alan said, PIO polling would be useful for some
> situation. Any plan for libata to support both PIO polling mode and the
> interrupt driven mode?
> 
> If permitted, I would like to take this opportunity to participate in
> the interrupt driven PIO task, under your design guideline.
> 

  Hi, Albert.

  For interrupt-driven PIO, IDE driver should be an example albeit quite 
complex.  Once interrupt-driven PIO is implemented, IMHO, generic 
polling IO can easily be added by calling interrupt handler periodically 
while the driver is expecting notification (any command is in-flight). 
Most low-level SATA driver interrupt handlers are already ready to 
handle spurious interrupts, and maybe we can add a flag to disable 
polling for those unable to swallow spurious interrupts if any such 
device/driver exsits.

-- 
tejun

Re: Organized summary of the pacthes

[Doug Maxey]

On Tue, 28 Jun 2005 19:00:12 BST, Alan Cox wrote:
>On Maw, 2005-06-28 at 16:21, Jeff Garzik wrote:
>> These two patches make me REALLY nervous.  This overrun business needs
>> to be handled in a different way.
>>
>> For DMA, we will want to copy 0-3 odd bytes into a 4-byte buffer, and
>> then make that 4-byte buffer than final DMA segment.

Indeed, all chipsets I have seen require that the dma be aligned on 4
byte boundaries, and the transfer be a multiple of 4 bytes.  Maybe I
have not seen that many, but all I have seen do seem to conform to the
original Intel specs (or what was SFF-8038i).  Normally this is not an
issue all the buffers are powers of 2, 512 bytes or larger.

Also, we do need to honor the 64k boundary, as at least in this case,
the underlying device is indeed PATA.

>
>You may need 16 or 32 byte alignment for some hardware on the host side.

Where would one specify this, at the block layer?  Then bounce the
portions that extend outside this region as a separate transfer to
device (including the odd bytes above)?

Or are you referring to internal buffer creation only?

>
>> However, I am pondering scrapping all the polling code, since on SATA,
>> interrupt-driven mode is much more desirable.
>
>You need both interrupt and polled command issue. ACPI needs to be able
>to issue taskfiles to the IDE drive fairly early during resume. Right
>now this isn't done at all. If polled is there it might also prove a
>useful way to reissue a command when you think the IRQ routing is hosed
>8)

This would be an enhancement, rather than something required currently,
correct?

>
>There is a really nasty case here that the old IDE layer totally screwed
>up: You may need to sequence speed changes into error recovery (BAD CRC)

Likewise?  Dynamic fallback would be great, and should print a big fat
"your cables or drive are failing" message when the fallback was
initiated.

>which is coming from a timeout and irq context. In the sata layer at
>least its from an eh thread.
>

++doug

Re: Organized summary of the pacthes

[Jeff Garzik]

On Sat, Jul 09, 2005 at 07:54:06PM -0500, Doug Maxey wrote:
> >> However, I am pondering scrapping all the polling code, since on SATA,
> >> interrupt-driven mode is much more desirable.
> >
> >You need both interrupt and polled command issue. ACPI needs to be able
> >to issue taskfiles to the IDE drive fairly early during resume. Right
> >now this isn't done at all. If polled is there it might also prove a
> >useful way to reissue a command when you think the IRQ routing is hosed
> >8)
> 
> This would be an enhancement, rather than something required currently,
> correct?

Switching over to interrupt-driven for SATA is basically a requirement.

Since the polling code is what exists now, it MAY be easier to leave it
in, which puts "leave in polling code" in the enhancement category.

	Jeff