* "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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox