Hang during boot, bisected to commit 96d60303fd

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.

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

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.

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.