AHCI driver preferring nr_ports over port map

AHCI driver preferring nr_ports over port map

[Jan Beulich]

Jeff,

while I realize that Intel's documentation may not be consistent with
anything more generic (which I don't know where to look for), this
current behavior seems to contradict what Intel documents for ESB2:

"23.3.1.4 PI – Ports Implemented Register (D31:F2)
Address Offset: ABAR + 0Ch–0Fh Attribute: R/WO, RO
Default Value: 00000000h Size: 32 bits

This register indicates which ports are exposed to the Intel®
631xESB/632xESB I/O Controller Hub. It is loaded by platform BIOS. It
indicates which ports that the device supports are available for
software to use. For ports that are not available, software must not
read or write to registers within that port."

Could you shed any extra light on this?

Thanks, Jan

Re: AHCI driver preferring nr_ports over port map

[Tejun Heo]

Jan Beulich wrote:
> Jeff,
> 
> while I realize that Intel's documentation may not be consistent with
> anything more generic (which I don't know where to look for), this
> current behavior seems to contradict what Intel documents for ESB2:
> 
> "23.3.1.4 PI – Ports Implemented Register (D31:F2)
> Address Offset: ABAR + 0Ch–0Fh Attribute: R/WO, RO
> Default Value: 00000000h Size: 32 bits
> 
> This register indicates which ports are exposed to the Intel®
> 631xESB/632xESB I/O Controller Hub. It is loaded by platform BIOS. It
> indicates which ports that the device supports are available for
> software to use. For ports that are not available, software must not
> read or write to registers within that port."

nr_ports is preferred over port_map when they disagree which shouldn't
happen in the first place.  On some earlier ahcis, PI was cleared to
zero and lower nr_port number of ports should be used.  The reason why
nr_ports is preferred over PI comes from similar place.  Hardware/BIOSen
are more likely to get PI wrong than nr_ports, so...

Do you have any real case where the above behavior causes problem?

-- 
tejun

Re: AHCI driver preferring nr_ports over port map

[Jan Beulich]

>>> Tejun Heo <htejun@gmail.com> 02/02/08 9:16 AM >>>
>Jan Beulich wrote:
>> Jeff,
>> 
>> while I realize that Intel's documentation may not be consistent with
>> anything more generic (which I don't know where to look for), this
>> current behavior seems to contradict what Intel documents for ESB2:
>> 
>> "23.3.1.4 PI – Ports Implemented Register (D31:F2)
>> Address Offset: ABAR + 0Ch–0Fh Attribute: R/WO, RO
>> Default Value: 00000000h Size: 32 bits
>> 
>> This register indicates which ports are exposed to the Intel®
>> 631xESB/632xESB I/O Controller Hub. It is loaded by platform BIOS. It
>> indicates which ports that the device supports are available for
>> software to use. For ports that are not available, software must not
>> read or write to registers within that port."
>
>nr_ports is preferred over port_map when they disagree which shouldn't
>happen in the first place.  On some earlier ahcis, PI was cleared to
>zero and lower nr_port number of ports should be used.  The reason why
>nr_ports is preferred over PI comes from similar place.  Hardware/BIOSen
>are more likely to get PI wrong than nr_ports, so...
>
>Do you have any real case where the above behavior causes problem?

It's not strictly a problem (i.e. nothing really mis-behaves), but it made
me wonder why the box I saw this on gets 6 ahci device instances set
up when spec as well as port map say there ought to be only 5. After
looking at the ESB2 spec it seemed that behavior was clearly violating
the spec: "For ports that are not available, software must not read or
write to registers within that port.", which contradicts status being
displayed for (and therefore status being read) from the 6th (not
present) port.

Jan

Re: AHCI driver preferring nr_ports over port map

[Tejun Heo]

Jan Beulich wrote:
>> Do you have any real case where the above behavior causes problem?
> 
> It's not strictly a problem (i.e. nothing really mis-behaves), but it made
> me wonder why the box I saw this on gets 6 ahci device instances set
> up when spec as well as port map say there ought to be only 5. After
> looking at the ESB2 spec it seemed that behavior was clearly violating
> the spec: "For ports that are not available, software must not read or
> write to registers within that port.", which contradicts status being
> displayed for (and therefore status being read) from the 6th (not
> present) port.

Well, two values don't agree with each other and we know for a fact that
vendors sometimes get PI wrong, so we trust n_ports in such cases.  We
can reverse the behavior but that's likely to cause more problems than
it fixes.

-- 
tejun

Re: AHCI driver preferring nr_ports over port map

[Jan Beulich]

>Well, two values don't agree with each other and we know for a fact that
>vendors sometimes get PI wrong, so we trust n_ports in such cases.  We
>can reverse the behavior but that's likely to cause more problems than
>it fixes.

I understand your concern, but I think you also understand mine. So
I'm not really asking for general reversal of the logic, but to perhaps
make it just a little smarter. The (not generally usable according to
what you said earlier) experiment I made was to use the smaller of
the two sets - if either set is empty this of course wouldn't be correct.
But perhaps, if you have a non-empty port map, that should be
preferred over nr_ports? Otherwise, chipset specific knowledge may
need to be applied here (i.e. for ESB2, port map ought to always be
used)...

Jan

Re: AHCI driver preferring nr_ports over port map

[Tejun Heo]

Jan Beulich wrote:
> I understand your concern, but I think you also understand mine. So
> I'm not really asking for general reversal of the logic, but to perhaps
> make it just a little smarter. The (not generally usable according to
> what you said earlier) experiment I made was to use the smaller of
> the two sets - if either set is empty this of course wouldn't be correct.
> But perhaps, if you have a non-empty port map, that should be
> preferred over nr_ports? Otherwise, chipset specific knowledge may
> need to be applied here (i.e. for ESB2, port map ought to always be
> used)...

Yes, we can be more smart if necessary.  I don't know.  The hardware is
clearly violating the spec which requires those two values to agree.
What status values are you seeing?  Hardware vendors usually don't get
n_ports wrong from the start, they probably have forgotten to decrement
it by one when one of the ports is plugged for some reason.  I bet the
silicon for the port is there and reporting offline PHY, right?

-- 
tejun

Re: AHCI driver preferring nr_ports over port map

[Jan Beulich]

>Yes, we can be more smart if necessary.  I don't know.  The hardware is
>clearly violating the spec which requires those two values to agree.

So are you saying the ESB2 spec is violating a higher level spec? I know
almost nothing about AHCI, so please forgive that question...

>What status values are you seeing?  Hardware vendors usually don't get
>n_ports wrong from the start, they probably have forgotten to decrement
>it by one when one of the ports is plugged for some reason.  I bet the
>silicon for the port is there and reporting offline PHY, right?

This is output from our SLE10SP2 kernel, the output is similar for others:

<6>scsi2 : ahci
<6>ata3: SATA link down (SStatus 0 SControl 300)
<6>scsi3 : ahci
<6>ata4: SATA link down (SStatus 0 SControl 300)
<6>scsi4 : ahci
<6>ata5: SATA link down (SStatus 4 SControl 300)
<6>scsi5 : ahci
<6>ata6: SATA link down (SStatus 0 SControl 0)

Even the message relating to ata5 seems a little dubious to me, as it's
not in sync with what the other unused ports say (and also not in sync
with what I see on other boxes - SStatus is always 0 for such ports).

Jan

Re: AHCI driver preferring nr_ports over port map

[Tejun Heo]

Jan Beulich wrote:
>> Yes, we can be more smart if necessary.  I don't know.  The hardware is
>> clearly violating the spec which requires those two values to agree.
> 
> So are you saying the ESB2 spec is violating a higher level spec? I know
> almost nothing about AHCI, so please forgive that question...

n_ports and PI should agree with each other.  That's what the ahci spec
requires.  If ESB2 spec has different opinion about n_ports, it's in
violation of AHCI spec but I don't think it explicitly state such
things, does it?

>> What status values are you seeing?  Hardware vendors usually don't get
>> n_ports wrong from the start, they probably have forgotten to decrement
>> it by one when one of the ports is plugged for some reason.  I bet the
>> silicon for the port is there and reporting offline PHY, right?
> 
> This is output from our SLE10SP2 kernel, the output is similar for others:
> 
> <6>scsi2 : ahci
> <6>ata3: SATA link down (SStatus 0 SControl 300)
> <6>scsi3 : ahci
> <6>ata4: SATA link down (SStatus 0 SControl 300)
> <6>scsi4 : ahci
> <6>ata5: SATA link down (SStatus 4 SControl 300)
> <6>scsi5 : ahci
> <6>ata6: SATA link down (SStatus 0 SControl 0)
>
> Even the message relating to ata5 seems a little dubious to me, as it's
> not in sync with what the other unused ports say (and also not in sync
> with what I see on other boxes - SStatus is always 0 for such ports).

I'd like to see more output including leading controller detection but
yeah, it seems there's no silicon implemented for the last port.  The
SStatus value 4 indicates that PHY is offline which usually happens when
the PHY is turned off from SControl.  Hmm... weird.  How many ports does
the machine actually have?  I agree we'll need to adjust PI handling for
the controller.

Thanks.

-- 
tejun

Re: AHCI driver preferring nr_ports over port map

[Jan Beulich]

>>> Tejun Heo <htejun@gmail.com> 05.02.08 13:21 >>>
>Jan Beulich wrote:
>>> Yes, we can be more smart if necessary.  I don't know.  The hardware is
>>> clearly violating the spec which requires those two values to agree.
>> 
>> So are you saying the ESB2 spec is violating a higher level spec? I know
>> almost nothing about AHCI, so please forgive that question...
>
>n_ports and PI should agree with each other.  That's what the ahci spec
>requires.  If ESB2 spec has different opinion about n_ports, it's in
>violation of AHCI spec but I don't think it explicitly state such
>things, does it?

It does, in the description for bits 4:0 of the host capabilities register:
"Number of Ports (NPS) – RO. Hardwired to 5h to indicate support for 6
ports. Note that the number of ports indicated in this field may be more
than the number of ports indicated in the PI (ABAR + 0Ch) register."

>I'd like to see more output including leading controller detection but
>yeah, it seems there's no silicon implemented for the last port.  The
>SStatus value 4 indicates that PHY is offline which usually happens when
>the PHY is turned off from SControl.  Hmm... weird.  How many ports does
>the machine actually have?  I agree we'll need to adjust PI handling for
>the controller.

<7>libata version 2.00 loaded.
<7>ahci 0000:00:1f.2: version 2.0
<6>ACPI: PCI Interrupt 0000:00:1f.2[C] -> GSI 20 (level, low) -> IRQ 66
<7>PCI: Setting latency timer of device 0000:00:1f.2 to 64
<6>ahci 0000:00:1f.2: AHCI 0001.0100 32 slots 6 ports 3 Gbps 0x1f impl SATA mode
<6>ahci 0000:00:1f.2: flags: 64bit ncq pm led slum part 
<6>ata1: SATA max UDMA/133 cmd 0xF882E100 ctl 0x0 bmdma 0x0 irq 66
<6>ata2: SATA max UDMA/133 cmd 0xF882E180 ctl 0x0 bmdma 0x0 irq 66
<6>ata3: SATA max UDMA/133 cmd 0xF882E200 ctl 0x0 bmdma 0x0 irq 66
<6>ata4: SATA max UDMA/133 cmd 0xF882E280 ctl 0x0 bmdma 0x0 irq 66
<6>ata5: SATA max UDMA/133 cmd 0xF882E300 ctl 0x0 bmdma 0x0 irq 66
<6>ata6: SATA max UDMA/133 cmd 0xF882E380 ctl 0x0 bmdma 0x0 irq 66
<6>scsi0 : ahci
<6>ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
<6>ata1.00: ATA-7, max UDMA/133, 312500000 sectors: LBA48 NCQ (depth 31/32)
<6>ata1.00: configured for UDMA/133
<6>scsi1 : ahci
<6>ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
<6>ata2.00: ATA-7, max UDMA/133, 312500000 sectors: LBA48 NCQ (depth 31/32)
<6>ata2.00: configured for UDMA/133
<6>scsi2 : ahci
<6>ata3: SATA link down (SStatus 0 SControl 300)
<6>scsi3 : ahci
<6>ata4: SATA link down (SStatus 0 SControl 300)
<6>scsi4 : ahci
<6>ata5: SATA link down (SStatus 4 SControl 300)
<6>scsi5 : ahci
<6>ata6: SATA link down (SStatus 0 SControl 0)
<5>  Vendor: ATA       Model: ST3160815AS       Rev: 3.AD
<5>  Type:   Direct-Access                      ANSI SCSI revision: 05
<5>SCSI device sda: 312500000 512-byte hdwr sectors (160000 MB)
<5>sda: Write Protect is off
<7>sda: Mode Sense: 00 3a 00 00
<5>SCSI device sda: drive cache: write back
<5>SCSI device sda: 312500000 512-byte hdwr sectors (160000 MB)
<5>sda: Write Protect is off
<7>sda: Mode Sense: 00 3a 00 00
<5>SCSI device sda: drive cache: write back
<6> sda: sda1 sda2 sda3 sda4 < sda5 sda6 sda7 sda8 sda9 sda10 >
<5>sd 0:0:0:0: Attached scsi disk sda
<5>  Vendor: ATA       Model: ST3160815AS       Rev: 3.AD
<5>  Type:   Direct-Access                      ANSI SCSI revision: 05
<5>sd 0:0:0:0: Attached scsi generic sg0 type 0
<5>SCSI device sdb: 312500000 512-byte hdwr sectors (160000 MB)
<5>sdb: Write Protect is off
<7>sdb: Mode Sense: 00 3a 00 00
<5>SCSI device sdb: drive cache: write back
<5>SCSI device sdb: 312500000 512-byte hdwr sectors (160000 MB)
<5>sdb: Write Protect is off
<7>sdb: Mode Sense: 00 3a 00 00
<5>SCSI device sdb: drive cache: write back
<6> sdb: sdb1
<5>sd 1:0:0:0: Attached scsi disk sdb
<5>sd 1:0:0:0: Attached scsi generic sg1 type 0

Jan

Re: AHCI driver preferring nr_ports over port map

[Tejun Heo]

Jan Beulich wrote:
> It does, in the description for bits 4:0 of the host capabilities register:
> "Number of Ports (NPS)" RO. Hardwired to 5h to indicate support for 6
> ports. Note that the number of ports indicated in this field may be more
> than the number of ports indicated in the PI (ABAR + 0Ch) register."

Oops, you're right, NP can go over PI.  Somehow I've been believing it
should match PI.  Oh well, that's me being delusional again.  :-(

>From ahci 1.1.

 Number of Ports (NP): 0's based value indicating the maximum number
 of ports supported by the HBA silicon. A maximum of 32 ports can be
 supported. A value of Impl.  '0h', indicating one port, is the
 minimum requirement. Note that the number of ports Spec.  indicated
 in this field may be more than the number of ports indicated in the
 GHC.PI register.

Does the following patch fix the problem?

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index e75966b..39627c7 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -679,24 +679,20 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 
 	/* cross check port_map and cap.n_ports */
 	if (port_map) {
-		u32 tmp_port_map = port_map;
-		int n_ports = ahci_nr_ports(cap);
+		int map_ports = 0;
 
-		for (i = 0; i < AHCI_MAX_PORTS && n_ports; i++) {
-			if (tmp_port_map & (1 << i)) {
-				n_ports--;
-				tmp_port_map &= ~(1 << i);
-			}
-		}
+		for (i = 0; i < AHCI_MAX_PORTS; i++)
+			if (port_map & (1 << i))
+				map_ports++;
 
-		/* If n_ports and port_map are inconsistent, whine and
-		 * clear port_map and let it be generated from n_ports.
+		/* If PI has more ports than n_ports, whine and clear
+		 * port_map and let it be generated from n_ports.
 		 */
-		if (n_ports || tmp_port_map) {
+		if (map_ports > ahci_nr_ports(cap)) {
 			dev_printk(KERN_WARNING, &pdev->dev,
-				   "nr_ports (%u) and implemented port map "
-				   "(0x%x) don't match, using nr_ports\n",
-				   ahci_nr_ports(cap), port_map);
+				   "implemented port map (0x%x) contains more "
+				   "ports than nr_ports (%u), using nr_ports\n",
+				   port_map, ahci_nr_ports(cap));
 			port_map = 0;
 		}
 	}

Re: AHCI driver preferring nr_ports over port map

[Jan Beulich]

>Does the following patch fix the problem?

Yes, it does - thanks!

Jan

*********************************

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index e75966b..39627c7 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -679,24 +679,20 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 
 	/* cross check port_map and cap.n_ports */
 	if (port_map) {
-		u32 tmp_port_map = port_map;
-		int n_ports = ahci_nr_ports(cap);
+		int map_ports = 0;
 
-		for (i = 0; i < AHCI_MAX_PORTS && n_ports; i++) {
-			if (tmp_port_map & (1 << i)) {
-				n_ports--;
-				tmp_port_map &= ~(1 << i);
-			}
-		}
+		for (i = 0; i < AHCI_MAX_PORTS; i++)
+			if (port_map & (1 << i))
+				map_ports++;
 
-		/* If n_ports and port_map are inconsistent, whine and
-		 * clear port_map and let it be generated from n_ports.
+		/* If PI has more ports than n_ports, whine and clear
+		 * port_map and let it be generated from n_ports.
 		 */
-		if (n_ports || tmp_port_map) {
+		if (map_ports > ahci_nr_ports(cap)) {
 			dev_printk(KERN_WARNING, &pdev->dev,
-				   "nr_ports (%u) and implemented port map "
-				   "(0x%x) don't match, using nr_ports\n",
-				   ahci_nr_ports(cap), port_map);
+				   "implemented port map (0x%x) contains more "
+				   "ports than nr_ports (%u), using nr_ports\n",
+				   port_map, ahci_nr_ports(cap));
 			port_map = 0;
 		}
 	}