libata: kill dead code paths in reset path

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.

---

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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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;

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.

---
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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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;

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

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

Hello, Mark.

Heh... sorry about the trouble.

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

Yes.

Yes.

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().

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

tejun

..

..

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

..

..

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

Hello, again.

Indeed. Was it like this before the big changes?

Okay, SIMG 3726.

Everything seems dandy till this point.

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

..

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

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

..

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

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.

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.

Great. :-)

tejun

..

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

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.

..

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

..

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

..

..

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.

??

..

..

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

..

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

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

Great.

tejun