"libata: fix non-uniform ports handling" breaks sata_sis secondary port

"libata: fix non-uniform ports handling" breaks sata_sis secondary port

[Patrick McHardy]

The patch "libata: fix non-uniform ports handling" breaks the
secondary SATA port with the sata_sis driver for me (no link
detected). The reason seems to be that the SIS_FLAG_CFGSCR flag
is only set on probe_ent->port_flags in sis_init_one(), but
ata_port_init() uses ent->pinfo2->flags for initialization of
the flags in struct ata_port for the secondary port.

lspci entry of the controller below, if more information is needed
please ask.


0000:00:05.0 RAID bus controller: Silicon Integrated Systems [SiS] RAID
bus controller 180 SATA/PATA  [SiS] (rev 01) (prog-if 85)
        Subsystem: ASUSTeK Computer Inc.: Unknown device 810e
        Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B-
        Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
<TAbort- <MAbort- >SERR- <PERR-
        Latency: 128
        Interrupt: pin A routed to IRQ 18
        Region 0: I/O ports at eff0 [size=8]
        Region 1: I/O ports at efe4 [size=4]
        Region 2: I/O ports at efa8 [size=8]
        Region 3: I/O ports at efe0 [size=4]
        Region 4: I/O ports at ef90 [size=16]
        Region 5: I/O ports at <unassigned>

[PATCH] sata_sis: fix flags handling for the secondary port

[Tejun Heo]

sis_init_one() modifies probe_ent->port_flags after allocating and
initializing it using ata_pci_init_native_mode().  This makes
port_flags for the secondary port (probe_ent->pinfo2->flags) go out of
sync resulting in misdetection of device due to incorrectly
initialized SCR access flag.

This patch make probe_ent alloc/init happen after the final port flags
value is determined.  This is fragile but probe_ent and all the
related mess are scheduled to go away soon for exactly this reason.
We just need to hold everything together till then.

This has been spotted and diagnosed by Patrick McHardy.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Patric McHardy <kaber@trash.net>
---
Patrick, can you test this patch and post result?

Jeff, please don't apply this patch till Patrick acks it.

Thanks.

--- a/drivers/ata/sata_sis.c
+++ b/drivers/ata/sata_sis.c
@@ -240,7 +240,7 @@ static int sis_init_one (struct pci_dev 
 	struct ata_probe_ent *probe_ent = NULL;
 	int rc;
 	u32 genctl;
-	struct ata_port_info *ppi[2];
+	struct ata_port_info pi = sis_port_info, *ppi[2] = { &pi, &pi };
 	int pci_dev_busy = 0;
 	u8 pmr;
 	u8 port2_start;
@@ -265,27 +265,20 @@ static int sis_init_one (struct pci_dev 
 	if (rc)
 		goto err_out_regions;
 
-	ppi[0] = ppi[1] = &sis_port_info;
-	probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY);
-	if (!probe_ent) {
-		rc = -ENOMEM;
-		goto err_out_regions;
-	}
-
 	/* check and see if the SCRs are in IO space or PCI cfg space */
 	pci_read_config_dword(pdev, SIS_GENCTL, &genctl);
 	if ((genctl & GENCTL_IOMAPPED_SCR) == 0)
-		probe_ent->port_flags |= SIS_FLAG_CFGSCR;
+		pi.flags |= SIS_FLAG_CFGSCR;
 
 	/* if hardware thinks SCRs are in IO space, but there are
 	 * no IO resources assigned, change to PCI cfg space.
 	 */
-	if ((!(probe_ent->port_flags & SIS_FLAG_CFGSCR)) &&
+	if ((!(pi.flags & SIS_FLAG_CFGSCR)) &&
 	    ((pci_resource_start(pdev, SIS_SCR_PCI_BAR) == 0) ||
 	     (pci_resource_len(pdev, SIS_SCR_PCI_BAR) < 128))) {
 		genctl &= ~GENCTL_IOMAPPED_SCR;
 		pci_write_config_dword(pdev, SIS_GENCTL, genctl);
-		probe_ent->port_flags |= SIS_FLAG_CFGSCR;
+		pi.flags |= SIS_FLAG_CFGSCR;
 	}
 
 	pci_read_config_byte(pdev, SIS_PMR, &pmr);
@@ -306,6 +299,12 @@ static int sis_init_one (struct pci_dev 
 		port2_start = 0x20;
 	}
 
+	probe_ent = ata_pci_init_native_mode(pdev, ppi, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY);
+	if (!probe_ent) {
+		rc = -ENOMEM;
+		goto err_out_regions;
+	}
+
 	if (!(probe_ent->port_flags & SIS_FLAG_CFGSCR)) {
 		probe_ent->port[0].scr_addr =
 			pci_resource_start(pdev, SIS_SCR_PCI_BAR);

Re: [PATCH] sata_sis: fix flags handling for the secondary port

[Patrick McHardy]

Tejun Heo wrote:
> sis_init_one() modifies probe_ent->port_flags after allocating and
> initializing it using ata_pci_init_native_mode().  This makes
> port_flags for the secondary port (probe_ent->pinfo2->flags) go out of
> sync resulting in misdetection of device due to incorrectly
> initialized SCR access flag.
> 
> This patch make probe_ent alloc/init happen after the final port flags
> value is determined.  This is fragile but probe_ent and all the
> related mess are scheduled to go away soon for exactly this reason.
> We just need to hold everything together till then.
> 
> This has been spotted and diagnosed by Patrick McHardy.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Patric McHardy <kaber@trash.net>
> ---
> Patrick, can you test this patch and post result?


The patch fixes the problem, both ports are properly detected and work
fine. Thanks Tejun.