"irq 4: nobody cared" when loading ahci driver on ce4100

"irq 4: nobody cared" when loading ahci driver on ce4100

[Maxime Bizon]

Hi Jeff & all,

I'm using an Intel CE4100 platform (Sodaville), and when I load the ahci
driver I get a short lock, then this message.

On the original ahci driver (back in 2007), the ata port interrupts were
not enabled until irq handler was registred (in ahci_thaw()).

But since commit 1c954a4d9a9e351fa3509533fd8dd5f3821206cd (ahci: clean
up PORT_IRQ_BAD_PMP enabling), it is now done early in
ahci_pmp_attach/ahci_pmp_detach:

ata_host_activate => port_start() callback => ahci_port_resume()

before ahci interrupt handler is even registered.

On my board, port irq stat is 0x00400040 before port mask is changed,
that's why interrupt is triggered immediately.

Any idea on the clean way to fix this ?

Thanks,

-- 
Maxime

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Robert Hancock]

On 03/14/2011 01:08 PM, Maxime Bizon wrote:
>
> Hi Jeff&  all,
>
> I'm using an Intel CE4100 platform (Sodaville), and when I load the ahci
> driver I get a short lock, then this message.
>
> On the original ahci driver (back in 2007), the ata port interrupts were
> not enabled until irq handler was registred (in ahci_thaw()).
>
> But since commit 1c954a4d9a9e351fa3509533fd8dd5f3821206cd (ahci: clean
> up PORT_IRQ_BAD_PMP enabling), it is now done early in
> ahci_pmp_attach/ahci_pmp_detach:
>
> ata_host_activate =>  port_start() callback =>  ahci_port_resume()
>
> before ahci interrupt handler is even registered.
>
> On my board, port irq stat is 0x00400040 before port mask is changed,
> that's why interrupt is triggered immediately.
>
> Any idea on the clean way to fix this ?

Where is ahci_thaw being called? It shouldn't be called before the IRQ 
handler is registered - I think it should only be called from the error 
handler which would be way later..

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Maxime Bizon]

On Mon, 2011-03-14 at 18:59 -0600, Robert Hancock wrote:

> Where is ahci_thaw being called? It shouldn't be called before the IRQ
> handler is registered - I think it should only be called from the
> error 

it is not

ahci_pmp_attach/ahci_pmp_detach are the one setting the irq_mask too
soon

-- 
Maxime

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Tejun Heo]

Hello,

On Tue, Mar 15, 2011 at 05:20:40AM +0100, Maxime Bizon wrote:
> On Mon, 2011-03-14 at 18:59 -0600, Robert Hancock wrote:
> 
> > Where is ahci_thaw being called? It shouldn't be called before the IRQ
> > handler is registered - I think it should only be called from the
> > error 
> 
> it is not
> 
> ahci_pmp_attach/ahci_pmp_detach are the one setting the irq_mask too
> soon

Does the following patch fix the problem?  Thanks.

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 26d4523..6a01e3d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1897,7 +1897,9 @@ static void ahci_pmp_attach(struct ata_port *ap)
 	ahci_enable_fbs(ap);
 
 	pp->intr_mask |= PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 static void ahci_pmp_detach(struct ata_port *ap)
@@ -1913,7 +1915,9 @@ static void ahci_pmp_detach(struct ata_port *ap)
 	writel(cmd, port_mmio + PORT_CMD);
 
 	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 int ahci_port_resume(struct ata_port *ap)


-- 
tejun

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Maxime Bizon]

On Tue, 2011-03-15 at 08:19 +0100, Tejun Heo wrote:

> Hello,
> 
> On Tue, Mar 15, 2011 at 05:20:40AM +0100, Maxime Bizon wrote:
> > On Mon, 2011-03-14 at 18:59 -0600, Robert Hancock wrote:
> > 
> > > Where is ahci_thaw being called? It shouldn't be called before the IRQ
> > > handler is registered - I think it should only be called from the
> > > error 
> > 
> > it is not
> > 
> > ahci_pmp_attach/ahci_pmp_detach are the one setting the irq_mask too
> > soon
> 
> Does the following patch fix the problem?  Thanks.

no it doesn't, but this one does:

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 26d4523..6a01e3d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1897,7 +1897,9 @@ static void ahci_pmp_attach(struct ata_port *ap)
 	ahci_enable_fbs(ap);
 
 	pp->intr_mask |= PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	if (!(ap->pflags & (ATA_PFLAG_FROZEN | ATA_PFLAG_INITIALIZING))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 static void ahci_pmp_detach(struct ata_port *ap)
@@ -1913,7 +1915,9 @@ static void ahci_pmp_detach(struct ata_port *ap)
 	writel(cmd, port_mmio + PORT_CMD);
 
 	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	if (!(ap->pflags & (ATA_PFLAG_FROZEN | ATA_PFLAG_INITIALIZING))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 int ahci_port_resume(struct ata_port *ap)




-- 
Maxime

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Maxime Bizon]

On Tue, 2011-03-15 at 15:48 +0100, Maxime Bizon wrote:

> no it doesn't, but this one does:

sorry it does not compile, missing ')', but actually works

I will send a proper patch if you agree with this solution

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 26d4523..6a01e3d 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1897,7 +1897,9 @@ static void ahci_pmp_attach(struct ata_port *ap)
>  	ahci_enable_fbs(ap);
>  
>  	pp->intr_mask |= PORT_IRQ_BAD_PMP;
> -	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
> +
> +	if (!(ap->pflags & (ATA_PFLAG_FROZEN | ATA_PFLAG_INITIALIZING))
> +		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
>  }
>  
>  static void ahci_pmp_detach(struct ata_port *ap)
> @@ -1913,7 +1915,9 @@ static void ahci_pmp_detach(struct ata_port *ap)
>  	writel(cmd, port_mmio + PORT_CMD);
>  
>  	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
> -	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
> +
> +	if (!(ap->pflags & (ATA_PFLAG_FROZEN | ATA_PFLAG_INITIALIZING))
> +		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
>  }
>  
>  int ahci_port_resume(struct ata_port *ap)
> 
> 
> 
> 


-- 
Maxime

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Tejun Heo]

On Tue, Mar 15, 2011 at 04:03:42PM +0100, Maxime Bizon wrote:
> 
> On Tue, 2011-03-15 at 15:48 +0100, Maxime Bizon wrote:
> 
> > no it doesn't, but this one does:
> 
> sorry it does not compile, missing ')', but actually works
> 
> I will send a proper patch if you agree with this solution

Hmm... I don't really like INITIALIZING being tested there.  It's much
more cumbersome to explain.  Can you please change ata_port_alloc() to
set pflags to INITIALIZING|FROZEN?  And add a comment in ahci briefly
noting why the test is necessary?

Thanks.

-- 
tejun

Re: "irq 4: nobody cared" when loading ahci driver on ce4100

[Maxime Bizon]

On Tue, 2011-03-15 at 16:19 +0100, Tejun Heo wrote:

> Hmm... I don't really like INITIALIZING being tested there.  It's much
> more cumbersome to explain.  Can you please change ata_port_alloc() to
> set pflags to INITIALIZING|FROZEN?  And add a comment in ahci briefly
> noting why the test is necessary?

will do

I'm trying to test the patch on linus tree instead of old vendor kernel,
and I will submit it once it's done.

-- 
Maxime

[PATCH] ahci: don't enable port irq before handler is registered

[Maxime Bizon]

From: Maxime Bizon <mbizon@freebox.fr>

The ahci_pmp_attach() & ahci_pmp_detach() unmask port irq, but they
are also called during port initialization, before ahci host irq
handler is registered. On ce4100 platform, this sometimes triggers
"irq 4: nobody cared" message when loading driver.

Fixed this by not touching the register if the port is in frozen
state, and mark all uninitialized port as frozen.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
---
 drivers/ata/libahci.c     |   15 +++++++++++++--
 drivers/ata/libata-core.c |    2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 26d4523..eb5afdd 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1897,7 +1897,15 @@ static void ahci_pmp_attach(struct ata_port *ap)
 	ahci_enable_fbs(ap);
 
 	pp->intr_mask |= PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/*
+	 * This may be called while we are still initializing the ahci
+	 * host, and interrupt handler is not yet registered at that
+	 * time. So don't mess with irq mask register unless the port
+	 * is actually enabled.
+	 */
+	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 static void ahci_pmp_detach(struct ata_port *ap)
@@ -1913,7 +1921,10 @@ static void ahci_pmp_detach(struct ata_port *ap)
 	writel(cmd, port_mmio + PORT_CMD);
 
 	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/* see comment above in ahci_pmp_attach() */
+	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 int ahci_port_resume(struct ata_port *ap)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d4e52e2..02d5703 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5480,7 +5480,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	if (!ap)
 		return NULL;
 	
-	ap->pflags |= ATA_PFLAG_INITIALIZING;
+	ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN;
 	ap->lock = &host->lock;
 	ap->print_id = -1;
 	ap->host = host;
-- 
1.7.1




-- 
Maxime

Re: [PATCH] ahci: don't enable port irq before handler is registered

[Tejun Heo]

On Tue, Mar 15, 2011 at 10:22:38PM +0100, Maxime Bizon wrote:
> 
> From: Maxime Bizon <mbizon@freebox.fr>
> 
> The ahci_pmp_attach() & ahci_pmp_detach() unmask port irq, but they
> are also called during port initialization, before ahci host irq
> handler is registered. On ce4100 platform, this sometimes triggers
> "irq 4: nobody cared" message when loading driver.
> 
> Fixed this by not touching the register if the port is in frozen
> state, and mark all uninitialized port as frozen.
> 
> Signed-off-by: Maxime Bizon <mbizon@freebox.fr>

 Acked-by: Tejun Heo <tj@kernel.org>

But just one nit.

> -	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
> +
> +	/*
> +	 * This may be called while we are still initializing the ahci
> +	 * host, and interrupt handler is not yet registered at that
> +	 * time. So don't mess with irq mask register unless the port
> +	 * is actually enabled.
> +	 */

Can you augment the comment so that it indicates that hardware IRQ
mask shouldn't be modified while the port is frozen and pp->intr_mask
will be restored when the port is thawed and note that the port starts
frozen?

Thanks.

-- 
tejun

Re: [PATCH] ahci: don't enable port irq before handler is registered

[Tejun Heo]

On Wed, Mar 16, 2011 at 10:04:53AM +0100, Tejun Heo wrote:
>  Acked-by: Tejun Heo <tj@kernel.org>
> 
> But just one nit.

Ooh, please also add

 Cc: stable@kernel.org

Thanks.

-- 
tejun

[PATCH v2] ahci: don't enable port irq before handler is registered

[Maxime Bizon]

From: Maxime Bizon <mbizon@freebox.fr>

The ahci_pmp_attach() & ahci_pmp_detach() unmask port irqs, but they
are also called during port initialization, before ahci host irq
handler is registered. On ce4100 platform, this sometimes triggers
"irq 4: nobody cared" message when loading driver.

Fixed this by not touching the register if the port is in frozen
state, and mark all uninitialized port as frozen.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: stable@kernel.org

---
 drivers/ata/libahci.c     |   17 +++++++++++++++--
 drivers/ata/libata-core.c |    2 +-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 26d4523..8498eb5 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1897,7 +1897,17 @@ static void ahci_pmp_attach(struct ata_port *ap)
 	ahci_enable_fbs(ap);
 
 	pp->intr_mask |= PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/*
+	 * We must not change the port interrupt mask register if the
+	 * port is marked frozen, the value in pp->intr_mask will be
+	 * restored later when the port is thawed.
+	 *
+	 * Note that during initialization, the port is marked as
+	 * frozen since the irq handler is not yet registered.
+	 */
+	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 static void ahci_pmp_detach(struct ata_port *ap)
@@ -1913,7 +1923,10 @@ static void ahci_pmp_detach(struct ata_port *ap)
 	writel(cmd, port_mmio + PORT_CMD);
 
 	pp->intr_mask &= ~PORT_IRQ_BAD_PMP;
-	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/* see comment above in ahci_pmp_attach() */
+	if (!(ap->pflags & ATA_PFLAG_FROZEN))
+		writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 int ahci_port_resume(struct ata_port *ap)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d4e52e2..02d5703 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5480,7 +5480,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	if (!ap)
 		return NULL;
 	
-	ap->pflags |= ATA_PFLAG_INITIALIZING;
+	ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN;
 	ap->lock = &host->lock;
 	ap->print_id = -1;
 	ap->host = host;
-- 
1.7.1




-- 
Maxime