Patchwork Hang during boot, bisected to commit 96d60303fd

login
register
mail settings
Submitter Tejun Heo
Date June 7, 2010, 9:22 a.m.
Message ID <4C0CBA5D.5000900@kernel.org>
Download mbox | patch
Permalink /patch/54840/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - June 7, 2010, 9:22 a.m.
Hello,

On 06/07/2010 06:34 AM, Robert Hancock wrote:
> I'm thinking that the hardreset and device classification makes more
> sense as a place to do this anyways, since that's the point where we
> really know if there's a device connected or not.. Tejun, any
> thoughts?

Heh, of course it doesn't work.  Device presence can't be reliably
determined by sampling status once like that.  We don't have all the
crap in libata-eh for nothing.  :-)

Marc, can you please try the following patch?

Thanks.
Marc Dionne - June 7, 2010, 12:21 p.m.
On Mon, Jun 7, 2010 at 5:22 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 06/07/2010 06:34 AM, Robert Hancock wrote:
>> I'm thinking that the hardreset and device classification makes more
>> sense as a place to do this anyways, since that's the point where we
>> really know if there's a device connected or not.. Tejun, any
>> thoughts?
>
> Heh, of course it doesn't work.  Device presence can't be reliably
> determined by sampling status once like that.  We don't have all the
> crap in libata-eh for nothing.  :-)
>
> Marc, can you please try the following patch?
>
> Thanks.
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1984a6e..261f86d 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -541,29 +541,11 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
>        return -EINVAL;
>  }
>
> -static int ahci_is_device_present(void __iomem *port_mmio)
> -{
> -       u8 status = readl(port_mmio + PORT_TFDATA) & 0xff;
> -
> -       /* Make sure PxTFD.STS.BSY and PxTFD.STS.DRQ are 0 */
> -       if (status & (ATA_BUSY | ATA_DRQ))
> -               return 0;
> -
> -       /* Make sure PxSSTS.DET is 3h */
> -       status = readl(port_mmio + PORT_SCR_STAT) & 0xf;
> -       if (status != 3)
> -               return 0;
> -       return 1;
> -}
> -
>  void ahci_start_engine(struct ata_port *ap)
>  {
>        void __iomem *port_mmio = ahci_port_base(ap);
>        u32 tmp;
>
> -       if (!ahci_is_device_present(port_mmio))
> -               return;
> -
>        /* start DMA */
>        tmp = readl(port_mmio + PORT_CMD);
>        tmp |= PORT_CMD_START;
> @@ -1892,6 +1874,9 @@ static void ahci_error_handler(struct ata_port *ap)
>        }
>
>        sata_pmp_error_handler(ap);
> +
> +       if (!ata_dev_enabled(ap->link.device))
> +               ahci_stop_engine(ap);
>  }
>
>  static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
>
> --
> tejun

Hi Tejun,

With that patch the system boots normally.  Out of curiosity I also
instrumented ahci_error_handler a bit, and the stop_engine case
triggers once for the 6 unused ports, but doesn't trigger for the 2
ports with devices (SSD and DVD), as expected.

Feel free to add a Tested-by if you submit it.

Thanks,
Marc
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett - June 7, 2010, 1:04 p.m.
On Mon, Jun 07, 2010 at 08:21:42AM -0400, Marc Dionne wrote:

> With that patch the system boots normally.  Out of curiosity I also
> instrumented ahci_error_handler a bit, and the stop_engine case
> triggers once for the 6 unused ports, but doesn't trigger for the 2
> ports with devices (SSD and DVD), as expected.
> 
> Feel free to add a Tested-by if you submit it.

Well, that's what trusting the ahci spec gets me. Thanks for testing 
that, Marc.
Tejun Heo - June 7, 2010, 1:09 p.m.
Hello,

On 06/07/2010 03:04 PM, Matthew Garrett wrote:
> On Mon, Jun 07, 2010 at 08:21:42AM -0400, Marc Dionne wrote:
> 
>> With that patch the system boots normally.  Out of curiosity I also
>> instrumented ahci_error_handler a bit, and the stop_engine case
>> triggers once for the 6 unused ports, but doesn't trigger for the 2
>> ports with devices (SSD and DVD), as expected.
>>
>> Feel free to add a Tested-by if you submit it.
> 
> Well, that's what trusting the ahci spec gets me. Thanks for testing 
> that, Marc.

Heh heh, welcome to ATA.  :-)

Will add Tested-by and send it to Jeff.

Thanks.

Patch

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 1984a6e..261f86d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -541,29 +541,11 @@  static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 	return -EINVAL;
 }

-static int ahci_is_device_present(void __iomem *port_mmio)
-{
-	u8 status = readl(port_mmio + PORT_TFDATA) & 0xff;
-
-	/* Make sure PxTFD.STS.BSY and PxTFD.STS.DRQ are 0 */
-	if (status & (ATA_BUSY | ATA_DRQ))
-		return 0;
-
-	/* Make sure PxSSTS.DET is 3h */
-	status = readl(port_mmio + PORT_SCR_STAT) & 0xf;
-	if (status != 3)
-		return 0;
-	return 1;
-}
-
 void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 tmp;

-	if (!ahci_is_device_present(port_mmio))
-		return;
-
 	/* start DMA */
 	tmp = readl(port_mmio + PORT_CMD);
 	tmp |= PORT_CMD_START;
@@ -1892,6 +1874,9 @@  static void ahci_error_handler(struct ata_port *ap)
 	}

 	sata_pmp_error_handler(ap);
+
+	if (!ata_dev_enabled(ap->link.device))
+		ahci_stop_engine(ap);
 }

 static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)