[PATCH #upstream 1/2] libata: kill dead code paths in reset path

[PATCH #upstream 1/2] libata: kill dead code paths in reset path

[Tejun Heo]

Some code paths which had been made obsolete by recent reset
simplification were still around.  Kill them.

* ata_eh_reset() checked for ATA_DEV_UNKNOWN to determine
  classification failure.  This is no longer applicable.

* ata_do_reset() should convert ATA_DEV_UNKNOWN to ATA_DEV_NONE
  regardless of reset result (e.g. -EAGAIN).

* LLDs don't need to convert ATA_DEV_UNKNOWN to ATA_DEV_NONE.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c     |   19 +------------------
 drivers/ata/sata_inic162x.c |    2 --
 drivers/ata/sata_sil24.c    |    3 ---
 3 files changed, 1 insertion(+), 23 deletions(-)

Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -2052,15 +2052,13 @@ static int ata_do_reset(struct ata_link 
 		classes[dev->devno] = ATA_DEV_UNKNOWN;
 
 	rc = reset(link, classes, deadline);
-	if (rc)
-		return rc;
 
 	/* convert all ATA_DEV_UNKNOWN to ATA_DEV_NONE */
 	ata_link_for_each_dev(dev, link)
 		if (classes[dev->devno] == ATA_DEV_UNKNOWN)
 			classes[dev->devno] = ATA_DEV_NONE;
 
-	return 0;
+	return rc;
 }
 
 static int ata_eh_followup_srst_needed(struct ata_link *link,
@@ -2209,21 +2207,6 @@ int ata_eh_reset(struct ata_link *link, 
 	if (rc && rc != -EAGAIN)
 		goto fail;
 
-	/* was classification successful? */
-	if (classify && classes[0] == ATA_DEV_UNKNOWN &&
-	    !(lflags & ATA_LFLAG_ASSUME_CLASS)) {
-		if (try < max_tries) {
-			ata_link_printk(link, KERN_WARNING,
-					"classification failed\n");
-			rc = -EINVAL;
-			goto fail;
-		}
-
-		ata_link_printk(link, KERN_WARNING,
-				"classfication failed, assuming ATA\n");
-		lflags |= ATA_LFLAG_ASSUME_ATA;
-	}
-
  done:
 	ata_link_for_each_dev(dev, link) {
 		/* After the reset, the device state is PIO 0 and the
Index: work/drivers/ata/sata_inic162x.c
===================================================================
--- work.orig/drivers/ata/sata_inic162x.c
+++ work/drivers/ata/sata_inic162x.c
@@ -428,8 +428,6 @@ static int inic_hardreset(struct ata_lin
 
 		ata_sff_tf_read(ap, &tf);
 		*class = ata_dev_classify(&tf);
-		if (*class == ATA_DEV_UNKNOWN)
-			*class = ATA_DEV_NONE;
 	}
 
 	return 0;
Index: work/drivers/ata/sata_sil24.c
===================================================================
--- work.orig/drivers/ata/sata_sil24.c
+++ work/drivers/ata/sata_sil24.c
@@ -693,9 +693,6 @@ static int sil24_softreset(struct ata_li
 	sil24_read_tf(ap, 0, &tf);
 	*class = ata_dev_classify(&tf);
 
-	if (*class == ATA_DEV_UNKNOWN)
-		*class = ATA_DEV_NONE;
-
  out:
 	DPRINTK("EXIT, class=%u\n", *class);
 	return 0;

[PATCH #upstream 2/2] libata: move link onlineness check out of softreset methods

[Tejun Heo]

Currently, SATA softresets should do link onlineness check before
actually performing SRST protocol but it doesn't really belong to
softreset.

This patch moves onlineness check in softreset to ata_eh_reset() and
ata_eh_followup_srst_needed() to clean up code and help future sata_mv
changes which need clear separation between SCR and TF accesses.

sata_fsl is peculiar in that its softreset really isn't softreset but
combination of hardreset and softreset.  This patch adds dummy private
->prereset to keep the current behavior but the driver really should
implement separate hard and soft resets and return -EAGAIN from
hardreset if it should be follwed by softreset.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
Jeff, this and the previous patch are on top of the modularize
patchset.  Mark, does this look good enough?  Here's git tree
containing everything.

  http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=for-mlord
  git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git for-mlord

Thanks.

 drivers/ata/ahci.c        |    6 ------
 drivers/ata/libata-core.c |    4 ++++
 drivers/ata/libata-eh.c   |    2 +-
 drivers/ata/libata-sff.c  |    6 ------
 drivers/ata/pata_bf54x.c  |    6 ------
 drivers/ata/pata_scc.c    |    6 ------
 drivers/ata/sata_fsl.c    |   10 ++++++++++
 drivers/ata/sata_sil24.c  |    7 -------
 8 files changed, 15 insertions(+), 32 deletions(-)

Index: work/drivers/ata/ahci.c
===================================================================
--- work.orig/drivers/ata/ahci.c
+++ work/drivers/ata/ahci.c
@@ -1273,12 +1273,6 @@ static int ahci_softreset(struct ata_lin
 
 	DPRINTK("ENTER\n");
 
-	if (ata_link_offline(link)) {
-		DPRINTK("PHY reports no device\n");
-		*class = ATA_DEV_NONE;
-		return 0;
-	}
-
 	/* prepare for SRST (AHCI-1.1 10.4.1) */
 	rc = ahci_kick_engine(ap, 1);
 	if (rc && rc != -EOPNOTSUPP)
Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -3541,6 +3541,10 @@ int ata_std_prereset(struct ata_link *li
 					"link for reset (errno=%d)\n", rc);
 	}
 
+	/* no point in trying softreset on offline link */
+	if (ata_link_offline(link))
+		ehc->i.action &= ~ATA_EH_SOFTRESET;
+
 	return 0;
 }
 
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -2065,7 +2065,7 @@ static int ata_eh_followup_srst_needed(s
 				       int rc, int classify,
 				       const unsigned int *classes)
 {
-	if (link->flags & ATA_LFLAG_NO_SRST)
+	if ((link->flags & ATA_LFLAG_NO_SRST) || ata_link_offline(link))
 		return 0;
 	if (rc == -EAGAIN) {
 		if (classify)
Index: work/drivers/ata/libata-sff.c
===================================================================
--- work.orig/drivers/ata/libata-sff.c
+++ work/drivers/ata/libata-sff.c
@@ -1889,11 +1889,6 @@ int ata_sff_softreset(struct ata_link *l
 
 	DPRINTK("ENTER\n");
 
-	if (ata_link_offline(link)) {
-		classes[0] = ATA_DEV_NONE;
-		goto out;
-	}
-
 	/* determine if device 0/1 are present */
 	if (ata_devchk(ap, 0))
 		devmask |= (1 << 0);
@@ -1919,7 +1914,6 @@ int ata_sff_softreset(struct ata_link *l
 		classes[1] = ata_sff_dev_classify(&link->device[1],
 						  devmask & (1 << 1), &err);
 
- out:
 	DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
 	return 0;
 }
Index: work/drivers/ata/pata_bf54x.c
===================================================================
--- work.orig/drivers/ata/pata_bf54x.c
+++ work/drivers/ata/pata_bf54x.c
@@ -1103,11 +1103,6 @@ static int bfin_softreset(struct ata_lin
 	unsigned int devmask = 0, err_mask;
 	u8 err;
 
-	if (ata_link_offline(link)) {
-		classes[0] = ATA_DEV_NONE;
-		goto out;
-	}
-
 	/* determine if device 0/1 are present */
 	if (bfin_devchk(ap, 0))
 		devmask |= (1 << 0);
@@ -1132,7 +1127,6 @@ static int bfin_softreset(struct ata_lin
 		classes[1] = ata_sff_dev_classify(&ap->link.device[1],
 					devmask & (1 << 1), &err);
 
- out:
 	return 0;
 }
 
Index: work/drivers/ata/pata_scc.c
===================================================================
--- work.orig/drivers/ata/pata_scc.c
+++ work/drivers/ata/pata_scc.c
@@ -615,11 +615,6 @@ static int scc_softreset(struct ata_link
 
 	DPRINTK("ENTER\n");
 
-	if (ata_link_offline(link)) {
-		classes[0] = ATA_DEV_NONE;
-		goto out;
-	}
-
 	/* determine if device 0/1 are present */
 	if (scc_devchk(ap, 0))
 		devmask |= (1 << 0);
@@ -645,7 +640,6 @@ static int scc_softreset(struct ata_link
 		classes[1] = ata_sff_dev_classify(&ap->link.device[1],
 						  devmask & (1 << 1), &err);
 
- out:
 	DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
 	return 0;
 }
Index: work/drivers/ata/sata_fsl.c
===================================================================
--- work.orig/drivers/ata/sata_fsl.c
+++ work/drivers/ata/sata_fsl.c
@@ -678,6 +678,15 @@ static unsigned int sata_fsl_dev_classif
 	return ata_dev_classify(&tf);
 }
 
+static int sata_fsl_prereset(struct ata_linke *link, unsigned long deadline)
+{
+	/* FIXME: Never skip softreset, sata_fsl_softreset() is
+	 * combination of soft and hard resets.  sata_fsl_softreset()
+	 * needs to be splitted into soft and hard resets.
+	 */
+	return 0;
+}
+
 static int sata_fsl_softreset(struct ata_link *link, unsigned int *class,
 			      unsigned long deadline)
 {
@@ -1157,6 +1166,7 @@ static const struct ata_port_operations 
 
 	.freeze = sata_fsl_freeze,
 	.thaw = sata_fsl_thaw,
+	.prereset = sata_fsl_prereset,
 	.softreset = sata_fsl_softreset,
 	.post_internal_cmd = sata_fsl_post_internal_cmd,
 
Index: work/drivers/ata/sata_sil24.c
===================================================================
--- work.orig/drivers/ata/sata_sil24.c
+++ work/drivers/ata/sata_sil24.c
@@ -663,12 +663,6 @@ static int sil24_softreset(struct ata_li
 
 	DPRINTK("ENTER\n");
 
-	if (ata_link_offline(link)) {
-		DPRINTK("PHY reports no device\n");
-		*class = ATA_DEV_NONE;
-		goto out;
-	}
-
 	/* put the port into known state */
 	if (sil24_init_port(ap)) {
 		reason = "port not ready";
@@ -693,7 +687,6 @@ static int sil24_softreset(struct ata_li
 	sil24_read_tf(ap, 0, &tf);
 	*class = ata_dev_classify(&tf);
 
- out:
 	DPRINTK("EXIT, class=%u\n", *class);
 	return 0;
 

Re: [PATCH #upstream 1/2] libata: kill dead code paths in reset path

[Jeff Garzik]

Tejun Heo wrote:
> Some code paths which had been made obsolete by recent reset
> simplification were still around.  Kill them.
> 
> * ata_eh_reset() checked for ATA_DEV_UNKNOWN to determine
>  classification failure.  This is no longer applicable.
> 
> * ata_do_reset() should convert ATA_DEV_UNKNOWN to ATA_DEV_NONE
>  regardless of reset result (e.g. -EAGAIN).
> 
> * LLDs don't need to convert ATA_DEV_UNKNOWN to ATA_DEV_NONE.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> drivers/ata/libata-eh.c     |   19 +------------------
> drivers/ata/sata_inic162x.c |    2 --
> drivers/ata/sata_sil24.c    |    3 ---
> 3 files changed, 1 insertion(+), 23 deletions(-)

applied 1-2

Mark, let us know how we stand WRT sata_mv PMP needs, this should help.

Re: [PATCH #upstream 1/2] libata: kill dead code paths in reset path

[Mark Lord]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Some code paths which had been made obsolete by recent reset
>> simplification were still around.  Kill them.
>>
>> * ata_eh_reset() checked for ATA_DEV_UNKNOWN to determine
>>  classification failure.  This is no longer applicable.
>>
>> * ata_do_reset() should convert ATA_DEV_UNKNOWN to ATA_DEV_NONE
>>  regardless of reset result (e.g. -EAGAIN).
>>
>> * LLDs don't need to convert ATA_DEV_UNKNOWN to ATA_DEV_NONE.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>> drivers/ata/libata-eh.c     |   19 +------------------
>> drivers/ata/sata_inic162x.c |    2 --
>> drivers/ata/sata_sil24.c    |    3 ---
>> 3 files changed, 1 insertion(+), 23 deletions(-)
> 
> applied 1-2
> 
> Mark, let us know how we stand WRT sata_mv PMP needs, this should help.
..

I plan to spend Monday updating the sata_mv PMP support over
to the newly restructured libata helpers from Tejun, so we'll
know for sure at that time.  Should be good, though.

There's also an existing reworked mv_hardreset patch to go into #upstream.
Did you catch that one?  ("[PATCH] sata_mv rework hardreset sequence")

If all goes well, the re-spun PMP support for sata_mv should
appear here as patches by Tuesday or so.

After that, the IRQ/EH rework will follow,
and then various errata fixes as I get to them.

There's a small chance that ATAPI support may also make
the merge window, but I'm not holding my breath on that one.

Jens is currently poking at rudimentary target mode
support for sata_mv, but it still has quite a ways to go.

Cheers

sata_mv & pmp support

[Mark Lord]

> Tejun Heo wrote:
>Currently, SATA softresets should do link onlineness check before
>actually performing SRST protocol but it doesn't really belong to
>softreset.
>
>This patch moves onlineness check in softreset to ata_eh_reset() and
>ata_eh_followup_srst_needed() to clean up code and help future sata_mv
>changes which need clear separation between SCR and TF accesses. 

Jeff Garzik wrote:
>Mark, let us know how we stand WRT sata_mv PMP needs, this should help.
 
Mark Lord wrote:
> I plan to spend Monday updating the sata_mv PMP support over
> to the newly restructured libata helpers from Tejun, so we'll
> know for sure at that time.  Should be good, though.

Well, I've spent much of the day porting to the new interfaces,
and then trying to get things working again.

One port multiplier card turned out to be totally dead.
It was never found (today) by libata, and got quite hot for some reason.
Since it is probably now toast, I've set it aside.

The other port multiplier here is different, and was not used
in the first round of tests.  So far, it gets detected by libata,
but I'm temporarily stuck on the old issue of libata not finding
any of the connected drives.  So I probably don't have the myriad
of pre/hard/soft/pmp/post reset handlers configured just right yet.

To establish some kind of baseline, I'm going to reimplement the
pmp_read/write hooks I used to have, and see if things come alive
again with those.  And then figure out again what has to change
to get rid of them.


Tejun:

Do you have any recommendations on exactly what should be in
mv_pmp_softreset() and mv_softreset() and mv_hardreset() under this scheme?

My basic attempt was to try this:

No prereset or postreset handlers of any kind.

mv_hardreset():  as in the hardreset rework patch (yet to be picked up
by Jeff), plus a call to mv_select_pmp(link->ap, SATA_PMP_CTRL_PORT)
before the sata_link_hardreset() wrapper loop.

mv_softreset():
First do mv_select_pmp(link->ap, SATA_PMP_CTRL_PORT),
and then call ata_sff_softreset().

mv_pmp_softreset():
First do mv_select_pmp(link->ap, link->pmp),
and then call ata_sff_softreset().

Resets of the pmp ports always fail with SRST failed (errno=-16).

Re: sata_mv & pmp support

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> Well, I've spent much of the day porting to the new interfaces,
> and then trying to get things working again.

Heh... sorry about the trouble.

> One port multiplier card turned out to be totally dead.
> It was never found (today) by libata, and got quite hot for some reason.
> Since it is probably now toast, I've set it aside.
> 
> The other port multiplier here is different, and was not used
> in the first round of tests.  So far, it gets detected by libata,
 >
> but I'm temporarily stuck on the old issue of libata not finding
> any of the connected drives.  So I probably don't have the myriad
> of pre/hard/soft/pmp/post reset handlers configured just right yet.

Which PMP are you using?  SIMG one or marvell one?

> To establish some kind of baseline, I'm going to reimplement the
> pmp_read/write hooks I used to have, and see if things come alive
> again with those.  And then figure out again what has to change
> to get rid of them.
> 
> 
> Tejun:
> 
> Do you have any recommendations on exactly what should be in
> mv_pmp_softreset() and mv_softreset() and mv_hardreset() under this scheme?
> 
> My basic attempt was to try this:
> 
> No prereset or postreset handlers of any kind.

Yes.

> mv_hardreset():  as in the hardreset rework patch (yet to be picked up
> by Jeff), plus a call to mv_select_pmp(link->ap, SATA_PMP_CTRL_PORT)
> before the sata_link_hardreset() wrapper loop.

Yes.

> mv_softreset():
> First do mv_select_pmp(link->ap, SATA_PMP_CTRL_PORT),
> and then call ata_sff_softreset().
> 
> mv_pmp_softreset():
> First do mv_select_pmp(link->ap, link->pmp),
> and then call ata_sff_softreset().

You can just use mv_select_pmp(link->ap, sata_srst_pmp(link)) in 
mv_softreset() and use it for both mv_softreset() and mv_pmp_softreset().

> Resets of the pmp ports always fail with SRST failed (errno=-16).

Hmm... That's -EBUSY.  I'll continue on the reply of the other mail.

-- 
tejun

Re: sata_mv & pmp support

[Tejun Heo]

Hello, again.

Mark Lord wrote:
> Mmm... here's what the same port multiplier does when connected to
> a sata_sil24 card I have here.  This does seem to be an awful lot
> of kernel log spamming for hotplugging a 4-drive port-multiplier.  ??

Indeed.  Was it like this before the big changes?

> [34326.979383] sata_sil24 0000:0d:00.0: version 1.1
> [34326.979423] PCI: Enabling device 0000:0d:00.0 (0000 -> 0003)
> [34326.979433] ACPI: PCI Interrupt 0000:0d:00.0[A] -> GSI 19 (level, 
> low) -> IRQ 19
> [34326.979506] PCI: Setting latency timer of device 0000:0d:00.0 to 64
> [34326.979699] scsi2 : sata_sil24
> [34326.980312] scsi3 : sata_sil24
> [34326.980375] ata3: SATA max UDMA/100 host m128@0xefa04000 port 
> 0xefa00000 irq 19
> [34326.980381] ata4: SATA max UDMA/100 host m128@0xefa04000 port 
> 0xefa02000 irq 19
> [33875.296258] ata3: SATA link down (SStatus 0 SControl 0)
> [33877.348890] ata4: SATA link down (SStatus 0 SControl 0)
> [33891.301792] ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xa 
> frozen
> [33891.301792] ata3: irq_stat 0x00b40090, PHY RDY changed
> [33891.301792] ata3: hard resetting link
> [33893.472639] ata3: SATA link up 3.0 Gbps (SStatus 123 SControl 0)
> [33893.473083] ata3.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, 
> feat 0x1/0x9

Okay, SIMG 3726.

> [33893.473581] ata3.00: hard resetting link
> [33893.896674] ata3.00: SATA link up 1.5 Gbps (SStatus 113 SControl 320)
> [33893.896737] ata3.01: hard resetting link
> [33894.334126] ata3.01: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> [33894.334126] ata3.02: hard resetting link
> [33894.753536] ata3.02: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> [33894.753604] ata3.03: hard resetting link
> [33895.190559] ata3.03: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> [33895.190623] ata3.04: hard resetting link
> [33895.627372] ata3.04: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> [33895.627383] ata3.05: hard resetting link
> [33895.945716] ata3.05: SATA link up 1.5 Gbps (SStatus 113 SControl 320)
> [33895.946640] ata3.00: ATA-7: ST3400832AS, 3.03, max UDMA/133
> [33895.946646] ata3.00: 781422768 sectors, multi 0: LBA48 NCQ (depth 31/32)
> [33895.947720] ata3.00: configured for UDMA/100
> [33895.948865] ata3.01: ATA-7: ST3400832AS, 3.03, max UDMA/133
> [33895.948871] ata3.01: 781422768 sectors, multi 0: LBA48 NCQ (depth 31/32)
> [33895.949940] ata3.01: configured for UDMA/100
> [33895.950873] ata3.02: ATA-7: ST3400832AS, 3.03, max UDMA/133
> [33895.950878] ata3.02: 781422768 sectors, multi 0: LBA48 NCQ (depth 31/32)
> [33895.952042] ata3.02: configured for UDMA/100
> [33895.952973] ata3.03: ATA-7: ST3400832AS, 3.03, max UDMA/133
> [33895.952980] ata3.03: 781422768 sectors, multi 0: LBA48 NCQ (depth 31/32)
> [33895.954049] ata3.03: configured for UDMA/100
> [33895.955011] ata3.04: ATA-7: ST3400832AS, 3.03, max UDMA/133
> [33895.955017] ata3.04: 781422768 sectors, multi 0: LBA48 NCQ (depth 31/32)
> [33895.955913] ata3.04: configured for UDMA/100
> [33895.955927] ata3: EH complete
> [33895.955927] scsi 2:0:0:0: Direct-Access     ATA      ST3400832AS      
> 3.03 PQ: 0 ANSI: 5
> [33895.955927] sd 2:0:0:0: [sdb] 781422768 512-byte hardware sectors 
> (400088 MB)
> [33895.955927] sd 2:0:0:0: [sdb] Write Protect is off
> [33895.955927] sd 2:0:0:0: [sdb] Mode Sense: 00 3a 00 00
> [33895.955927] sd 2:0:0:0: [sdb] Write cache: enabled, read cache: 
> enabled, doesn't support DPO or FUA
> [33895.955927] sd 2:0:0:0: [sdb] 781422768 512-byte hardware sectors 
> (400088 MB)
> [33895.955927] sd 2:0:0:0: [sdb] Write Protect is off
> [33895.955927] sd 2:0:0:0: [sdb] Mode Sense: 00 3a 00 00
> [33895.955927] sd 2:0:0:0: [sdb] Write cache: enabled, read cache: 
> enabled, doesn't support DPO or FUA
> [33895.955927]  sdb: unknown partition table
> [33895.971367] sd 2:0:0:0: [sdb] Attached SCSI disk
> [33895.971443] sd 2:0:0:0: Attached scsi generic sg2 type 0
> [33895.971617] scsi 2:1:0:0: Direct-Access     ATA      ST3400832AS      
> 3.03 PQ: 0 ANSI: 5
> [33895.971762] sd 2:1:0:0: [sdc] 781422768 512-byte hardware sectors 
> (400088 MB)
> [33895.971789] sd 2:1:0:0: [sdc] Write Protect is off
> [33895.971795] sd 2:1:0:0: [sdc] Mode Sense: 00 3a 00 00
> [33895.971842] sd 2:1:0:0: [sdc] Write cache: enabled, read cache: 
> enabled, doesn't support DPO or FUA
> [33895.971933] sd 2:1:0:0: [sdc] 781422768 512-byte hardware sectors 
> (400088 MB)
> [33895.971960] sd 2:1:0:0: [sdc] Write Protect is off
> [33895.971965] sd 2:1:0:0: [sdc] Mode Sense: 00 3a 00 00
> [33895.972011] sd 2:1:0:0: [sdc] Write cache: enabled, read cache: 
> enabled, doesn't support DPO or FUA

Everything seems dandy till this point.

> [33895.972016]  sdc:<3>ata3.01: failed to read log page 10h (errno=-5)
> [33896.280329] ata3.01: exception Emask 0x1 SAct 0x1 SErr 0x0 action 0x0
> [33896.280340] ata3.01: irq_stat 0x00060002, device error via D2H FIS
> [33896.280351] ata3.01: cmd 60/08:00:00:00:00/00:00:00:00:00/40 tag 0 
> ncq 4096 in
> [33896.280353]          res 50/00:00:00:00:00/00:00:00:00:00/00 Emask 
> 0x1 (device error)
> [33896.280358] ata3.01: status: { DRDY }
> [33896.282456] ata3.01: configured for UDMA/100

 From this point, all the errors are device errors from ata3.01.  What 
happens if you move the drive to different port.

Can you also post the log from sata_mv reset failures?

-- 
tejun

Re: sata_mv & pmp support

[Mark Lord]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> Well, I've spent much of the day porting to the new interfaces,
>> and then trying to get things working again.
..
>>
>> Do you have any recommendations on exactly what should be in
>> mv_pmp_softreset() and mv_softreset() and mv_hardreset() under this 
>> scheme?
>>
>> My basic attempt was to try this:
>>
>> No prereset or postreset handlers of any kind.
> 
> Yes.
> 
>> mv_hardreset():  as in the hardreset rework patch (yet to be picked up
>> by Jeff), plus a call to mv_select_pmp(link->ap, SATA_PMP_CTRL_PORT)
>> before the sata_link_hardreset() wrapper loop.
> 
> Yes.
> 
>> mv_softreset():
>> First do mv_select_pmp(link->ap, SATA_PMP_CTRL_PORT),
>> and then call ata_sff_softreset().
>>
>> mv_pmp_softreset():
>> First do mv_select_pmp(link->ap, link->pmp),
>> and then call ata_sff_softreset().
> 
> You can just use mv_select_pmp(link->ap, sata_srst_pmp(link)) in 
> mv_softreset() and use it for both mv_softreset() and mv_pmp_softreset().
..

?? But the ports are different, aren't they?
Or is link->


> 
>> Resets of the pmp ports always fail with SRST failed (errno=-16).
> 
> Hmm... That's -EBUSY.  I'll continue on the reply of the other mail.
> 

Re: sata_mv & pmp support

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
..
>> You can just use mv_select_pmp(link->ap, sata_srst_pmp(link)) in 
>> mv_softreset() and use it for both mv_softreset() and mv_pmp_softreset().
> ..
> 
> ?? But the ports are different, aren't they?
> Or is link->
..

Ignore that post.. hit "send" instead of "cancel" by mistake.  :)

Re: sata_mv & pmp support

[Mark Lord]

Tejun Heo wrote:
> Hello, again.
> 
> Mark Lord wrote:
>> Mmm... here's what the same port multiplier does when connected to
>> a sata_sil24 card I have here.  This does seem to be an awful lot
>> of kernel log spamming for hotplugging a 4-drive port-multiplier.  ??
> 
> Indeed.  Was it like this before the big changes?
> 
>> [34326.979383] sata_sil24 0000:0d:00.0: version 1.1
>> [34326.979423] PCI: Enabling device 0000:0d:00.0 (0000 -> 0003)
>> [34326.979433] ACPI: PCI Interrupt 0000:0d:00.0[A] -> GSI 19 (level, 
>> low) -> IRQ 19
>> [34326.979506] PCI: Setting latency timer of device 0000:0d:00.0 to 64
>> [34326.979699] scsi2 : sata_sil24
>> [34326.980312] scsi3 : sata_sil24
>> [34326.980375] ata3: SATA max UDMA/100 host m128@0xefa04000 port 
>> 0xefa00000 irq 19
>> [34326.980381] ata4: SATA max UDMA/100 host m128@0xefa04000 port 
>> 0xefa02000 irq 19
>> [33875.296258] ata3: SATA link down (SStatus 0 SControl 0)
>> [33877.348890] ata4: SATA link down (SStatus 0 SControl 0)
>> [33891.301792] ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xa 
>> frozen
>> [33891.301792] ata3: irq_stat 0x00b40090, PHY RDY changed
>> [33891.301792] ata3: hard resetting link
>> [33893.472639] ata3: SATA link up 3.0 Gbps (SStatus 123 SControl 0)
>> [33893.473083] ata3.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 
>> ports, feat 0x1/0x9
> 
> Okay, SIMG 3726.
> 
>> [33893.473581] ata3.00: hard resetting link
>> [33893.896674] ata3.00: SATA link up 1.5 Gbps (SStatus 113 SControl 320)
>> [33893.896737] ata3.01: hard resetting link
>> [33894.334126] ata3.01: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>> [33894.334126] ata3.02: hard resetting link
>> [33894.753536] ata3.02: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>> [33894.753604] ata3.03: hard resetting link
>> [33895.190559] ata3.03: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>> [33895.190623] ata3.04: hard resetting link
>> [33895.627372] ata3.04: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>> [33895.627383] ata3.05: hard resetting link
>> [33895.945716] ata3.05: SATA link up 1.5 Gbps (SStatus 113 SControl 320)
>> [33895.946640] ata3.00: ATA-7: ST3400832AS, 3.03, max UDMA/133
>> [33895.946646] ata3.00: 781422768 sectors, multi 0: LBA48 NCQ (depth 
>> 31/32)
>> [33895.947720] ata3.00: configured for UDMA/100
..

Okay, there's the problem, or at least the symptoms.
With sata_mv, I see "soft resetting link" messages there (above),
rather than the desired "hard resetting link".

I wonder why that is?

And another oddity:  on initial module load, sata_mv never finds the PM,
until I rmmod and then insmod again.  Always on the second try.

It's almost as if the hard/soft reset functions were reversed (?).

Gahd.. I wish I could just post the source and have you point out my silliness therein (!)
(Marvell's lawyers forbid such until somebody there clears it).


> Can you also post the log from sata_mv reset failures?

Sure, here it is, with a bunch of extraneous debug messages tossed in.
I've temporarily cloned (no changes) a few libata functions from the softreset
paths, to make it easier to add debug messages inside them, as per below:

insmod sata_mv
[   56.917762] sata_mv 0000:02:00.0: version 1.20
[   56.918205] ACPI: PCI Interrupt 0000:02:00.0[A] -> GSI 19 (level, low) -> IRQ 19
[   56.918256] sata_mv 0000:02:00.0: Applying 60X1C0 workarounds to unknown rev
[   56.918311] sata_mv 0000:02:00.0: Gen-IIE 32 slots 4 ports SCSI mode IRQ via INTx
[   56.918322] PCI: Setting latency timer of device 0000:02:00.0 to 64
[   56.918439] ata4294967295: mv_eh_freeze()
[   56.918557] ata4294967295: mv_eh_freeze()
[   56.918683] ata4294967295: mv_eh_freeze()
[   56.918797] ata4294967295: mv_eh_freeze()
[   56.918884] scsi40 : sata_mv
[   56.919086] scsi41 : sata_mv
[   56.919273] scsi42 : sata_mv
[   56.919461] scsi43 : sata_mv
[   56.919588] ata41: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff422000 irq 19
[   56.919594] ata42: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff424000 irq 19
[   56.919599] ata43: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff426000 irq 19
[   56.919605] ata44: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff428000 irq 19
[   43.304007] ata41: mv_eh_freeze()
[   43.304007] ata41: mv_hardreset
[   43.304007] ata41: mv_pmp_select(15)
[   43.364017] ata41: mv_hardreset rc=0, online=0
[   43.364031] ata41: SATA link down (SStatus 0 SControl 300)
[   43.364038] ata41: mv_eh_thaw()
[   43.364067] ata42: mv_eh_freeze()
[   43.364075] ata42: mv_hardreset
[   43.364518] ata42: mv_pmp_select(15)
[   43.414013] ata42: mv_hardreset rc=0, online=0
[   43.414025] ata42: SATA link down (SStatus 0 SControl 300)
[   43.414030] ata42: mv_eh_thaw()
[   43.414053] ata43: mv_eh_freeze()
[   43.414060] ata43: mv_hardreset
[   43.414503] ata43: mv_pmp_select(15)
[   43.467344] ata43: mv_hardreset rc=0, online=0
[   43.467357] ata43: SATA link down (SStatus 0 SControl 300)
[   43.467362] ata43: mv_eh_thaw()
[   43.467391] ata44: mv_eh_freeze()
[   43.467399] ata44: mv_hardreset
[   43.467843] ata44: mv_pmp_select(15)
[   43.517340] ata44: mv_hardreset rc=0, online=0
[   43.517351] ata44: SATA link down (SStatus 0 SControl 300)
[   43.517356] ata44: mv_eh_thaw()
[   88.369413] ata41: mv_eh_freeze()
[   88.370539] ata42: mv_eh_freeze()
[   88.371568] ata43: mv_eh_freeze()
[   88.373255] ata44: mv_eh_freeze()
[   88.375395] ACPI: PCI interrupt for device 0000:02:00.0 disabled
rmmod sata_mv
insmod sata_mv
[   93.729740] sata_mv 0000:02:00.0: version 1.20
[   93.731147] ACPI: PCI Interrupt 0000:02:00.0[A] -> GSI 19 (level, low) -> IRQ 19
[   93.731621] sata_mv 0000:02:00.0: Applying 60X1C0 workarounds to unknown rev
[   93.732052] sata_mv 0000:02:00.0: Gen-IIE 32 slots 4 ports SCSI mode IRQ via INTx
[   93.732802] PCI: Setting latency timer of device 0000:02:00.0 to 64
[   93.733267] ata4294967295: mv_eh_freeze()
[   93.733632] ata4294967295: mv_eh_freeze()
[   93.734004] ata4294967295: mv_eh_freeze()
[   93.734370] ata4294967295: mv_eh_freeze()
[   93.734706] scsi44 : sata_mv
[   93.735728] scsi45 : sata_mv
[   43.613987] scsi46 : sata_mv
[   43.613987] scsi47 : sata_mv
[   43.613987] ata45: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff422000 irq 19
[   43.613987] ata46: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff424000 irq 19
[   43.613987] ata47: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff426000 irq 19
[   43.613987] ata48: SATA max UDMA/133 mmio m1048576@0xff400000 port 0xff428000 irq 19
[   43.613999] ata45: mv_eh_freeze()
[   43.614005] ata45: mv_hardreset
[   43.614449] ata45: mv_pmp_select(15)
[   43.667331] ata45: mv_hardreset rc=0, online=0
[   43.667343] ata45: SATA link down (SStatus 0 SControl 300)
[   43.667349] ata45: mv_eh_thaw()
[   43.667376] ata46: mv_eh_freeze()
[   43.667382] ata46: mv_hardreset
[   43.667825] ata46: mv_pmp_select(15)
[   43.713994] ata46: mv_hardreset rc=0, online=0
[   43.714005] ata46: SATA link down (SStatus 0 SControl 300)
[   43.714010] ata46: mv_eh_thaw()
[   43.714036] ata47: mv_eh_freeze()
[   43.714041] ata47: mv_hardreset
[   43.714484] ata47: mv_pmp_select(15)
[   43.760658] ata47: mv_hardreset rc=0, online=0
[   43.760668] ata47: SATA link down (SStatus 0 SControl 300)
[   43.760673] ata47: mv_eh_thaw()
[   43.760698] ata48: mv_eh_freeze()
[   43.760704] ata48: mv_hardreset
[   43.761147] ata48: mv_pmp_select(15)
[   43.807322] ata48: mv_hardreset rc=-11, online=1
[   43.807328] ata48: mv_softreset
[   43.807331] ata48: local_ata_sff_softreset
[   43.807335] ata48: local_ata_bus_softreset: doing SRST on port 15
[   43.807381] ata48: local_ata_sff_wait_after_reset
[   43.813984] ata48: local_ata_wait_ready
[   43.820674] ata48: classes[0]=5
[   43.820682] ata48: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[   43.820689] ata48: mv_eh_thaw()
[   43.820695] ata48: sata_pmp_read(0)
[   43.820700] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.820804] ata48: sata_pmp_read(1)
[   43.820808] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.820909] ata48: sata_pmp_read(2)
[   43.820913] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.821013] ata48: sata_pmp_read(32)
[   43.821017] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.821117] ata48: sata_pmp_read(33)
[   43.821121] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.821221] ata48: sata_pmp_read(64)
[   43.821225] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.821325] ata48: sata_pmp_read(96)
[   43.821329] ata48: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.821430] ata48: sata_pmp_write(33,0x00010000)
[   43.821434] ata48: qc_issue(PIO, ata_op=0xe8, pmp=15)
[   43.824030] ata48.15: Port Multiplier 1.1, 0x1095:0x3726 r23, 6 ports, feat 0x1/0x9
[   43.824034] ata48.15: Asynchronous notification not supported, hotplug won't
[   43.824035]          work on fan-out ports. Use warm-plug instead.
[   43.824456] ata48.00: sata_pmp_read(2)
[   43.824461] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.824561] ata48.01: sata_pmp_read(2)
[   43.824566] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.824664] ata48.02: sata_pmp_read(2)
[   43.824669] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.824767] ata48.03: sata_pmp_read(2)
[   43.824772] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.824870] ata48.04: sata_pmp_read(2)
[   43.824874] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.824972] ata48.05: sata_pmp_read(2)
[   43.824976] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.825075] ata48.15: sata_pmp_read(0)
[   43.825079] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   43.825185] ata48.00: soft resetting link
[   43.825187] ata48.00: mv_softreset
[   43.825190] ata48: mv_pmp_select(0)
[   43.825194] ata48.00: local_ata_sff_softreset
[   43.825198] ata48: local_ata_bus_softreset: doing SRST on port 0
[   43.825245] ata48.15: local_ata_sff_wait_after_reset
[   43.863979] ata48.15: local_ata_wait_ready
[   44.823925] ata48.15: local_ata_wait_ready: link is slow to respond, please be patient (ready=0)
[   45.757200] ata48.00: SRST failed (errno=-16)
[   45.757253] ata48.00: sata_pmp_read(0)
[   45.757257] ata48: mv_pmp_select(15)
[   45.757261] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   45.757362] ata48.00: soft resetting link
[   45.757365] ata48.00: mv_softreset
[   45.757368] ata48: mv_pmp_select(0)
[   45.757372] ata48.00: local_ata_sff_softreset
[   45.757375] ata48: local_ata_bus_softreset: doing SRST on port 0
[   45.757422] ata48.15: local_ata_sff_wait_after_reset
[   45.773855] ata48.15: local_ata_wait_ready
[   46.730468] ata48.15: local_ata_wait_ready: link is slow to respond, please be patient (ready=0)
[   47.653744] ata48.00: SRST failed (errno=-16)
[   47.653794] ata48.00: sata_pmp_read(0)
[   47.653799] ata48: mv_pmp_select(15)
[   47.653802] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   47.653904] ata48.00: soft resetting link
[   47.653906] ata48.00: mv_softreset
[   47.653909] ata48: mv_pmp_select(0)
[   47.653913] ata48.00: local_ata_sff_softreset
[   47.653917] ata48: local_ata_bus_softreset: doing SRST on port 0
[   47.653963] ata48.15: local_ata_sff_wait_after_reset
[   47.670399] ata48.15: local_ata_wait_ready
[   48.620345] ata48.15: local_ata_wait_ready: link is slow to respond, please be patient (ready=0)
[   54.423305] ata48.00: SRST failed (errno=-16)
[   54.423359] ata48.00: sata_pmp_read(0)
[   54.423364] ata48: mv_pmp_select(15)
[   54.423369] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   54.423474] ata48.00: sata_pmp_read(0)
[   54.423479] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   54.423580] ata48.00: limiting SATA link speed to 1.5 Gbps
[   54.423583] ata48.00: soft resetting link
[   54.423585] ata48.00: mv_softreset
[   54.423588] ata48: mv_pmp_select(0)
[   54.423593] ata48.00: local_ata_sff_softreset
[   54.423597] ata48: local_ata_bus_softreset: doing SRST on port 0
[   54.423643] ata48.15: local_ata_sff_wait_after_reset
[   54.443292] ata48.15: local_ata_wait_ready
[   55.363244] ata48.00: SRST failed (errno=-16)
[   55.363293] ata48.00: sata_pmp_read(0)
[   55.363298] ata48: mv_pmp_select(15)
[   55.363301] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.363405] ata48.00: reset failed, giving up
[   55.363454] ata48: mv_eh_freeze()
[   55.363460] ata48.15: hard resetting link
[   55.363464] ata48.15: mv_hardreset
[   55.363908] ata48: mv_pmp_select(15)
[   55.406568] ata48.15: mv_hardreset rc=-11, online=1
[   55.406574] ata48.15: mv_softreset
[   55.406578] ata48.15: local_ata_sff_softreset
[   55.406583] ata48: local_ata_bus_softreset: doing SRST on port 15
[   55.406629] ata48.15: local_ata_sff_wait_after_reset
[   55.416571] ata48.15: local_ata_wait_ready
[   55.426587] ata48.15: classes[0]=5
[   55.426595] ata48.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[   55.426602] ata48: mv_eh_thaw()
[   55.426610] ata48.15: sata_pmp_read(0)
[   55.426615] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.426716] ata48.15: sata_pmp_read(1)
[   55.426721] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.426821] ata48.15: sata_pmp_read(2)
[   55.426825] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.426925] ata48.15: sata_pmp_read(32)
[   55.426929] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.427029] ata48.15: sata_pmp_read(33)
[   55.427033] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.427134] ata48.15: sata_pmp_read(64)
[   55.427138] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.427238] ata48.15: sata_pmp_read(96)
[   55.427242] ata48.15: qc_issue(PIO, ata_op=0xe4, pmp=15)
[   55.427343] ata48.15: sata_pmp_write(33,0x00010000)
[   55.427347] ata48.15: qc_issue(PIO, ata_op=0xe8, pmp=15)
[   55.429940] ata48.01: soft resetting link
[   55.429943] ata48.01: mv_softreset
[   55.429946] ata48: mv_pmp_select(1)
[   55.429949] ata48.01: local_ata_sff_softreset
[   55.429953] ata48: local_ata_bus_softreset: doing SRST on port 1
[   55.429999] ata48.15: local_ata_sff_wait_after_reset
[   55.436561] ata48.15: local_ata_wait_ready

Re: sata_mv & pmp support

[Mark Lord]

Mark Lord wrote:
>
>>> [33893.473581] ata3.00: hard resetting link
>>> [33893.896674] ata3.00: SATA link up 1.5 Gbps (SStatus 113 SControl 320)
>>> [33893.896737] ata3.01: hard resetting link
>>> [33894.334126] ata3.01: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>> [33894.334126] ata3.02: hard resetting link
>>> [33894.753536] ata3.02: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>> [33894.753604] ata3.03: hard resetting link
>>> [33895.190559] ata3.03: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>> [33895.190623] ata3.04: hard resetting link
>>> [33895.627372] ata3.04: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>>> [33895.627383] ata3.05: hard resetting link
>>> [33895.945716] ata3.05: SATA link up 1.5 Gbps (SStatus 113 SControl 320)
>>> [33895.946640] ata3.00: ATA-7: ST3400832AS, 3.03, max UDMA/133
>>> [33895.946646] ata3.00: 781422768 sectors, multi 0: LBA48 NCQ (depth 
>>> 31/32)
>>> [33895.947720] ata3.00: configured for UDMA/100
> ..
> 
> Okay, there's the problem, or at least the symptoms.
> With sata_mv, I see "soft resetting link" messages there (above),
> rather than the desired "hard resetting link".
> 
> I wonder why that is?
> 
> And another oddity:  on initial module load, sata_mv never finds the PM,
> until I rmmod and then insmod again.  Always on the second try.
> 
> It's almost as if the hard/soft reset functions were reversed (?).
..

Heh.  Okay, apparently it now needs a .pmp_hardreset function,
which was never there before.  So I've now added one that does this:

	mv_pmp_select(link->ap, sata_srst_pmp(link));
        return sata_std_hardreset(link, class, deadline);

And it seems to be working now with that function supplied, at least for port-0.
Anything else new that it should have?  I'll scan through libata-pmp.c and see.

Time to clean out the debug cruft and test a few things a bit more.

Cheers

Re: sata_mv & pmp support

[Tejun Heo]

Mark Lord wrote:
> Heh.  Okay, apparently it now needs a .pmp_hardreset function,
> which was never there before.  So I've now added one that does this:

Ah... right.  That's something changed while changing libata EH to favor 
hardreset over softreset.  With the change, resuming link on PMP fan out 
ports became hardreset's responsibility, which BTW is necessary anyway 
for certain PMPs (marvell ones).  So, one way or another, you need 
pmp_hardreset.

>     mv_pmp_select(link->ap, sata_srst_pmp(link));
>        return sata_std_hardreset(link, class, deadline);
> 
> And it seems to be working now with that function supplied, at least for 
> port-0.
> Anything else new that it should have?  I'll scan through libata-pmp.c 
> and see.

The hardreset thing was the only behavior change if I didn't screw up. 
It wasn't supposed to cause any actual behavior change tho as all PMP 
supporting PMPs are supposed to have SCR access on fan-out ports and 
thus the capability to hardreset them.

> Time to clean out the debug cruft and test a few things a bit more.

Great. :-)

-- 
tejun

Re: sata_mv & pmp support

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Heh.  Okay, apparently it now needs a .pmp_hardreset function,
>> which was never there before.  So I've now added one that does this:
> 
> Ah... right.  That's something changed while changing libata EH to favor 
> hardreset over softreset.  With the change, resuming link on PMP fan out 
> ports became hardreset's responsibility, which BTW is necessary anyway 
> for certain PMPs (marvell ones).  So, one way or another, you need 
> pmp_hardreset.
> 
>>     mv_pmp_select(link->ap, sata_srst_pmp(link));
>>        return sata_std_hardreset(link, class, deadline);
>>
>> And it seems to be working now with that function supplied, at least 
>> for port-0.
>> Anything else new that it should have?  I'll scan through libata-pmp.c 
>> and see.
> 
> The hardreset thing was the only behavior change if I didn't screw up. 
> It wasn't supposed to cause any actual behavior change tho as all PMP 
> supporting PMPs are supposed to have SCR access on fan-out ports and 
> thus the capability to hardreset them.
> 
>> Time to clean out the debug cruft and test a few things a bit more.
> 
> Great. :-)
..

Yeah, I'm happy now.  :)

It seems very rock solid, actually, at least on the one PM I have that works.
The Fed-Ex guy is due tomorrow with a fresh Marvell PM for me,
but I might send some stuff out for #upstream in the interim.

Still gotta reboot/retest with a 60x1 chipset first, though.

 

Re: sata_mv & pmp support

[Mark Lord]

Mark Lord wrote:
>
> Yeah, I'm happy now.  :)
> 
> It seems very rock solid, actually, at least on the one PM I have that works.
> The Fed-Ex guy is due tomorrow with a fresh Marvell PM for me,
> but I might send some stuff out for #upstream in the interim.
> 
> Still gotta reboot/retest with a 60x1 chipset first, though.
..

Both command-based and FIS-based switching seem to work just fine.
But there is a problem, maybe you could help with ?

I've just noticed that, if I unplug/replug the host-side SATA cable
on the Sil3726 PM, and *then* "insmod sata_mv", the PM is not found.

If I then simply do "rmmod sata_mv ; insmod sata_mv", then the PM is found.

This happens with the original hardreset code from Jeff,
as well as the updated sata_mv code that I've posted earlier.
So it is not anything I've broken (recently :) ).

Very strange.. I wonder if it also happens with the Marvell PM,
except I don't have one here to test with at the moment.

Ever heard of anything strange like that?

??

Re: sata_mv & pmp support

[Tejun Heo]

Mark Lord wrote:
> Mark Lord wrote:
>>
>> Yeah, I'm happy now.  :)
>>
>> It seems very rock solid, actually, at least on the one PM I have that 
>> works.
>> The Fed-Ex guy is due tomorrow with a fresh Marvell PM for me,
>> but I might send some stuff out for #upstream in the interim.
>>
>> Still gotta reboot/retest with a 60x1 chipset first, though.
> ..
> 
> Both command-based and FIS-based switching seem to work just fine.
> But there is a problem, maybe you could help with ?
> 
> I've just noticed that, if I unplug/replug the host-side SATA cable
> on the Sil3726 PM, and *then* "insmod sata_mv", the PM is not found.
> 
> If I then simply do "rmmod sata_mv ; insmod sata_mv", then the PM is found.
> 
> This happens with the original hardreset code from Jeff,
> as well as the updated sata_mv code that I've posted earlier.
> So it is not anything I've broken (recently :) ).
> 
> Very strange.. I wonder if it also happens with the Marvell PM,
> except I don't have one here to test with at the moment.
> 
> Ever heard of anything strange like that?

IIRC, ICH8 ahci + 4726 fails the initial reset sequence because the PMP 
comes up after the hardreset code times out, which triggers another EH 
iteration which works out fine.  If the problem is caused by the PMP not 
responding fast enough, it should cause a hotplug event afterwards. 
Maybe there's a race condition where hotplug events can be lost?

-- 
tejun

Re: sata_mv & pmp support

[Gwendal Grignou]

At reset time, according to the SATA state machine in SATA spec
[16.3.3.5.1] A PM waits for a FIS from the host to port 0xF to be
fully initialized. If for any reason the PM does not issue PHYRDY fast
enough or other problem the host decide there is no device on the
port, it will not issue a FIS, and the PM will never be seen. A PM
will not send asynchronous notification for itself, only to devices
connected to it [and the host controller must be able to handle it].

Maybe polling SATA port could help?

Gwendal.

On Tue, Apr 15, 2008 at 7:10 PM, Tejun Heo <htejun@gmail.com> wrote:
> Mark Lord wrote:
>
> > Mark Lord wrote:
> >
> > >
> > > Yeah, I'm happy now.  :)
> > >
> > > It seems very rock solid, actually, at least on the one PM I have that
> works.
> > > The Fed-Ex guy is due tomorrow with a fresh Marvell PM for me,
> > > but I might send some stuff out for #upstream in the interim.
> > >
> > > Still gotta reboot/retest with a 60x1 chipset first, though.
> > >
> > ..
> >
> > Both command-based and FIS-based switching seem to work just fine.
> > But there is a problem, maybe you could help with ?
> >
> > I've just noticed that, if I unplug/replug the host-side SATA cable
> > on the Sil3726 PM, and *then* "insmod sata_mv", the PM is not found.
> >
> > If I then simply do "rmmod sata_mv ; insmod sata_mv", then the PM is
> found.
> >
> > This happens with the original hardreset code from Jeff,
> > as well as the updated sata_mv code that I've posted earlier.
> > So it is not anything I've broken (recently :) ).
> >
> > Very strange.. I wonder if it also happens with the Marvell PM,
> > except I don't have one here to test with at the moment.
> >
> > Ever heard of anything strange like that?
> >
>
>  IIRC, ICH8 ahci + 4726 fails the initial reset sequence because the PMP
> comes up after the hardreset code times out, which triggers another EH
> iteration which works out fine.  If the problem is caused by the PMP not
> responding fast enough, it should cause a hotplug event afterwards. Maybe
> there's a race condition where hotplug events can be lost?
>
>  --
>  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: sata_mv & pmp support

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> I've just noticed that, if I unplug/replug the host-side SATA cable
>> on the Sil3726 PM, and *then* "insmod sata_mv", the PM is not found.
>>
>> If I then simply do "rmmod sata_mv ; insmod sata_mv", then the PM is 
>> found.
>>
>> This happens with the original hardreset code from Jeff,
>> as well as the updated sata_mv code that I've posted earlier.
>> So it is not anything I've broken (recently :) ).
>>
>> Very strange.. I wonder if it also happens with the Marvell PM,
>> except I don't have one here to test with at the moment.
>>
>> Ever heard of anything strange like that?
> 
> IIRC, ICH8 ahci + 4726 fails the initial reset sequence because the PMP 
> comes up after the hardreset code times out, which triggers another EH 
> iteration which works out fine.  If the problem is caused by the PMP not 
> responding fast enough, it should cause a hotplug event afterwards. 
> Maybe there's a race condition where hotplug events can be lost?
..

Ah.. perhaps that could be it.  I've temporarily disabled the hotplug
detection in sata_mv, because it is broken at present and interfered
with PMP support during earlier testing.

It is next on the list of things to fix (this week, hopefully) as part
of the IRQ/EH handling overhaul in sata_mv.

But strange that the driver always finds it on a simple rmmod/insmod reload.

??

Re: sata_mv & pmp support

[Mark Lord]

Gwendal Grignou wrote:
> At reset time, according to the SATA state machine in SATA spec
> [16.3.3.5.1] A PM waits for a FIS from the host to port 0xF to be
> fully initialized. If for any reason the PM does not issue PHYRDY fast
> enough or other problem the host decide there is no device on the
> port, it will not issue a FIS, and the PM will never be seen.
..

Yes, nothing special there -- libata does all of that (and more).


> A PM will not send asynchronous notification for itself, only to devices
> connected to it [and the host controller must be able to handle it].
> 
> Maybe polling SATA port could help?
..

That's what Tejun meant by a "hotplug even afterwards".
Except he was referring to the hotplug mechanism in the host side
chipset/driver, which is based on PHYRDY, not on any form of AN from the PM.

So the issue is likely that we simply will need hotplug support
in sata_mv before this specific unplug/replug Sil PM works.

No big deal, then -- IRQ/EH/hotplug patches are next in the pipeline
for sata_mv, later this week if all goes well.

Cheers

Re: sata_mv & pmp support

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> I've just noticed that, if I unplug/replug the host-side SATA cable
>> on the Sil3726 PM, and *then* "insmod sata_mv", the PM is not found.
>>
>> If I then simply do "rmmod sata_mv ; insmod sata_mv", then the PM is 
>> found.
>>
>> This happens with the original hardreset code from Jeff,
>> as well as the updated sata_mv code that I've posted earlier.
>> So it is not anything I've broken (recently :) ).
>>
>> Very strange.. I wonder if it also happens with the Marvell PM,
>> except I don't have one here to test with at the moment.
>>
>> Ever heard of anything strange like that?
> 
> IIRC, ICH8 ahci + 4726 fails the initial reset sequence because the PMP 
> comes up after the hardreset code times out, which triggers another EH 
> iteration which works out fine.  If the problem is caused by the PMP not 
> responding fast enough, it should cause a hotplug event afterwards. 
..

So I ought to be able to insert (for debug purposes only) a msleep(3000)
at an appropriate location in the code, and suddenly see this problem go away?

Not for upstream -- the eventual hotplug support in sata_mv is the real solution.
But just to confirm that this is indeed the issue.

Cheers

Re: sata_mv & pmp support

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
> ..
>>> I've just noticed that, if I unplug/replug the host-side SATA cable
>>> on the Sil3726 PM, and *then* "insmod sata_mv", the PM is not found.
>>>
>>> If I then simply do "rmmod sata_mv ; insmod sata_mv", then the PM is 
>>> found.
>>>
>>> This happens with the original hardreset code from Jeff,
>>> as well as the updated sata_mv code that I've posted earlier.
>>> So it is not anything I've broken (recently :) ).
>>>
>>> Very strange.. I wonder if it also happens with the Marvell PM,
>>> except I don't have one here to test with at the moment.
>>>
>>> Ever heard of anything strange like that?
>>
>> IIRC, ICH8 ahci + 4726 fails the initial reset sequence because the 
>> PMP comes up after the hardreset code times out, which triggers 
>> another EH iteration which works out fine.  If the problem is caused 
>> by the PMP not responding fast enough, it should cause a hotplug event 
>> afterwards. 
> ..
> 
> So I ought to be able to insert (for debug purposes only) a msleep(3000)
> at an appropriate location in the code, and suddenly see this problem go 
> away?
> 
> Not for upstream -- the eventual hotplug support in sata_mv is the real solution.
> But just to confirm that this is indeed the issue.
..

Heh.. darned near exactly 3 seconds does it, alright.

Okay, I won't fuss over this for now -- it works if the PM is plugged in
at boot (because the BIOS fiddles it first), and it works when warmplugged
so long as sata_mv is unloaded and reloaded.

This will get fixed completely with the hotplug support fixes coming later this week.

Cheers

Re: sata_mv & pmp support

[Tejun Heo]

Mark Lord wrote:
>> So I ought to be able to insert (for debug purposes only) a msleep(3000)
>> at an appropriate location in the code, and suddenly see this problem 
>> go away?
>>
>> Not for upstream -- the eventual hotplug support in sata_mv is the 
>> real solution.
>> But just to confirm that this is indeed the issue.
> ..
> 
> Heh.. darned near exactly 3 seconds does it, alright.

Heh... 3 secs sounds right.  Yeah, that was the slowest I've ever seen. 
  Maybe we need to extend reset timings.

> Okay, I won't fuss over this for now -- it works if the PM is plugged in
> at boot (because the BIOS fiddles it first), and it works when warmplugged
> so long as sata_mv is unloaded and reloaded.
> 
> This will get fixed completely with the hotplug support fixes coming 
> later this week.

Great.

-- 
tejun