All of lore.kernel.org
 help / color / mirror / Atom feed
* "irq 4: nobody cared" when loading ahci driver on ce4100
@ 2011-03-14 19:08 Maxime Bizon
  2011-03-15  0:59 ` Robert Hancock
  0 siblings, 1 reply; 12+ messages in thread
From: Maxime Bizon @ 2011-03-14 19:08 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Tejun Heo, linux-ide


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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-14 19:08 "irq 4: nobody cared" when loading ahci driver on ce4100 Maxime Bizon
@ 2011-03-15  0:59 ` Robert Hancock
  2011-03-15  4:20   ` Maxime Bizon
  0 siblings, 1 reply; 12+ messages in thread
From: Robert Hancock @ 2011-03-15  0:59 UTC (permalink / raw)
  To: mbizon; +Cc: Jeff Garzik, Tejun Heo, linux-ide

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..

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-15  0:59 ` Robert Hancock
@ 2011-03-15  4:20   ` Maxime Bizon
  2011-03-15  7:19     ` Tejun Heo
  0 siblings, 1 reply; 12+ messages in thread
From: Maxime Bizon @ 2011-03-15  4:20 UTC (permalink / raw)
  To: Robert Hancock; +Cc: Jeff Garzik, Tejun Heo, linux-ide


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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-15  4:20   ` Maxime Bizon
@ 2011-03-15  7:19     ` Tejun Heo
  2011-03-15 14:48       ` Maxime Bizon
  0 siblings, 1 reply; 12+ messages in thread
From: Tejun Heo @ 2011-03-15  7:19 UTC (permalink / raw)
  To: Maxime Bizon; +Cc: Robert Hancock, Jeff Garzik, linux-ide

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-15  7:19     ` Tejun Heo
@ 2011-03-15 14:48       ` Maxime Bizon
  2011-03-15 15:03         ` Maxime Bizon
  0 siblings, 1 reply; 12+ messages in thread
From: Maxime Bizon @ 2011-03-15 14:48 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Robert Hancock, Jeff Garzik, linux-ide


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



^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-15 14:48       ` Maxime Bizon
@ 2011-03-15 15:03         ` Maxime Bizon
  2011-03-15 15:19           ` Tejun Heo
  0 siblings, 1 reply; 12+ messages in thread
From: Maxime Bizon @ 2011-03-15 15:03 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Robert Hancock, Jeff Garzik, linux-ide


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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-15 15:03         ` Maxime Bizon
@ 2011-03-15 15:19           ` Tejun Heo
  2011-03-15 17:03             ` Maxime Bizon
  2011-03-15 21:22             ` [PATCH] ahci: don't enable port irq before handler is registered Maxime Bizon
  0 siblings, 2 replies; 12+ messages in thread
From: Tejun Heo @ 2011-03-15 15:19 UTC (permalink / raw)
  To: Maxime Bizon; +Cc: Robert Hancock, Jeff Garzik, linux-ide

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: "irq 4: nobody cared" when loading ahci driver on ce4100
  2011-03-15 15:19           ` Tejun Heo
@ 2011-03-15 17:03             ` Maxime Bizon
  2011-03-15 21:22             ` [PATCH] ahci: don't enable port irq before handler is registered Maxime Bizon
  1 sibling, 0 replies; 12+ messages in thread
From: Maxime Bizon @ 2011-03-15 17:03 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Robert Hancock, Jeff Garzik, linux-ide


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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] ahci: don't enable port irq before handler is registered
  2011-03-15 15:19           ` Tejun Heo
  2011-03-15 17:03             ` Maxime Bizon
@ 2011-03-15 21:22             ` Maxime Bizon
  2011-03-16  9:04               ` Tejun Heo
  1 sibling, 1 reply; 12+ messages in thread
From: Maxime Bizon @ 2011-03-15 21:22 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Robert Hancock, Jeff Garzik, linux-ide


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



^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH] ahci: don't enable port irq before handler is registered
  2011-03-15 21:22             ` [PATCH] ahci: don't enable port irq before handler is registered Maxime Bizon
@ 2011-03-16  9:04               ` Tejun Heo
  2011-03-16  9:05                 ` Tejun Heo
  2011-03-16 13:58                 ` [PATCH v2] " Maxime Bizon
  0 siblings, 2 replies; 12+ messages in thread
From: Tejun Heo @ 2011-03-16  9:04 UTC (permalink / raw)
  To: Maxime Bizon; +Cc: Robert Hancock, Jeff Garzik, linux-ide

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] ahci: don't enable port irq before handler is registered
  2011-03-16  9:04               ` Tejun Heo
@ 2011-03-16  9:05                 ` Tejun Heo
  2011-03-16 13:58                 ` [PATCH v2] " Maxime Bizon
  1 sibling, 0 replies; 12+ messages in thread
From: Tejun Heo @ 2011-03-16  9:05 UTC (permalink / raw)
  To: Maxime Bizon; +Cc: Robert Hancock, Jeff Garzik, linux-ide

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH v2] ahci: don't enable port irq before handler is registered
  2011-03-16  9:04               ` Tejun Heo
  2011-03-16  9:05                 ` Tejun Heo
@ 2011-03-16 13:58                 ` Maxime Bizon
  1 sibling, 0 replies; 12+ messages in thread
From: Maxime Bizon @ 2011-03-16 13:58 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Robert Hancock, Jeff Garzik, linux-ide, stable

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



^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-03-16 13:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-14 19:08 "irq 4: nobody cared" when loading ahci driver on ce4100 Maxime Bizon
2011-03-15  0:59 ` Robert Hancock
2011-03-15  4:20   ` Maxime Bizon
2011-03-15  7:19     ` Tejun Heo
2011-03-15 14:48       ` Maxime Bizon
2011-03-15 15:03         ` Maxime Bizon
2011-03-15 15:19           ` Tejun Heo
2011-03-15 17:03             ` Maxime Bizon
2011-03-15 21:22             ` [PATCH] ahci: don't enable port irq before handler is registered Maxime Bizon
2011-03-16  9:04               ` Tejun Heo
2011-03-16  9:05                 ` Tejun Heo
2011-03-16 13:58                 ` [PATCH v2] " Maxime Bizon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.