Re: AHCI driver preferring nr_ports over port map
>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;
}
}
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
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;
}
}
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
>>> Tejun Heo <[EMAIL PROTECTED]> 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 :00:1f.2: version 2.0 <6>ACPI: PCI Interrupt :00:1f.2[C] -> GSI 20 (level, low) -> IRQ 66 <7>PCI: Setting latency timer of device :00:1f.2 to 64 <6>ahci :00:1f.2: AHCI 0001.0100 32 slots 6 ports 3 Gbps 0x1f impl SATA mode <6>ahci :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, 31250 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, 31250 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: 31250 512-byte hdwr sectors (16 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: 31250 512-byte hdwr sectors (16 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: 31250 512-byte hdwr sectors (16 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: 31250 512-byte hdwr sectors (16 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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
>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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
>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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
>>> Tejun Heo <[EMAIL PROTECTED]> 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: h 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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
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: h 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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
AHCI driver preferring nr_ports over port map
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: h 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 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
