Re: possible esata regression in 2.6.35

Re: possible esata regression in 2.6.35

[Mark Lord]

On 10-08-21 02:52 PM, Nicolas Jungers wrote:
> My arm box doesn't succeed to use my esata port multiplier (addonics sil3726 based).  It was working well with 2.6.34.1 and 2.6.34.4 but not with both 2.6.35.2 and 2.6.35.3.  I haven't test other kernels.
>
> The kernels are from http://sheeva.with-linux.com/sheeva/ with for example the following config http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>
> The symptoms are in the console a loop on the esata links. Here is the start of it:
>
> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
> ata2: SError: { PHYRdyChg DevExch }
> ata2: hard resetting link
> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
> ata2.00: hard resetting link
> ata2.01: hard resetting link
> ata2.02: hard resetting link
> ata2.03: hard resetting link
> ata2.04: hard resetting link
> ata2.05: hard resetting link
> ata2.00: qc timeout (cmd 0xec)
> ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
> ata2.15: hard resetting link
> ata2.15: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
> ata2.00: hard resetting link
> ata2.01: hard resetting link
> ata2.02: hard resetting link
> ata2.04: hard resetting link
> ata2.05: hard resetting link
> ata2.00: qc timeout (cmd 0xec)
> ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
> ata2.15: hard resetting link
> ata2.15: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
> ata2.00: hard resetting link
> ata2.01: hard resetting link
> ata2.02: hard resetting link
> ata2.03: hard resetting link
> ata2.04: hard resetting link
> ata2.05: hard resetting link
> ata2.00: qc timeout (cmd 0xec)
> ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
> ata2.00: failed to recover link after 3 tries, disabling
> ata2.15: hard resetting link
> ata2.15: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
...

 From the "edma_err" keyword above, we can deduce that you've got this PM
plugged into a Marvell SATA controller of some kind, managed by sata_mv.
Care to tell us more?

There are various changes in libata in 2.6.34/35 that break sata_mv,
particularly for ATAPI drives and any SSDs that support the DSM/TRIM commands.
Do either of those two things apply in your case?

If so, there are two pending patches which fix them.
They were posted to linux-ide this past Thursday,
and can also be found as attachments to this bug report:

   https://bugzilla.kernel.org/show_bug.cgi?id=16434

Cheers

Re: possible esata regression in 2.6.35

[Nicolas Jungers]

On 08/21/2010 09:13 PM, Mark Lord wrote:
> On 10-08-21 02:52 PM, Nicolas Jungers wrote:
>> My arm box doesn't succeed to use my esata port multiplier (addonics
>> sil3726 based). It was working well with 2.6.34.1 and 2.6.34.4 but not
>> with both 2.6.35.2 and 2.6.35.3. I haven't test other kernels.
>>
>> The kernels are from http://sheeva.with-linux.com/sheeva/ with for
>> example the following config
>> http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>>
>> The symptoms are in the console a loop on the esata links. Here is the
>> start of it:
>>
>> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
>> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
>> ata2: SError: { PHYRdyChg DevExch }
>> ata2: hard resetting link
>> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
>> ata2.00: hard resetting link
>> ata2.01: hard resetting link
>> ata2.02: hard resetting link
>> ata2.03: hard resetting link
>> ata2.04: hard resetting link
>> ata2.05: hard resetting link
>> ata2.00: qc timeout (cmd 0xec)
>> ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>> ata2.15: hard resetting link
>> ata2.15: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>> ata2.00: hard resetting link
>> ata2.01: hard resetting link
>> ata2.02: hard resetting link
>> ata2.04: hard resetting link
>> ata2.05: hard resetting link
>> ata2.00: qc timeout (cmd 0xec)
>> ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>> ata2.15: hard resetting link
>> ata2.15: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>> ata2.00: hard resetting link
>> ata2.01: hard resetting link
>> ata2.02: hard resetting link
>> ata2.03: hard resetting link
>> ata2.04: hard resetting link
>> ata2.05: hard resetting link
>> ata2.00: qc timeout (cmd 0xec)
>> ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>> ata2.00: failed to recover link after 3 tries, disabling
>> ata2.15: hard resetting link
>> ata2.15: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
> ...
>
>  From the "edma_err" keyword above, we can deduce that you've got this PM
> plugged into a Marvell SATA controller of some kind, managed by sata_mv.
> Care to tell us more?

sorry, I'm deep into it, so it's too obvious for me.  Yes, it's he 
marvell plug computer (esata sheevaplug), kirkwood platform with indeed 
the sata_mv module taking care of the sata port.

Start of the boot:

Linux version 2.6.35.3 (kelly@speedy) (gcc version 4.4.3 (Sourcery G++ 
Lite er) ) #1 PREEMPT Fri Aug 20 18:22:29 MDT 2010
CPU: Feroceon 88FR131 [56251311] revision 1 (ARMv5TE), cr=00053177
CPU: VIVT data cache, VIVT instruction cache
Machine: Marvell eSATA SheevaPlug Reference Board

and later:

Waiting for /dev to be fully populated...sata_mv sata_mv.0: version 1.28
sata_mv sata_mv.0: slots 32 ports 2
scsi0 : sata_mv
scsi1 : sata_mv
ata1: SATA max UDMA/133 irq 21
ata2: SATA max UDMA/133 irq 21
ata1: SATA link down (SStatus 0 SControl F300)
ata2: SATA link down (SStatus 0 SControl F300)
done.

>
> There are various changes in libata in 2.6.34/35 that break sata_mv,
> particularly for ATAPI drives and any SSDs that support the DSM/TRIM
> commands.
> Do either of those two things apply in your case?

I couldn't tell, but the problem is with the port multiplier.  When I 
directly attach a sata drive, it works.

>
> If so, there are two pending patches which fix them.
> They were posted to linux-ide this past Thursday,
> and can also be found as attachments to this bug report:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=16434

I haven't compiled a kernel in ages, but I'll try that Monday and report.

>
> Cheers

thanks,
N.

Re: possible esata regression in 2.6.35

[Jeff Garzik]

On 08/21/2010 02:52 PM, Nicolas Jungers wrote:
> My arm box doesn't succeed to use my esata port multiplier (addonics
> sil3726 based). It was working well with 2.6.34.1 and 2.6.34.4 but not
> with both 2.6.35.2 and 2.6.35.3. I haven't test other kernels.
>
> The kernels are from http://sheeva.with-linux.com/sheeva/ with for
> example the following config
> http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>
> The symptoms are in the console a loop on the esata links. Here is the
> start of it:
>
> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
> ata2: SError: { PHYRdyChg DevExch }
> ata2: hard resetting link
> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9

Can you post or link to the entire dmesg?

Notably, we need to see the probe messages to determine what SATA chip 
you are using...  From the edma_err_cause config I'd guess sata_mv, but 
more info would be useful.

	Jeff

Re: possible esata regression in 2.6.35

[Jeff Garzik]

On 08/22/2010 03:54 PM, Jeff Garzik wrote:
> On 08/21/2010 02:52 PM, Nicolas Jungers wrote:
>> My arm box doesn't succeed to use my esata port multiplier (addonics
>> sil3726 based). It was working well with 2.6.34.1 and 2.6.34.4 but not
>> with both 2.6.35.2 and 2.6.35.3. I haven't test other kernels.
>>
>> The kernels are from http://sheeva.with-linux.com/sheeva/ with for
>> example the following config
>> http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>>
>> The symptoms are in the console a loop on the esata links. Here is the
>> start of it:
>>
>> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
>> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
>> ata2: SError: { PHYRdyChg DevExch }
>> ata2: hard resetting link
>> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
>
> Can you post or link to the entire dmesg?
>
> Notably, we need to see the probe messages to determine what SATA chip
> you are using... From the edma_err_cause config I'd guess sata_mv, but
> more info would be useful.

Nevermind, I see the reply (it got auto-sorted into the wrong folder 
locally).

Re: possible esata regression in 2.6.35

[Gwendal Grignou]

I can reproduce the problem on uptsream-linux using a PC with Marvell
7042 controller and Sil3726 PMP. Without the SIl3726, it works fine.

What I can see on the SATA analyzer [I will send clean trace tomorrow]
is the disk send the DATA FIS back to the PMP, but the PMP does not
manage to have the data accepted by the host.

Non data commands work fine.

In dmesg:
[10058.404047] ata29: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[10058.404742] ata29.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6
ports, feat 0x1/0x9
[10058.405250] ata29.00: hard resetting link
[10058.809151] ata29.00: link resume succeeded after 1 retries
[10058.911613] ata29.01: hard resetting link
[10059.217572] ata29.02: hard resetting link
[10059.523572] ata29.03: hard resetting link
[10059.829505] ata29.04: hard resetting link
[10060.134572] ata29.05: hard resetting link
[10065.440079] ata29.03: qc timeout (cmd 0xec)
[10065.440085] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
[10065.440092] ata29.15: hard resetting link
[10065.947049] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[10065.947737] ata29.00: hard resetting link
[10066.352106] ata29.00: link resume succeeded after 1 retries
[10066.454616] ata29.01: hard resetting link
[10066.760591] ata29.02: hard resetting link
[10067.066570] ata29.03: hard resetting link
[10067.372505] ata29.05: hard resetting link
[10077.677071] ata29.03: qc timeout (cmd 0xec)
[10077.677076] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
[10077.677081] ata29.15: hard resetting link
[10078.184073] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[10078.184783] ata29.00: hard resetting link
[10078.589185] ata29.00: link resume succeeded after 1 retries
[10078.691593] ata29.01: hard resetting link
[10078.997592] ata29.02: hard resetting link
[10079.303571] ata29.03: hard resetting link
[10079.609505] ata29.04: hard resetting link
[10079.914572] ata29.05: hard resetting link
[10110.220078] ata29.03: qc timeout (cmd 0xec)
[10110.220083] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
[10110.220087] ata29.03: failed to recover link after 3 tries, disabling
[10110.220094] ata29.15: hard resetting link
[10110.727044] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[10111.033375] ata29.00: hard resetting link
[10111.338571] ata29.01: hard resetting link
[10111.644591] ata29.02: hard resetting link
[10111.950594] ata29.05: hard resetting link
[10112.256643] ata29: EH complete


Gwendal.




On Sun, Aug 22, 2010 at 12:57 PM, Jeff Garzik <jeff@garzik.org> wrote:
> On 08/22/2010 03:54 PM, Jeff Garzik wrote:
>>
>> On 08/21/2010 02:52 PM, Nicolas Jungers wrote:
>>>
>>> My arm box doesn't succeed to use my esata port multiplier (addonics
>>> sil3726 based). It was working well with 2.6.34.1 and 2.6.34.4 but not
>>> with both 2.6.35.2 and 2.6.35.3. I haven't test other kernels.
>>>
>>> The kernels are from http://sheeva.with-linux.com/sheeva/ with for
>>> example the following config
>>> http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>>>
>>> The symptoms are in the console a loop on the esata links. Here is the
>>> start of it:
>>>
>>> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
>>> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
>>> ata2: SError: { PHYRdyChg DevExch }
>>> ata2: hard resetting link
>>> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>>> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
>>
>> Can you post or link to the entire dmesg?
>>
>> Notably, we need to see the probe messages to determine what SATA chip
>> you are using... From the edma_err_cause config I'd guess sata_mv, but
>> more info would be useful.
>
> Nevermind, I see the reply (it got auto-sorted into the wrong folder
> locally).
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: possible esata regression in 2.6.35

[Gwendal Grignou]

I found the problem: in ata_sff_pio_stack:

struct ata_queued_cmd *qc = ap->port_task_data;

has been replaced by:

/* qc can be NULL if timeout occurred */
qc = ata_qc_from_tag(ap, ap->link.active_tag);
if (!qc)
	return;

That does not work in case of port multipler, because the link to look
at is not ap->link. ap->link.active_tag is ATA_POISON.

I will submit a patch where I re-introduce port_task_data, this time
containing the link to to look at.

Gwendal.



On Fri, Aug 27, 2010 at 12:29 PM, Gwendal Grignou <gwendal@google.com> wrote:
> I include the trace. You need windows and a lecroy satasuite to look
> at the trace.
>
> Looking at extended traces, the identify never move in HSM:
>
> Aug 27 12:07:53 halab-59 kernel: [  548.613529] ata_sff_flush_pio_task: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613561] ata_sff_exec_command:
> ata13: cmd 0xE4
> Aug 27 12:07:53 halab-59 kernel: [  548.613577] ata_sff_hsm_move:
> ata13: protocol 1 task_state 3 (dev_stat 0x50)
> Aug 27 12:07:53 halab-59 kernel: [  548.613580] ata_sff_hsm_move:
> ata13: dev 0 command complete, drv_stat 0x50
> Aug 27 12:07:53 halab-59 kernel: [  548.613605] ata_sff_flush_pio_task: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613637] ata_sff_exec_command:
> ata13: cmd 0xE4
> Aug 27 12:07:53 halab-59 kernel: [  548.613654] ata_sff_hsm_move:
> ata13: protocol 1 task_state 3 (dev_stat 0x50)
> Aug 27 12:07:53 halab-59 kernel: [  548.613656] ata_sff_hsm_move:
> ata13: dev 0 command complete, drv_stat 0x50
> Aug 27 12:07:53 halab-59 kernel: [  548.613681] ata_sff_flush_pio_task: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613688]
> ata_eh_revalidate_and_attach: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613691]
> ata_eh_revalidate_and_attach: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613693]
> ata_eh_revalidate_and_attach: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613694]
> ata_eh_revalidate_and_attach: ENTER
> Aug 27 12:07:53 halab-59 kernel: [  548.613725] ata_sff_exec_command:
> ata13: cmd 0xEC
> ...
> Aug 27 12:08:23 halab-59 kernel: [  578.613048] __ata_port_freeze:
> ata13 port frozen
> Aug 27 12:08:23 halab-59 kernel: [  578.613058] ata13.03: qc timeout (cmd 0xec)
> Aug 27 12:08:23 halab-59 kernel: [  578.613062] ata13.03: failed to
> IDENTIFY (I/O error, err_mask=0x4)
>
>
> On Fri, Aug 27, 2010 at 1:19 AM, Gwendal Grignou <gwendal@google.com> wrote:
>> I can reproduce the problem on uptsream-linux using a PC with Marvell
>> 7042 controller and Sil3726 PMP. Without the SIl3726, it works fine.
>>
>> What I can see on the SATA analyzer [I will send clean trace tomorrow]
>> is the disk send the DATA FIS back to the PMP, but the PMP does not
>> manage to have the data accepted by the host.
>>
>> Non data commands work fine.
>>
>> In dmesg:
>> [10058.404047] ata29: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>> [10058.404742] ata29.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6
>> ports, feat 0x1/0x9
>> [10058.405250] ata29.00: hard resetting link
>> [10058.809151] ata29.00: link resume succeeded after 1 retries
>> [10058.911613] ata29.01: hard resetting link
>> [10059.217572] ata29.02: hard resetting link
>> [10059.523572] ata29.03: hard resetting link
>> [10059.829505] ata29.04: hard resetting link
>> [10060.134572] ata29.05: hard resetting link
>> [10065.440079] ata29.03: qc timeout (cmd 0xec)
>> [10065.440085] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
>> [10065.440092] ata29.15: hard resetting link
>> [10065.947049] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>> [10065.947737] ata29.00: hard resetting link
>> [10066.352106] ata29.00: link resume succeeded after 1 retries
>> [10066.454616] ata29.01: hard resetting link
>> [10066.760591] ata29.02: hard resetting link
>> [10067.066570] ata29.03: hard resetting link
>> [10067.372505] ata29.05: hard resetting link
>> [10077.677071] ata29.03: qc timeout (cmd 0xec)
>> [10077.677076] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
>> [10077.677081] ata29.15: hard resetting link
>> [10078.184073] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>> [10078.184783] ata29.00: hard resetting link
>> [10078.589185] ata29.00: link resume succeeded after 1 retries
>> [10078.691593] ata29.01: hard resetting link
>> [10078.997592] ata29.02: hard resetting link
>> [10079.303571] ata29.03: hard resetting link
>> [10079.609505] ata29.04: hard resetting link
>> [10079.914572] ata29.05: hard resetting link
>> [10110.220078] ata29.03: qc timeout (cmd 0xec)
>> [10110.220083] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
>> [10110.220087] ata29.03: failed to recover link after 3 tries, disabling
>> [10110.220094] ata29.15: hard resetting link
>> [10110.727044] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>> [10111.033375] ata29.00: hard resetting link
>> [10111.338571] ata29.01: hard resetting link
>> [10111.644591] ata29.02: hard resetting link
>> [10111.950594] ata29.05: hard resetting link
>> [10112.256643] ata29: EH complete
>>
>>
>> Gwendal.
>>
>>
>>
>>
>> On Sun, Aug 22, 2010 at 12:57 PM, Jeff Garzik <jeff@garzik.org> wrote:
>>> On 08/22/2010 03:54 PM, Jeff Garzik wrote:
>>>>
>>>> On 08/21/2010 02:52 PM, Nicolas Jungers wrote:
>>>>>
>>>>> My arm box doesn't succeed to use my esata port multiplier (addonics
>>>>> sil3726 based). It was working well with 2.6.34.1 and 2.6.34.4 but not
>>>>> with both 2.6.35.2 and 2.6.35.3. I haven't test other kernels.
>>>>>
>>>>> The kernels are from http://sheeva.with-linux.com/sheeva/ with for
>>>>> example the following config
>>>>> http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>>>>>
>>>>> The symptoms are in the console a loop on the esata links. Here is the
>>>>> start of it:
>>>>>
>>>>> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
>>>>> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
>>>>> ata2: SError: { PHYRdyChg DevExch }
>>>>> ata2: hard resetting link
>>>>> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>>>>> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
>>>>
>>>> Can you post or link to the entire dmesg?
>>>>
>>>> Notably, we need to see the probe messages to determine what SATA chip
>>>> you are using... From the edma_err_cause config I'd guess sata_mv, but
>>>> more info would be useful.
>>>
>>> Nevermind, I see the reply (it got auto-sorted into the wrong folder
>>> locally).
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>

[PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Gwendal Grignou]

Re-introduce port_task_data to hold the link on the which the current
request is in progress. It allows support of links behind port multiplier.

Not all libata-sff is PMP compliant. Code for native BMDMA controller
does not take in accound PMP.

Tested on Marvell 7042 and Sil7526.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-sff.c |   25 ++++++++++++++++---------
 drivers/ata/sata_mv.c    |    2 +-
 include/linux/libata.h   |    4 +++-
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3b82d8e..70c0e16 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1042,7 +1042,8 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 		     u8 status, int in_wq)
 {
-	struct ata_eh_info *ehi = &ap->link.eh_info;
+	struct ata_link *link = qc->dev->link;
+	struct ata_eh_info *ehi = &link->eh_info;
 	unsigned long flags = 0;
 	int poll_next;
 
@@ -1298,8 +1299,11 @@ fsm_start:
 }
 EXPORT_SYMBOL_GPL(ata_sff_hsm_move);
 
-void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay)
+void ata_sff_queue_pio_task(struct ata_port *ap, void *data,
+			    unsigned long delay)
 {
+	ap->port_task_data = data;
+
 	/* may fail if ata_sff_flush_pio_task() in progress */
 	queue_delayed_work(ata_sff_wq, &ap->sff_pio_task,
 			   msecs_to_jiffies(delay));
@@ -1321,12 +1325,13 @@ static void ata_sff_pio_task(struct work_struct *work)
 {
 	struct ata_port *ap =
 		container_of(work, struct ata_port, sff_pio_task.work);
+	struct ata_link *link = ap->port_task_data;
 	struct ata_queued_cmd *qc;
 	u8 status;
 	int poll_next;
 
 	/* qc can be NULL if timeout occurred */
-	qc = ata_qc_from_tag(ap, ap->link.active_tag);
+	qc = ata_qc_from_tag(ap, link->active_tag);
 	if (!qc)
 		return;
 
@@ -1345,7 +1350,7 @@ fsm_start:
 		msleep(2);
 		status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
 		if (status & ATA_BUSY) {
-			ata_sff_queue_pio_task(ap, ATA_SHORT_PAUSE);
+			ata_sff_queue_pio_task(ap, link, ATA_SHORT_PAUSE);
 			return;
 		}
 	}
@@ -1376,6 +1381,7 @@ fsm_start:
 unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_link *link = qc->dev->link;
 
 	/* Use polling pio if the LLD doesn't handle
 	 * interrupt driven pio and atapi CDB interrupt.
@@ -1396,7 +1402,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		ap->hsm_task_state = HSM_ST_LAST;
 
 		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(ap, link, 0);
 
 		break;
 
@@ -1409,7 +1415,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		if (qc->tf.flags & ATA_TFLAG_WRITE) {
 			/* PIO data out protocol */
 			ap->hsm_task_state = HSM_ST_FIRST;
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(ap, link, 0);
 
 			/* always send first data block using the
 			 * ata_sff_pio_task() codepath.
@@ -1419,7 +1425,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 			ap->hsm_task_state = HSM_ST;
 
 			if (qc->tf.flags & ATA_TFLAG_POLLING)
-				ata_sff_queue_pio_task(ap, 0);
+				ata_sff_queue_pio_task(ap, link, 0);
 
 			/* if polling, ata_sff_pio_task() handles the
 			 * rest.  otherwise, interrupt handler takes
@@ -1441,7 +1447,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		/* send cdb by polling if no cdb interrupt */
 		if ((!(qc->dev->flags & ATA_DFLAG_CDB_INTR)) ||
 		    (qc->tf.flags & ATA_TFLAG_POLLING))
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(ap, link, 0);
 		break;
 
 	default:
@@ -2734,6 +2740,7 @@ EXPORT_SYMBOL_GPL(ata_bmdma_dumb_qc_prep);
 unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_link *link = qc->dev->link;
 
 	/* defer PIO handling to sff_qc_issue */
 	if (!ata_is_dma(qc->tf.protocol))
@@ -2762,7 +2769,7 @@ unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
 
 		/* send cdb by polling if no cdb interrupt */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(ap, link, 0);
 		break;
 
 	default:
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 8198259..1538d5b 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2284,7 +2284,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
 	}
 
 	if (qc->tf.flags & ATA_TFLAG_POLLING)
-		ata_sff_queue_pio_task(ap, 0);
+		ata_sff_queue_pio_task(ap, link, 0);
 	return 0;
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f010f18..1939d92 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -723,6 +723,7 @@ struct ata_port {
 	struct ata_ioports	ioaddr;	/* ATA cmd/ctl/dma register blocks */
 	u8			ctl;	/* cache of ATA control register */
 	u8			last_ctl;	/* Cache last written value */
+	void*			port_task_data;
 	struct delayed_work	sff_pio_task;
 #ifdef CONFIG_ATA_BMDMA
 	struct ata_bmdma_prd	*bmdma_prd;	/* BMDMA SG list */
@@ -1594,7 +1595,8 @@ extern void ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
 extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 			    u8 status, int in_wq);
-extern void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay);
+extern void ata_sff_queue_pio_task(struct ata_port *ap, void* data,
+				   unsigned long delay);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
 extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_port_intr(struct ata_port *ap,
-- 
1.7.1

Re: possible esata regression in 2.6.35

[Gwendal Grignou]

I submit a patch, but messed up with the --in-reply-to argument. Check
"libata-sff: Reenable Port Multiplier after libata-sff remodeling."
thread.

I can now see my drive:
Aug 27 17:42:36 halab-59 kernel: [  103.631293] ata_sff_exec_command:
ata5: cmd 0xEC
Aug 27 17:42:36 halab-59 kernel: [  103.634050] ata_sff_hsm_move:
ata5: protocol 2 task_state 2 (dev_stat 0x58)
Aug 27 17:42:36 halab-59 kernel: [  103.634053] ata_pio_sector: data read
Aug 27 17:42:36 halab-59 kernel: [  103.634228] ata_sff_hsm_move:
ata5: protocol 2 task_state 3 (dev_stat 0x50)
Aug 27 17:42:36 halab-59 kernel: [  103.634231] ata_sff_hsm_move:
ata5: dev 0 command complete, drv_stat 0x50

In general BMDMA does not support PMP, it assumes all qc are on ap->host.
sata_mv does it sometimes, I will submit a patch for that, after I
test the error path through-fully.

Gwendal.


On Fri, Aug 27, 2010 at 3:56 PM, Gwendal Grignou <gwendal@google.com> wrote:
> I found the problem: in ata_sff_pio_stack:
>
> struct ata_queued_cmd *qc = ap->port_task_data;
>
> has been replaced by:
>
> /* qc can be NULL if timeout occurred */
> qc = ata_qc_from_tag(ap, ap->link.active_tag);
> if (!qc)
>        return;
>
> That does not work in case of port multipler, because the link to look
> at is not ap->link. ap->link.active_tag is ATA_POISON.
>
> I will submit a patch where I re-introduce port_task_data, this time
> containing the link to to look at.
>
> Gwendal.
>
>
>
> On Fri, Aug 27, 2010 at 12:29 PM, Gwendal Grignou <gwendal@google.com> wrote:
>> I include the trace. You need windows and a lecroy satasuite to look
>> at the trace.
>>
>> Looking at extended traces, the identify never move in HSM:
>>
>> Aug 27 12:07:53 halab-59 kernel: [  548.613529] ata_sff_flush_pio_task: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613561] ata_sff_exec_command:
>> ata13: cmd 0xE4
>> Aug 27 12:07:53 halab-59 kernel: [  548.613577] ata_sff_hsm_move:
>> ata13: protocol 1 task_state 3 (dev_stat 0x50)
>> Aug 27 12:07:53 halab-59 kernel: [  548.613580] ata_sff_hsm_move:
>> ata13: dev 0 command complete, drv_stat 0x50
>> Aug 27 12:07:53 halab-59 kernel: [  548.613605] ata_sff_flush_pio_task: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613637] ata_sff_exec_command:
>> ata13: cmd 0xE4
>> Aug 27 12:07:53 halab-59 kernel: [  548.613654] ata_sff_hsm_move:
>> ata13: protocol 1 task_state 3 (dev_stat 0x50)
>> Aug 27 12:07:53 halab-59 kernel: [  548.613656] ata_sff_hsm_move:
>> ata13: dev 0 command complete, drv_stat 0x50
>> Aug 27 12:07:53 halab-59 kernel: [  548.613681] ata_sff_flush_pio_task: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613688]
>> ata_eh_revalidate_and_attach: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613691]
>> ata_eh_revalidate_and_attach: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613693]
>> ata_eh_revalidate_and_attach: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613694]
>> ata_eh_revalidate_and_attach: ENTER
>> Aug 27 12:07:53 halab-59 kernel: [  548.613725] ata_sff_exec_command:
>> ata13: cmd 0xEC
>> ...
>> Aug 27 12:08:23 halab-59 kernel: [  578.613048] __ata_port_freeze:
>> ata13 port frozen
>> Aug 27 12:08:23 halab-59 kernel: [  578.613058] ata13.03: qc timeout (cmd 0xec)
>> Aug 27 12:08:23 halab-59 kernel: [  578.613062] ata13.03: failed to
>> IDENTIFY (I/O error, err_mask=0x4)
>>
>>
>> On Fri, Aug 27, 2010 at 1:19 AM, Gwendal Grignou <gwendal@google.com> wrote:
>>> I can reproduce the problem on uptsream-linux using a PC with Marvell
>>> 7042 controller and Sil3726 PMP. Without the SIl3726, it works fine.
>>>
>>> What I can see on the SATA analyzer [I will send clean trace tomorrow]
>>> is the disk send the DATA FIS back to the PMP, but the PMP does not
>>> manage to have the data accepted by the host.
>>>
>>> Non data commands work fine.
>>>
>>> In dmesg:
>>> [10058.404047] ata29: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>>> [10058.404742] ata29.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6
>>> ports, feat 0x1/0x9
>>> [10058.405250] ata29.00: hard resetting link
>>> [10058.809151] ata29.00: link resume succeeded after 1 retries
>>> [10058.911613] ata29.01: hard resetting link
>>> [10059.217572] ata29.02: hard resetting link
>>> [10059.523572] ata29.03: hard resetting link
>>> [10059.829505] ata29.04: hard resetting link
>>> [10060.134572] ata29.05: hard resetting link
>>> [10065.440079] ata29.03: qc timeout (cmd 0xec)
>>> [10065.440085] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
>>> [10065.440092] ata29.15: hard resetting link
>>> [10065.947049] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>>> [10065.947737] ata29.00: hard resetting link
>>> [10066.352106] ata29.00: link resume succeeded after 1 retries
>>> [10066.454616] ata29.01: hard resetting link
>>> [10066.760591] ata29.02: hard resetting link
>>> [10067.066570] ata29.03: hard resetting link
>>> [10067.372505] ata29.05: hard resetting link
>>> [10077.677071] ata29.03: qc timeout (cmd 0xec)
>>> [10077.677076] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
>>> [10077.677081] ata29.15: hard resetting link
>>> [10078.184073] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>>> [10078.184783] ata29.00: hard resetting link
>>> [10078.589185] ata29.00: link resume succeeded after 1 retries
>>> [10078.691593] ata29.01: hard resetting link
>>> [10078.997592] ata29.02: hard resetting link
>>> [10079.303571] ata29.03: hard resetting link
>>> [10079.609505] ata29.04: hard resetting link
>>> [10079.914572] ata29.05: hard resetting link
>>> [10110.220078] ata29.03: qc timeout (cmd 0xec)
>>> [10110.220083] ata29.03: failed to IDENTIFY (I/O error, err_mask=0x4)
>>> [10110.220087] ata29.03: failed to recover link after 3 tries, disabling
>>> [10110.220094] ata29.15: hard resetting link
>>> [10110.727044] ata29.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
>>> [10111.033375] ata29.00: hard resetting link
>>> [10111.338571] ata29.01: hard resetting link
>>> [10111.644591] ata29.02: hard resetting link
>>> [10111.950594] ata29.05: hard resetting link
>>> [10112.256643] ata29: EH complete
>>>
>>>
>>> Gwendal.
>>>
>>>
>>>
>>>
>>> On Sun, Aug 22, 2010 at 12:57 PM, Jeff Garzik <jeff@garzik.org> wrote:
>>>> On 08/22/2010 03:54 PM, Jeff Garzik wrote:
>>>>>
>>>>> On 08/21/2010 02:52 PM, Nicolas Jungers wrote:
>>>>>>
>>>>>> My arm box doesn't succeed to use my esata port multiplier (addonics
>>>>>> sil3726 based). It was working well with 2.6.34.1 and 2.6.34.4 but not
>>>>>> with both 2.6.35.2 and 2.6.35.3. I haven't test other kernels.
>>>>>>
>>>>>> The kernels are from http://sheeva.with-linux.com/sheeva/ with for
>>>>>> example the following config
>>>>>> http://sheeva.with-linux.com/sheeva/2.6.35.3/sheeva-2.6.35.3.config
>>>>>>
>>>>>> The symptoms are in the console a loop on the esata links. Here is the
>>>>>> start of it:
>>>>>>
>>>>>> ata2: exception Emask 0x10 SAct 0x0 SErr 0x4010000 action 0xe frozen
>>>>>> ata2: edma_err_cause=00000010 pp_flags=00000000, dev connect
>>>>>> ata2: SError: { PHYRdyChg DevExch }
>>>>>> ata2: hard resetting link
>>>>>> ata2: SATA link up 3.0 Gbps (SStatus 123 SControl F300)
>>>>>> ata2.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
>>>>>
>>>>> Can you post or link to the entire dmesg?
>>>>>
>>>>> Notably, we need to see the probe messages to determine what SATA chip
>>>>> you are using... From the edma_err_cause config I'd guess sata_mv, but
>>>>> more info would be useful.
>>>>
>>>> Nevermind, I see the reply (it got auto-sorted into the wrong folder
>>>> locally).
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Tejun Heo]

Hello,

On 08/29/2010 02:31 AM, Gwendal Grignou wrote:
> Re-introduce port_task_data to hold the link on the which the current
> request is in progress. It allows support of links behind port multiplier.
> 
> Not all libata-sff is PMP compliant. Code for native BMDMA controller
> does not take in accound PMP.
> 
> Tested on Marvell 7042 and Sil7526.

So sata_mv is using SFF state machine for PMP devices too.  Didn't see
that coming.  Thanks for fixing this.  Can you please consider the
followings?

* Make ata_sff_queue_pio_task() take a @qc (or @link if @qc can't be
  used for whatever reason) and @delay, instead of taking @ap, @link
  and @delay.

* Change ata_port->port_task_data to something more specific.  ie.
  sff_pio_task_qc or sff_pio_task_link.

Thanks.

-- 
tejun

[PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Gwendal Grignou]

Keep track of the link on the which the current request is in progress.
It allows support of links behind port multiplier.

Not all libata-sff is PMP compliant. Code for native BMDMA controller
does not take in accound PMP.

Tested on Marvell 7042 and Sil7526.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-sff.c |   25 ++++++++++++++++---------
 drivers/ata/sata_mv.c    |    2 +-
 include/linux/libata.h   |    3 ++-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3b82d8e..23d69a4 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1042,7 +1042,8 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 		     u8 status, int in_wq)
 {
-	struct ata_eh_info *ehi = &ap->link.eh_info;
+	struct ata_link *link = qc->dev->link;
+	struct ata_eh_info *ehi = &link->eh_info;
 	unsigned long flags = 0;
 	int poll_next;
 
@@ -1298,8 +1299,11 @@ fsm_start:
 }
 EXPORT_SYMBOL_GPL(ata_sff_hsm_move);
 
-void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay)
+void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay)
 {
+	struct ata_port *ap = link->ap;
+	ap->sff_pio_task_link = link;
+
 	/* may fail if ata_sff_flush_pio_task() in progress */
 	queue_delayed_work(ata_sff_wq, &ap->sff_pio_task,
 			   msecs_to_jiffies(delay));
@@ -1321,12 +1325,13 @@ static void ata_sff_pio_task(struct work_struct *work)
 {
 	struct ata_port *ap =
 		container_of(work, struct ata_port, sff_pio_task.work);
+	struct ata_link *link = ap->sff_pio_task_link;
 	struct ata_queued_cmd *qc;
 	u8 status;
 	int poll_next;
 
 	/* qc can be NULL if timeout occurred */
-	qc = ata_qc_from_tag(ap, ap->link.active_tag);
+	qc = ata_qc_from_tag(ap, link->active_tag);
 	if (!qc)
 		return;
 
@@ -1345,7 +1350,7 @@ fsm_start:
 		msleep(2);
 		status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
 		if (status & ATA_BUSY) {
-			ata_sff_queue_pio_task(ap, ATA_SHORT_PAUSE);
+			ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
 			return;
 		}
 	}
@@ -1376,6 +1381,7 @@ fsm_start:
 unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_link *link = qc->dev->link;
 
 	/* Use polling pio if the LLD doesn't handle
 	 * interrupt driven pio and atapi CDB interrupt.
@@ -1396,7 +1402,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		ap->hsm_task_state = HSM_ST_LAST;
 
 		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 
 		break;
 
@@ -1409,7 +1415,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		if (qc->tf.flags & ATA_TFLAG_WRITE) {
 			/* PIO data out protocol */
 			ap->hsm_task_state = HSM_ST_FIRST;
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 
 			/* always send first data block using the
 			 * ata_sff_pio_task() codepath.
@@ -1419,7 +1425,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 			ap->hsm_task_state = HSM_ST;
 
 			if (qc->tf.flags & ATA_TFLAG_POLLING)
-				ata_sff_queue_pio_task(ap, 0);
+				ata_sff_queue_pio_task(link, 0);
 
 			/* if polling, ata_sff_pio_task() handles the
 			 * rest.  otherwise, interrupt handler takes
@@ -1441,7 +1447,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		/* send cdb by polling if no cdb interrupt */
 		if ((!(qc->dev->flags & ATA_DFLAG_CDB_INTR)) ||
 		    (qc->tf.flags & ATA_TFLAG_POLLING))
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 		break;
 
 	default:
@@ -2734,6 +2740,7 @@ EXPORT_SYMBOL_GPL(ata_bmdma_dumb_qc_prep);
 unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_link *link = qc->dev->link;
 
 	/* defer PIO handling to sff_qc_issue */
 	if (!ata_is_dma(qc->tf.protocol))
@@ -2762,7 +2769,7 @@ unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
 
 		/* send cdb by polling if no cdb interrupt */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 		break;
 
 	default:
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 8198259..a9fd970 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2284,7 +2284,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
 	}
 
 	if (qc->tf.flags & ATA_TFLAG_POLLING)
-		ata_sff_queue_pio_task(ap, 0);
+		ata_sff_queue_pio_task(link, 0);
 	return 0;
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f010f18..74596a3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -723,6 +723,7 @@ struct ata_port {
 	struct ata_ioports	ioaddr;	/* ATA cmd/ctl/dma register blocks */
 	u8			ctl;	/* cache of ATA control register */
 	u8			last_ctl;	/* Cache last written value */
+	struct ata_link*	sff_pio_task_link; /* link currently used */
 	struct delayed_work	sff_pio_task;
 #ifdef CONFIG_ATA_BMDMA
 	struct ata_bmdma_prd	*bmdma_prd;	/* BMDMA SG list */
@@ -1594,7 +1595,7 @@ extern void ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
 extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 			    u8 status, int in_wq);
-extern void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay);
+extern void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
 extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_port_intr(struct ata_port *ap,
-- 
1.7.1

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Gwendal Grignou]

On Sun, Aug 29, 2010 at 2:05 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 08/29/2010 02:31 AM, Gwendal Grignou wrote:
>> Re-introduce port_task_data to hold the link on the which the current
>> request is in progress. It allows support of links behind port multiplier.
>>
>> Not all libata-sff is PMP compliant. Code for native BMDMA controller
>> does not take in accound PMP.
>>
>> Tested on Marvell 7042 and Sil7526.
>
> So sata_mv is using SFF state machine for PMP devices too.  Didn't see
> that coming.  Thanks for fixing this.  Can you please consider the
> followings?
>
> * Make ata_sff_queue_pio_task() take a @qc (or @link if @qc can't be
>  used for whatever reason) and @delay, instead of taking @ap, @link
>  and @delay.
Done. I keep link, because qc could be reused if the command timeout
while the task is delayed.
>
> * Change ata_port->port_task_data to something more specific.  ie.
>  sff_pio_task_qc or sff_pio_task_link.
Done.

path is next.
>
> Thanks.
>
> --
> tejun
>

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Tejun Heo]

Hello,

On 08/30/2010 07:17 PM, Gwendal Grignou wrote:
> Keep track of the link on the which the current request is in progress.
> It allows support of links behind port multiplier.
> 
> Not all libata-sff is PMP compliant. Code for native BMDMA controller
> does not take in accound PMP.

Can you please elaborate a bit more on what broke and how this patch
fixes the problem?

> -void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay)
> +void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay)
>  {
> +	struct ata_port *ap = link->ap;

New line here, please.

> +	ap->sff_pio_task_link = link;

It would also be useful to have WARN/BUG_ON() to make sure no two
links try to use pio_task at the same time.  ie. Set
ap->sff_pio_task_link here and clear it with NULL when done and make
sure it's NULL before setting it.

Otherwise, looks good to me.

Thanks!

-- 
tejun

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Gwendal Grignou]

On Tue, Aug 31, 2010 at 1:04 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 08/30/2010 07:17 PM, Gwendal Grignou wrote:
>> Keep track of the link on the which the current request is in progress.
>> It allows support of links behind port multiplier.
>>
>> Not all libata-sff is PMP compliant. Code for native BMDMA controller
>> does not take in accound PMP.
>
> Can you please elaborate a bit more on what broke and how this patch
> fixes the problem?
Before this patch, all libata-sff assumes the qc in progess is tied to
ap->link, the host port link.
That's fine as long as the controllers do not support port multiplier,
which is the case of all controller inheriting ata_sff_port_ops except
some controllers managed by sata_mv.
Also, before the libata-ssf reorg, it did not matter, qc was given the
sff task directly.

However, sata_mv supports port multiplier and use part of libata-sff
to hanlde PIO commands to disks. qc sent to disk behind port
multiplier are tight to one of element pmp_link array.
Therefore, the part of libata-sff sata_mv exercises must be retrieve
qc from the provided link instead of ap->link.

>
>> -void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay)
>> +void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay)
>>  {
>> +     struct ata_port *ap = link->ap;
>
> New line here, please.
Done
>
>> +     ap->sff_pio_task_link = link;
>
> It would also be useful to have WARN/BUG_ON() to make sure no two
> links try to use pio_task at the same time.  ie. Set
> ap->sff_pio_task_link here and clear it with NULL when done and make
> sure it's NULL before setting it.
Add some WARN/BUG. I set link to NULL very early, I believe it is
cleaner than setting it in hsm_move() itself.
Patch after the break.

Gwendal.
>
> Otherwise, looks good to me.
>
> Thanks!
>
> --
> tejun
>

[PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Gwendal Grignou]

Keep track of the link on the which the current request is in progress.
It allows support of links behind port multiplier.

Not all libata-sff is PMP compliant. Code for native BMDMA controller
does not take in accound PMP.

Tested on Marvell 7042 and Sil7526.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-sff.c |   38 ++++++++++++++++++++++++++++----------
 drivers/ata/sata_mv.c    |    2 +-
 include/linux/libata.h   |    3 ++-
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 3b82d8e..55cf7f4 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1042,7 +1042,8 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 		     u8 status, int in_wq)
 {
-	struct ata_eh_info *ehi = &ap->link.eh_info;
+	struct ata_link *link = qc->dev->link;
+	struct ata_eh_info *ehi = &link->eh_info;
 	unsigned long flags = 0;
 	int poll_next;
 
@@ -1298,8 +1299,14 @@ fsm_start:
 }
 EXPORT_SYMBOL_GPL(ata_sff_hsm_move);
 
-void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay)
+void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay)
 {
+	struct ata_port *ap = link->ap;
+
+	WARN_ON((ap->sff_pio_task_link != NULL) &&
+		(ap->sff_pio_task_link != link));
+	ap->sff_pio_task_link = link;
+
 	/* may fail if ata_sff_flush_pio_task() in progress */
 	queue_delayed_work(ata_sff_wq, &ap->sff_pio_task,
 			   msecs_to_jiffies(delay));
@@ -1321,14 +1328,18 @@ static void ata_sff_pio_task(struct work_struct *work)
 {
 	struct ata_port *ap =
 		container_of(work, struct ata_port, sff_pio_task.work);
+	struct ata_link *link = ap->sff_pio_task_link;
 	struct ata_queued_cmd *qc;
 	u8 status;
 	int poll_next;
 
+	BUG_ON(ap->sff_pio_task_link == NULL); 
 	/* qc can be NULL if timeout occurred */
-	qc = ata_qc_from_tag(ap, ap->link.active_tag);
-	if (!qc)
+	qc = ata_qc_from_tag(ap, link->active_tag);
+	if (!qc) {
+		ap->sff_pio_task_link = NULL;
 		return;
+	}
 
 fsm_start:
 	WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);
@@ -1345,11 +1356,16 @@ fsm_start:
 		msleep(2);
 		status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
 		if (status & ATA_BUSY) {
-			ata_sff_queue_pio_task(ap, ATA_SHORT_PAUSE);
+			ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
 			return;
 		}
 	}
 
+	/*
+	 * hsm_move() may trigger another command to be processed.
+	 * clean the link beforehand.
+	 */
+	ap->sff_pio_task_link = NULL;
 	/* move the HSM */
 	poll_next = ata_sff_hsm_move(ap, qc, status, 1);
 
@@ -1376,6 +1392,7 @@ fsm_start:
 unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_link *link = qc->dev->link;
 
 	/* Use polling pio if the LLD doesn't handle
 	 * interrupt driven pio and atapi CDB interrupt.
@@ -1396,7 +1413,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		ap->hsm_task_state = HSM_ST_LAST;
 
 		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 
 		break;
 
@@ -1409,7 +1426,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		if (qc->tf.flags & ATA_TFLAG_WRITE) {
 			/* PIO data out protocol */
 			ap->hsm_task_state = HSM_ST_FIRST;
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 
 			/* always send first data block using the
 			 * ata_sff_pio_task() codepath.
@@ -1419,7 +1436,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 			ap->hsm_task_state = HSM_ST;
 
 			if (qc->tf.flags & ATA_TFLAG_POLLING)
-				ata_sff_queue_pio_task(ap, 0);
+				ata_sff_queue_pio_task(link, 0);
 
 			/* if polling, ata_sff_pio_task() handles the
 			 * rest.  otherwise, interrupt handler takes
@@ -1441,7 +1458,7 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc)
 		/* send cdb by polling if no cdb interrupt */
 		if ((!(qc->dev->flags & ATA_DFLAG_CDB_INTR)) ||
 		    (qc->tf.flags & ATA_TFLAG_POLLING))
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 		break;
 
 	default:
@@ -2734,6 +2751,7 @@ EXPORT_SYMBOL_GPL(ata_bmdma_dumb_qc_prep);
 unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_link *link = qc->dev->link;
 
 	/* defer PIO handling to sff_qc_issue */
 	if (!ata_is_dma(qc->tf.protocol))
@@ -2762,7 +2780,7 @@ unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc)
 
 		/* send cdb by polling if no cdb interrupt */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			ata_sff_queue_pio_task(ap, 0);
+			ata_sff_queue_pio_task(link, 0);
 		break;
 
 	default:
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 8198259..a9fd970 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2284,7 +2284,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
 	}
 
 	if (qc->tf.flags & ATA_TFLAG_POLLING)
-		ata_sff_queue_pio_task(ap, 0);
+		ata_sff_queue_pio_task(link, 0);
 	return 0;
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f010f18..74596a3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -723,6 +723,7 @@ struct ata_port {
 	struct ata_ioports	ioaddr;	/* ATA cmd/ctl/dma register blocks */
 	u8			ctl;	/* cache of ATA control register */
 	u8			last_ctl;	/* Cache last written value */
+	struct ata_link*	sff_pio_task_link; /* link currently used */
 	struct delayed_work	sff_pio_task;
 #ifdef CONFIG_ATA_BMDMA
 	struct ata_bmdma_prd	*bmdma_prd;	/* BMDMA SG list */
@@ -1594,7 +1595,7 @@ extern void ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
 extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 			    u8 status, int in_wq);
-extern void ata_sff_queue_pio_task(struct ata_port *ap, unsigned long delay);
+extern void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
 extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_port_intr(struct ata_port *ap,
-- 
1.7.1

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Jeff Garzik]

On 08/31/2010 07:20 PM, Gwendal Grignou wrote:
> Keep track of the link on the which the current request is in progress.
> It allows support of links behind port multiplier.
>
> Not all libata-sff is PMP compliant. Code for native BMDMA controller
> does not take in accound PMP.
>
> Tested on Marvell 7042 and Sil7526.
>
> Signed-off-by: Gwendal Grignou<gwendal@google.com>
> ---
>   drivers/ata/libata-sff.c |   38 ++++++++++++++++++++++++++++----------
>   drivers/ata/sata_mv.c    |    2 +-
>   include/linux/libata.h   |    3 ++-
>   3 files changed, 31 insertions(+), 12 deletions(-)

FWIW, the normal standard for revised patches is to include a short 
changelog following the "---", but preceding the patch itself. 
Something like:

---------------------SNIP-----------------------
Not all libata-sff is PMP compliant. Code for native BMDMA controller
does not take in accound PMP.

Tested on Marvell 7042 and Sil7526.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
v3: cleanups, add WARN_ON() etc
v2: ata_sff_queue_pio_task takes a qc etc

  drivers/ata/libata-sff.c |   38 ++++++++++++++++++++++++++++----------
  drivers/ata/sata_mv.c    |    2 +-
  include/linux/libata.h   |    3 ++-
  3 files changed, 31 insertions(+), 12 deletions(-)
---------------------SNIP-----------------------



I'll queue this up along with the other libata fixes tonight (unless 
there are further revisions of course:))

	Jeff

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Gwendal Grignou]

Got it for the patch. Thanks for taking care of the submission.
Gwendal.

On Tue, Aug 31, 2010 at 4:32 PM, Jeff Garzik <jeff@garzik.org> wrote:
> On 08/31/2010 07:20 PM, Gwendal Grignou wrote:
>>
>> Keep track of the link on the which the current request is in progress.
>> It allows support of links behind port multiplier.
>>
>> Not all libata-sff is PMP compliant. Code for native BMDMA controller
>> does not take in accound PMP.
>>
>> Tested on Marvell 7042 and Sil7526.
>>
>> Signed-off-by: Gwendal Grignou<gwendal@google.com>
>> ---
>>  drivers/ata/libata-sff.c |   38 ++++++++++++++++++++++++++++----------
>>  drivers/ata/sata_mv.c    |    2 +-
>>  include/linux/libata.h   |    3 ++-
>>  3 files changed, 31 insertions(+), 12 deletions(-)
>
> FWIW, the normal standard for revised patches is to include a short
> changelog following the "---", but preceding the patch itself. Something
> like:
>
> ---------------------SNIP-----------------------
> Not all libata-sff is PMP compliant. Code for native BMDMA controller
> does not take in accound PMP.
>
> Tested on Marvell 7042 and Sil7526.
>
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> ---
> v3: cleanups, add WARN_ON() etc
> v2: ata_sff_queue_pio_task takes a qc etc
>
>  drivers/ata/libata-sff.c |   38 ++++++++++++++++++++++++++++----------
>  drivers/ata/sata_mv.c    |    2 +-
>  include/linux/libata.h   |    3 ++-
>  3 files changed, 31 insertions(+), 12 deletions(-)
> ---------------------SNIP-----------------------
>
>
>
> I'll queue this up along with the other libata fixes tonight (unless there
> are further revisions of course:))
>
>        Jeff
>
>
>

Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

[Tejun Heo]

Hello,

On 09/01/2010 01:17 AM, Gwendal Grignou wrote:
> On Tue, Aug 31, 2010 at 1:04 AM, Tejun Heo <tj@kernel.org> wrote:
>> On 08/30/2010 07:17 PM, Gwendal Grignou wrote:
>>> Keep track of the link on the which the current request is in progress.
>>> It allows support of links behind port multiplier.
>>>
>>> Not all libata-sff is PMP compliant. Code for native BMDMA controller
>>> does not take in accound PMP.
>>
>> Can you please elaborate a bit more on what broke and how this patch
>> fixes the problem?
> Before this patch, all libata-sff assumes the qc in progess is tied to
> ap->link, the host port link.
> That's fine as long as the controllers do not support port multiplier,
> which is the case of all controller inheriting ata_sff_port_ops except
> some controllers managed by sata_mv.
> Also, before the libata-ssf reorg, it did not matter, qc was given the
> sff task directly.
> 
> However, sata_mv supports port multiplier and use part of libata-sff
> to hanlde PIO commands to disks. qc sent to disk behind port
> multiplier are tight to one of element pmp_link array.
> Therefore, the part of libata-sff sata_mv exercises must be retrieve
> qc from the provided link instead of ap->link.

Heh, I meant to elaborate in the patch description. :-) Sorry about
not being clearer.

>> It would also be useful to have WARN/BUG_ON() to make sure no two
>> links try to use pio_task at the same time.  ie. Set
>> ap->sff_pio_task_link here and clear it with NULL when done and make
>> sure it's NULL before setting it.
>
> Add some WARN/BUG. I set link to NULL very early, I believe it is
> cleaner than setting it in hsm_move() itself.
> Patch after the break.

Thanks.

-- 
tejun