[PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Xiangliang Yu]

If device is attached to port multiplier, the detection process
look like this:
	ahci_hardreset(link, class, deadline)
	if (class == ATA_DEV_PMP) {
		sata_pmp_attach(dev)    /* will enable FBS */
		sata_pmp_init_links(ap, nr_ports);
		ata_for_each_link(link, ap, EDGE) {
			sata_std_hardreset(link, class, deadline);
			if (link_is_online)
				ahci_softreset(link, class, deadline);
		}
	}
But, according to chapter 9.3.9 in AHCI spec: Prior to issuing software reset,
software shall clear PxCMD.ST to '0' and then clear PxFBS.EN to '0'.

Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
---
 drivers/ata/libahci.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index acfd0f7..8d024a4 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1267,9 +1267,11 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 {
 	struct ata_port *ap = link->ap;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
 	const char *reason = NULL;
 	unsigned long now, msecs;
 	struct ata_taskfile tf;
+	bool flag = FALSE;
 	int rc;
 
 	DPRINTK("ENTER\n");
@@ -1279,6 +1281,11 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 	if (rc && rc != -EOPNOTSUPP)
 		ata_link_warn(link, "failed to reset engine (errno=%d)\n", rc);
 
+	if (!ata_is_host_link(link) && pp->fbs_enabled) {
+		ahci_disable_fbs(ap);
+		flag = TRUE;
+	}
+
 	ata_tf_init(link->device, &tf);
 
 	/* issue the first D2H Register FIS */
@@ -1319,6 +1326,9 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 	} else
 		*class = ahci_dev_classify(ap);
 
+	if (flag)
+		ahci_enable_fbs(ap);
+
 	DPRINTK("EXIT, class=%u\n", *class);
 	return 0;
 
-- 
1.7.1

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Tejun Heo]

Hello,

On Sat, Sep 28, 2013 at 07:13:36PM +0800, Xiangliang Yu wrote:
> If device is attached to port multiplier, the detection process
> look like this:
> 	ahci_hardreset(link, class, deadline)
> 	if (class == ATA_DEV_PMP) {
> 		sata_pmp_attach(dev)    /* will enable FBS */
> 		sata_pmp_init_links(ap, nr_ports);
> 		ata_for_each_link(link, ap, EDGE) {
> 			sata_std_hardreset(link, class, deadline);
> 			if (link_is_online)
> 				ahci_softreset(link, class, deadline);
> 		}
> 	}
> But, according to chapter 9.3.9 in AHCI spec: Prior to issuing software reset,
> software shall clear PxCMD.ST to '0' and then clear PxFBS.EN to '0'.

How was this tested?  Do you observe any behavior difference?  At this
point, we are not using SRST on PMP ports anyway, so I can't see how
this would make any difference.

Thanks.

-- 
tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[xiangliang yu]

hi, Tejun
i had tested the patch with Marvell 88se9235. And driver can find disk
if FBS disabled, or can't find disk.
Please see chapter 9.3.9 and 9.3.8 of AHCI spec. Thanks.


2013/9/28 Tejun Heo <tj@kernel.org>:
> Hello,
>
> On Sat, Sep 28, 2013 at 07:13:36PM +0800, Xiangliang Yu wrote:
>> If device is attached to port multiplier, the detection process
>> look like this:
>>       ahci_hardreset(link, class, deadline)
>>       if (class == ATA_DEV_PMP) {
>>               sata_pmp_attach(dev)    /* will enable FBS */
>>               sata_pmp_init_links(ap, nr_ports);
>>               ata_for_each_link(link, ap, EDGE) {
>>                       sata_std_hardreset(link, class, deadline);
>>                       if (link_is_online)
>>                               ahci_softreset(link, class, deadline);
>>               }
>>       }
>> But, according to chapter 9.3.9 in AHCI spec: Prior to issuing software reset,
>> software shall clear PxCMD.ST to '0' and then clear PxFBS.EN to '0'.
>
> How was this tested?  Do you observe any behavior difference?  At this
> point, we are not using SRST on PMP ports anyway, so I can't see how
> this would make any difference.
>
> Thanks.
>
> --
> tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Tejun Heo]

Hello,

On Sat, Sep 28, 2013 at 09:04:15PM +0800, xiangliang yu wrote:
> hi, Tejun
> i had tested the patch with Marvell 88se9235. And driver can find disk
> if FBS disabled, or can't find disk.

So it can't find the disk if FBS stays enabled?  Can you please attach
the boot log before & after?

> Please see chapter 9.3.9 and 9.3.8 of AHCI spec. Thanks.

Given how crappy and outdated PMP hardware are in general, I'm very
unwilling to make changes just because the spec says so, so please
stop repeating the spec and provide data.

Thanks.

-- 
tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Sergei Shtylyov]

Hello.

On 28-09-2013 15:13, Xiangliang Yu wrote:

> If device is attached to port multiplier, the detection process
> look like this:
> 	ahci_hardreset(link, class, deadline)
> 	if (class == ATA_DEV_PMP) {
> 		sata_pmp_attach(dev)    /* will enable FBS */
> 		sata_pmp_init_links(ap, nr_ports);
> 		ata_for_each_link(link, ap, EDGE) {
> 			sata_std_hardreset(link, class, deadline);
> 			if (link_is_online)
> 				ahci_softreset(link, class, deadline);
> 		}
> 	}
> But, according to chapter 9.3.9 in AHCI spec: Prior to issuing software reset,
> software shall clear PxCMD.ST to '0' and then clear PxFBS.EN to '0'.

> Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
> ---
>   drivers/ata/libahci.c |   10 ++++++++++
>   1 files changed, 10 insertions(+), 0 deletions(-)

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index acfd0f7..8d024a4 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1267,9 +1267,11 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>   {
>   	struct ata_port *ap = link->ap;
>   	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	struct ahci_port_priv *pp = ap->private_data;
>   	const char *reason = NULL;
>   	unsigned long now, msecs;
>   	struct ata_taskfile tf;
> +	bool flag = FALSE;

    Why not lowercase 'false'? And this is not a good name for a variable (too 
generic), it should better be called 'fbs' or something...

> @@ -1279,6 +1281,11 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
>   	if (rc && rc != -EOPNOTSUPP)
>   		ata_link_warn(link, "failed to reset engine (errno=%d)\n", rc);
>
> +	if (!ata_is_host_link(link) && pp->fbs_enabled) {
> +		ahci_disable_fbs(ap);
> +		flag = TRUE;

    Why not lowercase 'true'?

WBR, Sergei

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[xiangliang yu]

Hi,


> On Sat, Sep 28, 2013 at 09:04:15PM +0800, xiangliang yu wrote:
>> hi, Tejun
>> i had tested the patch with Marvell 88se9235. And driver can find disk
>> if FBS disabled, or can't find disk.
>
> So it can't find the disk if FBS stays enabled?  Can you please attach
> the boot log before & after?

Below is boot log:

ata8: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
ata_eh_revalidate_and_attach: dev->class = 5.
ata8.15: Port Multiplier 1.2, 0x1b4b:0x9715 r160, 5 ports, feat 0x1/0x1f
ahci 0000:03:00.0: FBS is enabled
ata8.00: hard resetting link
ata8.00: SATA link down (SStatus 0 SControl 330)
ata8.01: hard resetting link
ata8.01: SATA link down (SStatus 0 SControl 330)
ata8.02: hard resetting link
ata8.02: SATA link down (SStatus 0 SControl 330)
ata8.03: hard resetting link
ata8.03: SATA link up 6.0 Gbps (SStatus 133 SControl 133)
ata8.04: hard resetting link
ata8.04: failed to resume link (SControl 133)
ata8.04: failed to read SCR 0 (Emask=0x40)
ata8.04: failed to read SCR 0 (Emask=0x40)
ata8.04: failed to read SCR 1 (Emask=0x40)
ata8.04: failed to read SCR 0 (Emask=0x40)
ata_eh_revalidate_and_attach: dev->class = 1.
ata8.03: native sectors (2) is smaller than sectors (976773168)
ata8.03: ATA-8: ST3500413AS, JC4B, max UDMA/133
ata8.03: 976773168 sectors, multi 0: LBA48 NCQ (depth 31/32)
ata8.03: configured for UDMA/133
ata_eh_revalidate_and_attach: dev->class = 1.
ata8.04: failed to IDENTIFY (I/O error, err_mask=0x100)
ata8.15: hard resetting link
ata8.15: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
ata8.15: Port Multiplier vendor mismatch '0x1b4b' != '0x133'
ata8.15: PMP revalidation failed (errno=-19)
ata8.15: hard resetting link
ata8.15: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
ata8.15: Port Multiplier vendor mismatch '0x1b4b' != '0x133'
ata8.15: PMP revalidation failed (errno=-19)
ata8.15: limiting SATA link speed to 3.0 Gbps
ata8.15: hard resetting link
ata8.15: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
ata8.15: Port Multiplier vendor mismatch '0x1b4b' != '0x133'
ata8.15: PMP revalidation failed (errno=-19)
ata8.15: failed to recover PMP after 5 tries, giving up
ata8.15: Port Multiplier detaching
ata8.03: disabled
ata8.00: disabled
ata8: EH complete

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Tejun Heo]

Hello,

On Sat, Oct 12, 2013 at 02:56:34PM +0800, xiangliang yu wrote:
> > So it can't find the disk if FBS stays enabled?  Can you please attach
> > the boot log before & after?
> 
> Below is boot log:

And it works after the patch, right?  Can you please update the patch
description so that it includes what was failing before and how it was
tested?

Thanks.

-- 
tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[xiangliang yu]

Hi,

> On Sat, Oct 12, 2013 at 02:56:34PM +0800, xiangliang yu wrote:
>> > So it can't find the disk if FBS stays enabled?  Can you please attach
>> > the boot log before & after?
>>
>> Below is boot log:
>
> And it works after the patch, right?  Can you please update the patch
> description so that it includes what was failing before and how it was
> tested?
>
Yes. I'll update the patch later.

Thanks.

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[xiangliang yu]

Hi,

> On Sat, Oct 12, 2013 at 02:56:34PM +0800, xiangliang yu wrote:
>> > So it can't find the disk if FBS stays enabled?  Can you please attach
>> > the boot log before & after?
>>
>> Below is boot log:
>
> And it works after the patch, right?  Can you please update the patch
> description so that it includes what was failing before and how it was
> tested?

How about this update:

Subject: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

Tested with Marvell 88se9125, attached with one port mulitplier(5 ports)
and one disk, we will get following boot log messages if using current
code:

  ata8: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
  ata8.15: Port Multiplier 1.2, 0x1b4b:0x9715 r160, 5 ports, feat 0x1/0x1f
  ahci 0000:03:00.0: FBS is enabled
  ata8.00: hard resetting link
  ata8.00: SATA link down (SStatus 0 SControl 330)
  ata8.01: hard resetting link
  ata8.01: SATA link down (SStatus 0 SControl 330)
  ata8.02: hard resetting link
  ata8.02: SATA link down (SStatus 0 SControl 330)
  ata8.03: hard resetting link
  ata8.03: SATA link up 6.0 Gbps (SStatus 133 SControl 133)
  ata8.04: hard resetting link
  ata8.04: failed to resume link (SControl 133)
  ata8.04: failed to read SCR 0 (Emask=0x40)
  ata8.04: failed to read SCR 0 (Emask=0x40)
  ata8.04: failed to read SCR 1 (Emask=0x40)
  ata8.04: failed to read SCR 0 (Emask=0x40)
  ata8.03: native sectors (2) is smaller than sectors (976773168)
  ata8.03: ATA-8: ST3500413AS, JC4B, max UDMA/133
  ata8.03: 976773168 sectors, multi 0: LBA48 NCQ (depth 31/32)
  ata8.03: configured for UDMA/133
  ata8.04: failed to IDENTIFY (I/O error, err_mask=0x100)
  ata8.15: hard resetting link
  ata8.15: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
  ata8.15: Port Multiplier vendor mismatch '0x1b4b' != '0x133'
  ata8.15: PMP revalidation failed (errno=-19)
  ata8.15: hard resetting link
  ata8.15: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
  ata8.15: Port Multiplier vendor mismatch '0x1b4b' != '0x133'
  ata8.15: PMP revalidation failed (errno=-19)
  ata8.15: limiting SATA link speed to 3.0 Gbps
  ata8.15: hard resetting link
  ata8.15: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
  ata8.15: Port Multiplier vendor mismatch '0x1b4b' != '0x133'
  ata8.15: PMP revalidation failed (errno=-19)
  ata8.15: failed to recover PMP after 5 tries, giving up
  ata8.15: Port Multiplier detaching
  ata8.03: disabled
  ata8.00: disabled
  ata8: EH complete

The reason is that current detection code doesn't follow AHCI spec:

First,the port multiplier detection process look like this:

ahci_hardreset(link, class, deadline)
if (class == ATA_DEV_PMP) {
sata_pmp_attach(dev) /* will enable FBS */
sata_pmp_init_links(ap, nr_ports);
ata_for_each_link(link, ap, EDGE) {
sata_std_hardreset(link, class, deadline);
if (link_is_online) /* do soft reset */
ahci_softreset(link, class, deadline);
}
}
But, according to chapter 9.3.9 in AHCI spec: Prior to issuing software
reset, software shall clear PxCMD.ST to '0' and then clear PxFBS.EN to
'0'.

The patch test ok with kernel 3.11.1.

Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
---
 drivers/ata/libahci.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 34c8216..c2a29bf 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1266,9 +1266,11 @@ int ahci_do_softreset(struct ata_link *link,
unsigned int *class,
 {
  struct ata_port *ap = link->ap;
  struct ahci_host_priv *hpriv = ap->host->private_data;
+ struct ahci_port_priv *pp = ap->private_data;
  const char *reason = NULL;
  unsigned long now, msecs;
  struct ata_taskfile tf;
+ bool fbs_flag = false;
  int rc;

  DPRINTK("ENTER\n");
@@ -1278,6 +1280,11 @@ int ahci_do_softreset(struct ata_link *link,
unsigned int *class,
  if (rc && rc != -EOPNOTSUPP)
  ata_link_warn(link, "failed to reset engine (errno=%d)\n", rc);

+ if (!ata_is_host_link(link) && pp->fbs_enabled) {
+ ahci_disable_fbs(ap);
+ fbs_flag = true;
+ }
+
  ata_tf_init(link->device, &tf);

  /* issue the first D2H Register FIS */
@@ -1318,6 +1325,9 @@ int ahci_do_softreset(struct ata_link *link,
unsigned int *class,
  } else
  *class = ahci_dev_classify(ap);

+ if (fbs_flag)
+ ahci_enable_fbs(ap);
+
  DPRINTK("EXIT, class=%u\n", *class);
  return 0;

-- 
1.7.1


Thanks!

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Tejun Heo]

Hello,

On Tue, Oct 15, 2013 at 11:40:27AM +0800, xiangliang yu wrote:
> @@ -1278,6 +1280,11 @@ int ahci_do_softreset(struct ata_link *link,
> unsigned int *class,
>   if (rc && rc != -EOPNOTSUPP)
>   ata_link_warn(link, "failed to reset engine (errno=%d)\n", rc);
> 

Please add a comment here.

> + if (!ata_is_host_link(link) && pp->fbs_enabled) {
> + ahci_disable_fbs(ap);
> + fbs_flag = true;
> + }

White space damaged?

> +
>   ata_tf_init(link->device, &tf);
> 
>   /* issue the first D2H Register FIS */
> @@ -1318,6 +1325,9 @@ int ahci_do_softreset(struct ata_link *link,
> unsigned int *class,
>   } else
>   *class = ahci_dev_classify(ap);
> 
> + if (fbs_flag)
> + ahci_enable_fbs(ap);
> +

Thanks.

-- 
tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Tejun Heo]

On Thu, Nov 14, 2013 at 08:26:54AM +0800, xiangliang yu wrote:
> hi
> how about this patch? do you have any update? thanks!

Hmm?  I already applied the patch manually as I wrote before.

Thanks.

-- 
tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[xiangliang yu]

hi
how about this patch? do you have any update? thanks!


> On Wed, Oct 16, 2013 at 11:36:20AM2013/10/27 Tejun Heo <tj@kernel.org>:
> Hello,
> +0800, xiangliang yu wrote:
>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>> index 34c8216..fbe592f 100644
>> --- a/drivers/ata/libahci.c
>> +++ b/drivers/ata/libahci.c
>> @@ -1266,9 +1266,11 @@ int ahci_do_softreset(struct ata_link *link,
>> unsigned int *class,
>
> Your patch is still completely white space corrupted.  Please check
> your mail settings.  Using git-send-email is usually a good idea.  I'm
> applying the patch manually this time but *please* make sure your mail
> setup is working before posting things next time.
>
> Thanks.
>
> --
> tejun

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[xiangliang yu]

Hi,

> Your patch is still completely white space corrupted.  Please check
> your mail settings.  Using git-send-email is usually a good idea.  I'm
> applying the patch manually this time but *please* make sure your mail
> setup is working before posting things next time.

Sorry, my gmail maybe has setting issue. I'll use git to send mail
next time, thank very much.

Re: [PATCH 1/1] AHCI: disabled FBS prior to issuing software reset

[Tejun Heo]

Hello,

On Wed, Oct 16, 2013 at 11:36:20AM +0800, xiangliang yu wrote:
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 34c8216..fbe592f 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1266,9 +1266,11 @@ int ahci_do_softreset(struct ata_link *link,
> unsigned int *class,

Your patch is still completely white space corrupted.  Please check
your mail settings.  Using git-send-email is usually a good idea.  I'm
applying the patch manually this time but *please* make sure your mail
setup is working before posting things next time.

Thanks.

-- 
tejun