Unhappy ahci controller on Dell XPS 13 9350

Unhappy ahci controller on Dell XPS 13 9350

[Andy Lutomirski]

Hi-

My XPS 13 9350 laptop has an nvme drive.  It has on-chip ahci
capability, but the M.2 port isn't using it due to the aforementioned
nvme drive using the same signal pins.

It is very unhappy:

[    0.230455] libata version 3.00 loaded.
[    1.039360] ahci 0000:00:17.0: version 3.0
[    1.039386] ahci 0000:00:17.0: forcing PORTS_IMPL to 0xff
[    1.539388] ahci 0000:00:17.0: failed to stop engine (-5)
[    2.039392] ahci 0000:00:17.0: failed to stop engine (-5)
[    2.539402] ahci 0000:00:17.0: failed to stop engine (-5)
[    2.539437] ahci 0000:00:17.0: AHCI 0001.0301 32 slots 8 ports 6
Gbps 0xff impl SATA mode
[    2.539440] ahci 0000:00:17.0: flags: 64bit ncq pm led clo only pio
slum part deso sadm sds apst
[    2.540855] scsi host0: ahci
[    2.541455] scsi host1: ahci
[    2.541972] scsi host2: ahci
[    2.542504] scsi host3: ahci
[    2.543027] scsi host4: ahci
[    2.543550] scsi host5: ahci
[    2.544035] scsi host6: ahci
[    2.544565] scsi host7: ahci
[    2.544641] ata1: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333100 irq 122
[    2.544645] ata2: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333180 irq 122
[    2.544648] ata3: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333200 irq 122
[    2.544652] ata4: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333280 irq 122
[    2.544656] ata5: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333300 irq 122
[    2.544659] ata6: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333380 irq 122
[    2.544662] ata7: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333400 irq 122
[    2.544666] ata8: SATA max UDMA/133 abar m2048@0xdc333000 port
0xdc333480 irq 122
[    3.552450] ata8: failed to resume link (SControl 0)
[    3.552471] ata8: SATA link down (SStatus 0 SControl 0)
[    3.552491] ata7: failed to resume link (SControl 0)
[    3.552508] ata7: SATA link down (SStatus 0 SControl 0)
[    3.552528] ata6: failed to resume link (SControl 0)
[    3.552548] ata6: SATA link down (SStatus 0 SControl 0)
[    3.552567] ata5: failed to resume link (SControl 0)
[    3.552585] ata5: SATA link down (SStatus 0 SControl 0)
[    3.552602] ata4: failed to resume link (SControl 0)
[    3.552616] ata4: SATA link down (SStatus 0 SControl 0)
[    4.051472] ata2: failed to resume link (SControl FFFFFFFF)
[    4.051493] ata2: SATA link down (SStatus FFFFFFFF SControl FFFFFFFF)
[    4.052349] ata1: failed to resume link (SControl FFFFFFFF)
[    4.052370] ata1: SATA link down (SStatus FFFFFFFF SControl FFFFFFFF)
[    4.052394] ata3: failed to resume link (SControl FFFFFFFF)
[    4.052414] ata3: SATA link down (SStatus FFFFFFFF SControl FFFFFFFF)
[    4.556598] Write protecting the kernel read-only data: 14336k
[   12.410784] EXT4-fs (dm-1): mounted filesystem with ordered data
mode. Opts: (null)
[   13.471239] EXT4-fs (nvme0n1p2): mounted filesystem with ordered
data mode. Opts: (null)

It has Alpine Ridge, but I don't think there's such thing as SATA over
USB Type C.  I'd guess that that what's going on is that the ahci
controller genuinely has zero ports enabled and the workaround for
port_map == 0 is firing incorrectly.

This is Linux 4.4.

--Andy

Re: Unhappy ahci controller on Dell XPS 13 9350

[Tejun Heo]

Hello, Andy.

On Tue, Jan 12, 2016 at 10:51:15PM -0800, Andy Lutomirski wrote:
> Hi-
> 
> My XPS 13 9350 laptop has an nvme drive.  It has on-chip ahci
> capability, but the M.2 port isn't using it due to the aforementioned
> nvme drive using the same signal pins.
> 
> It is very unhappy:
> 
> [    0.230455] libata version 3.00 loaded.
> [    1.039360] ahci 0000:00:17.0: version 3.0
> [    1.039386] ahci 0000:00:17.0: forcing PORTS_IMPL to 0xff
...
> It has Alpine Ridge, but I don't think there's such thing as SATA over
> USB Type C.  I'd guess that that what's going on is that the ahci
> controller genuinely has zero ports enabled and the workaround for
> port_map == 0 is firing incorrectly.

Yeah, that's a workaround for very early ahci controllers.  I'll think
of a way to disable it for newer controllers.  Filtering on ahci
controller version should work.

Thanks.

-- 
tejun

[PATCH] libata: disable forced PORTS_IMPL for >= v3.0 controllers

[Tejun Heo]

Some early controllers incorrectly reported zero ports in PORTS_IMPL
register and the ahci driver fabricates PORTS_IMPL from the number of
ports in those cases.  This hasn't mattered but with the new nvme
controllers there are cases where zero PORTS_IMPL is valid and should
be honored.

Disable the workaround for controllers >= v3.0.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Andy Lutomirski <luto@amacapital.net>
Link: http://lkml.kernel.org/g/CALCETrU7yMvXEDhjAUShoHEhDwifJGapdw--BKxsP0jmjKGmRw@mail.gmail.com
---
Hello, Andy.

Can you please test whether this fixes the issue?

Thanks.

 drivers/ata/libahci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index d61740e..4931075 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -496,8 +496,8 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
 		}
 	}
 
-	/* fabricate port_map from cap.nr_ports */
-	if (!port_map) {
+	/* fabricate port_map from cap.nr_ports for older controllers */
+	if (!port_map && (vers >> 16) <= 2) {
 		port_map = (1 << ahci_nr_ports(cap)) - 1;
 		dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
 

Re: [PATCH] libata: disable forced PORTS_IMPL for >= v3.0 controllers

[Andy Lutomirski]

On Thu, Jan 14, 2016 at 2:37 PM, Tejun Heo <tj@kernel.org> wrote:
> Some early controllers incorrectly reported zero ports in PORTS_IMPL
> register and the ahci driver fabricates PORTS_IMPL from the number of
> ports in those cases.  This hasn't mattered but with the new nvme
> controllers there are cases where zero PORTS_IMPL is valid and should
> be honored.
>
> Disable the workaround for controllers >= v3.0.

I still see:

[  +0.001350] ahci 0000:00:17.0: version 3.0
[  +0.000034] ahci 0000:00:17.0: forcing PORTS_IMPL to 0xff

My "vers" is 0x10301.

--Andy

Re: [PATCH] libata: disable forced PORTS_IMPL for >= v3.0 controllers

[Tejun Heo]

Hello,

On Thu, Jan 14, 2016 at 05:00:57PM -0800, Andy Lutomirski wrote:
> [  +0.001350] ahci 0000:00:17.0: version 3.0
> [  +0.000034] ahci 0000:00:17.0: forcing PORTS_IMPL to 0xff
> 
> My "vers" is 0x10301.

Ah, right, of course.  I was looking at the driver version, which I'm
not even sure why we have.  Will update the patch.

Thanks.

-- 
tejun

[PATCH v2] libata: disable forced PORTS_IMPL for >= AHCI 1.3

[Tejun Heo]

Some early controllers incorrectly reported zero ports in PORTS_IMPL
register and the ahci driver fabricates PORTS_IMPL from the number of
ports in those cases.  This hasn't mattered but with the new nvme
controllers there are cases where zero PORTS_IMPL is valid and should
be honored.

Disable the workaround for >= AHCI 1.3.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Andy Lutomirski <luto@amacapital.net>
Link: http://lkml.kernel.org/g/CALCETrU7yMvXEDhjAUShoHEhDwifJGapdw--BKxsP0jmjKGmRw@mail.gmail.com
---
Hello, Andy.

Can you please see whether this one works?

Thanks.

 drivers/ata/libahci.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index d61740e..a91432a 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -496,8 +496,9 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
 		}
 	}
 
-	/* fabricate port_map from cap.nr_ports */
-	if (!port_map) {
+	/* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
+	if (!port_map && (!(vers >> 16) ||
+			  ((vers >> 16) == 1 && (vers & 0xFFFF) < 0x300))) {
 		port_map = (1 << ahci_nr_ports(cap)) - 1;
 		dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
 

Re: [PATCH v2] libata: disable forced PORTS_IMPL for >= AHCI 1.3

[Andy Lutomirski]

On Fri, Jan 15, 2016 at 12:13 PM, Tejun Heo <tj@kernel.org> wrote:
> Some early controllers incorrectly reported zero ports in PORTS_IMPL
> register and the ahci driver fabricates PORTS_IMPL from the number of
> ports in those cases.  This hasn't mattered but with the new nvme
> controllers there are cases where zero PORTS_IMPL is valid and should
> be honored.
>
> Disable the workaround for >= AHCI 1.3.

Tested-by: Andy Lutomirski <luto@kernel.org>

--Andy

Re: [PATCH v2] libata: disable forced PORTS_IMPL for >= AHCI 1.3

[Sergei Shtylyov]

Hello.

On 1/15/2016 11:13 PM, Tejun Heo wrote:

> Some early controllers incorrectly reported zero ports in PORTS_IMPL
> register and the ahci driver fabricates PORTS_IMPL from the number of
> ports in those cases.  This hasn't mattered but with the new nvme
> controllers there are cases where zero PORTS_IMPL is valid and should
> be honored.
>
> Disable the workaround for >= AHCI 1.3.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Andy Lutomirski <luto@amacapital.net>
> Link: http://lkml.kernel.org/g/CALCETrU7yMvXEDhjAUShoHEhDwifJGapdw--BKxsP0jmjKGmRw@mail.gmail.com
> ---
> Hello, Andy.
>
> Can you please see whether this one works?
>
> Thanks.
>
>   drivers/ata/libahci.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index d61740e..a91432a 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -496,8 +496,9 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>   		}
>   	}
>
> -	/* fabricate port_map from cap.nr_ports */
> -	if (!port_map) {
> +	/* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
> +	if (!port_map && (!(vers >> 16) ||
> +			  ((vers >> 16) == 1 && (vers & 0xFFFF) < 0x300))) {

     Hm, won't just (vers < 0x1300) just work?

MBR, Sergei

Re: [PATCH v2] libata: disable forced PORTS_IMPL for >= AHCI 1.3

[Tejun Heo]

On Sat, Jan 16, 2016 at 01:09:37PM +0300, Sergei Shtylyov wrote:
> >+	/* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
> >+	if (!port_map && (!(vers >> 16) ||
> >+			  ((vers >> 16) == 1 && (vers & 0xFFFF) < 0x300))) {
> 
>     Hm, won't just (vers < 0x1300) just work?

lol, indeed.  Lemme update the patch.

Thanks.

-- 
tejun

Re: [PATCH v2] libata: disable forced PORTS_IMPL for >= AHCI 1.3

[Sergei Shtylyov]

Hello.

On 01/19/2016 08:22 PM, Tejun Heo wrote:

>>> +	/* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
>>> +	if (!port_map && (!(vers >> 16) ||
>>> +			  ((vers >> 16) == 1 && (vers & 0xFFFF) < 0x300))) {
>>
>>      Hm, won't just (vers < 0x1300) just work?
>
> lol, indeed.  Lemme update the patch.

    Oops. 0x10300, of course.

> Thanks.

MBR, Sergei

[PATCH v3] libata: disable forced PORTS_IMPL for >= AHCI 1.3

[Tejun Heo]

Hello,

Applied the following to libata/for-4.5-fixes.

Thanks.

------ 8< ------
>From 566d1827df2ef0cbe921d3d6946ac3007b1a6938 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Fri, 15 Jan 2016 15:13:05 -0500
Subject: [PATCH] libata: disable forced PORTS_IMPL for >= AHCI 1.3

Some early controllers incorrectly reported zero ports in PORTS_IMPL
register and the ahci driver fabricates PORTS_IMPL from the number of
ports in those cases.  This hasn't mattered but with the new nvme
controllers there are cases where zero PORTS_IMPL is valid and should
be honored.

Disable the workaround for >= AHCI 1.3.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Andy Lutomirski <luto@amacapital.net>
Link: http://lkml.kernel.org/g/CALCETrU7yMvXEDhjAUShoHEhDwifJGapdw--BKxsP0jmjKGmRw@mail.gmail.com
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: stable@vger.kernel.org
---
 drivers/ata/libahci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 284a176..4029679 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -496,8 +496,8 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
 		}
 	}
 
-	/* fabricate port_map from cap.nr_ports */
-	if (!port_map) {
+	/* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
+	if (!port_map && vers < 0x10300) {
 		port_map = (1 << ahci_nr_ports(cap)) - 1;
 		dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
 
-- 
2.5.0