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

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

[Kalra Ashish-B00888]

Sent from my HTC

-----Original Message-----
From: Tejun Heo <tj@kernel.org>
Sent: 01 September 2010 1:52 PM
To: Gwendal Grignou <gwendal@google.com>
Cc: jeff@garzik.org <jeff@garzik.org>; nicolas@jungers.net <nicolas@jungers.net>; linux-ide@vger.kernel.org <linux-ide@vger.kernel.org>
Subject: Re: [PATCH] libata-sff: Reenable Port Multiplier after libata-sff remodeling.

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
--
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: [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.

[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
>

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

[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.

[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
>