Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Atsushi Nemoto]

On Tue, 07 Aug 2007 17:27:37 +0100, linux-mips@linux-mips.org wrote:
> Author: Ralf Baechle <ralf@linux-mips.org> Tue Aug 7 17:18:28 2007 +0100
> Commit: 00cc123703425aa362b0af75616134cbad4e0689
> Gitweb: http://www.linux-mips.org/g/linux/00cc1237
> Branch: master

With this commit, ide_default_io_base(0) and ide_default_io_base(1)
always returns non-zero value so some platform ide driver (such as
ide/mips/swarm.c) may be assigned to ide2, instead of ide0.

It seems restoreing the pci_get_class() checking would revert the old
behavior, but I cannot see whole story of SMTC vs. ide_default_io_base().

Why pci_get_class() in ide_default_io_base() cause crash on SMTC?

---
Atsushi Nemoto

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Chris Dearman]

Atsushi Nemoto wrote:
> Why pci_get_class() in ide_default_io_base() cause crash on SMTC?

The bug wasn't really SMTC specific, it was just that it showed up on 
SMTC builds. The failure was caused by the early parsing of the 
idebus=xx argument. The argument parser ended up calling
pci_scan that unconditionally enabled interrupts prematurely.

Ralf says this has now been fixed in head of tree:
> Turns out our dear friends at Intel recently had trouble with some JVC CDROM
> drive and their changes made a proper fix for us fairly easy.
> 
> master: 00cc123703425aa362b0af75616134cbad4e0689
> 2.6.22: 50a32ae87aed46b01c8e0c2e90cd6f06a3800c33
> 
> For older kernels the generic PCI code doesn't have the necessary bits in so
> that'd be somewhat more surgery than I want in lmo.

Chris

-- 
Chris Dearman          7200 Cambridge Research Park     +44 1223 203108
MIPS Technologies (UK) Waterbeach, Cambs CB25 9TL  fax  +44 1223 203181

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Ralf Baechle]

On Mon, Sep 03, 2007 at 04:50:06PM +0100, Chris Dearman wrote:

> Atsushi Nemoto wrote:
> >Why pci_get_class() in ide_default_io_base() cause crash on SMTC?
> 
> The bug wasn't really SMTC specific, it was just that it showed up on 
> SMTC builds. The failure was caused by the early parsing of the 
> idebus=xx argument. The argument parser ended up calling
> pci_scan that unconditionally enabled interrupts prematurely.

Which at the time did also happen for x86 - it just happened to be a
harmless bug there.

  Ralf

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Atsushi Nemoto]

On Tue, 4 Sep 2007 13:50:10 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> > The bug wasn't really SMTC specific, it was just that it showed up on 
> > SMTC builds. The failure was caused by the early parsing of the 
> > idebus=xx argument. The argument parser ended up calling
> > pci_scan that unconditionally enabled interrupts prematurely.
> 
> Which at the time did also happen for x86 - it just happened to be a
> harmless bug there.

Hmm, I see.  Then I think no_pci_device() can be used to avoid such
case, as like as pci_find_subsys().

Anyway the commit 00cc123703425aa362b0af75616134cbad4e0689 may change
ide numbering for swarm or au1xxx (if those platform did not have ISA
brigdes).  How about this?


Subject: No ide_default_io_base() if PCI IDE was not found

Revert b5438582090406e2ccb4169d9b2df7c9939ae42b and add
no_pci_devices() check to avoid crash due to early calling of
pci_get_class().

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
diff --git a/include/asm-mips/mach-generic/ide.h b/include/asm-mips/mach-generic/ide.h
index 2b92857..a771283 100644
--- a/include/asm-mips/mach-generic/ide.h
+++ b/include/asm-mips/mach-generic/ide.h
@@ -29,6 +29,35 @@
 
 #define IDE_ARCH_OBSOLETE_DEFAULTS
 
+static __inline__ int ide_probe_legacy(void)
+{
+#ifdef CONFIG_PCI
+	struct pci_dev *dev;
+	/*
+	 * This can be called on the ide_setup() path, super-early in
+	 * boot.  But the down_read() will enable local interrupts,
+	 * which can cause some machines to crash.  So here we detect
+	 * and flag that situation and bail out early.
+	 */
+	if (no_pci_devices())
+		return 0;
+	dev = pci_get_class(PCI_CLASS_BRIDGE_EISA << 8, NULL);
+	if (dev)
+		goto found;
+	dev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
+	if (dev)
+		goto found;
+	return 0;
+found:
+	pci_dev_put(dev);
+	return 1;
+#elif defined(CONFIG_EISA) || defined(CONFIG_ISA)
+	return 1;
+#else
+	return 0;
+#endif
+}
+
 static __inline__ int ide_default_irq(unsigned long base)
 {
 	switch (base) {
@@ -45,6 +74,8 @@ static __inline__ int ide_default_irq(unsigned long base)
 
 static __inline__ unsigned long ide_default_io_base(int index)
 {
+	if (!ide_probe_legacy())
+		return 0;
 	/*
 	 *      If PCI is present then it is not safe to poke around
 	 *      the other legacy IDE ports. Only 0x1f0 and 0x170 are

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Ralf Baechle]

On Tue, Sep 04, 2007 at 11:02:02PM +0900, Atsushi Nemoto wrote:

> Hmm, I see.  Then I think no_pci_device() can be used to avoid such
> case, as like as pci_find_subsys().
> 
> Anyway the commit 00cc123703425aa362b0af75616134cbad4e0689 may change
> ide numbering for swarm or au1xxx (if those platform did not have ISA
> brigdes).  How about this?
> 
> 
> Subject: No ide_default_io_base() if PCI IDE was not found
> 
> Revert b5438582090406e2ccb4169d9b2df7c9939ae42b and add
> no_pci_devices() check to avoid crash due to early calling of
> pci_get_class().
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

Thanks for you guys discussing it out ;-)   Applied, thanks.

  Ralf

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Maciej W. Rozycki]

On Tue, 7 Aug 2007, linux-mips@linux-mips.org wrote:

> Author: Ralf Baechle <ralf@linux-mips.org> Tue Aug 7 17:18:28 2007 +0100
> Commit: 00cc123703425aa362b0af75616134cbad4e0689
> Gitweb: http://www.linux-mips.org/g/linux/00cc1237
> Branch: master
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> ---
> 
>  include/asm-mips/mach-generic/ide.h |   76 ++++++++++++-----------------------
>  1 files changed, 25 insertions(+), 51 deletions(-)

 This change breaks the SWARM -- depending on the setting of 
CONFIG_IDE_GENERIC, either a bus error happens because of blind probing or 
the onboard IDE interface gets designated as "ide2".  I cannot really see 
a dependency between "idebus=" (which merely sets a variable somewhere in 
drivers/ide/ide.c) and code affected by this change -- what's the reason 
behind it?

 Also the comment is misleading -- there is almost nothing about IDE in 
the PCI spec; certainly nothing that would make I/O ports at 0x1f0 and 
0x170 special.  They may be special for a IDE controller PCI devices, but 
one cannot simply assume such a device is present somewhere on the bus (or 
that something will subtractively decode unclaimed addresses).

  Maciej

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Atsushi Nemoto]

On Mon, 10 Sep 2007 14:27:54 +0100 (BST), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
> >  include/asm-mips/mach-generic/ide.h |   76 ++++++++++++-----------------------
> >  1 files changed, 25 insertions(+), 51 deletions(-)
> 
>  This change breaks the SWARM -- depending on the setting of 
> CONFIG_IDE_GENERIC, either a bus error happens because of blind probing or 
> the onboard IDE interface gets designated as "ide2".  I cannot really see 
> a dependency between "idebus=" (which merely sets a variable somewhere in 
> drivers/ide/ide.c) and code affected by this change -- what's the reason 
> behind it?

http://www.linux-mips.org/archives/linux-mips/2007-09/msg00016.html
would be the answer :)

And how about this patch?  Does this fix the problem on SWARM?
http://www.linux-mips.org/archives/linux-mips/2007-09/msg00036.html

---
Atsushi Nemoto

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Maciej W. Rozycki]

On Tue, 11 Sep 2007, Atsushi Nemoto wrote:

> And how about this patch?  Does this fix the problem on SWARM?
> http://www.linux-mips.org/archives/linux-mips/2007-09/msg00036.html

 Thanks a lot -- it makes things work for my SWARM as expected.  But 
doesn't it break systems where there actually is a PCI-(E)ISA bridge and a 
legacy IDE interface?

  Maciej

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Atsushi Nemoto]

On Tue, 11 Sep 2007 14:40:57 +0100 (BST), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
> > And how about this patch?  Does this fix the problem on SWARM?
> > http://www.linux-mips.org/archives/linux-mips/2007-09/msg00036.html
> 
>  Thanks a lot -- it makes things work for my SWARM as expected.  But 
> doesn't it break systems where there actually is a PCI-(E)ISA bridge and a 
> legacy IDE interface?

Yes, it would breaks such systems, but that is exactly we have been
doing for years, isn't it?

And if such a platform was really exists, mach-specific ide.h could be
used as final workaround.

---
Atsushi Nemoto

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Maciej W. Rozycki]

On Tue, 11 Sep 2007, Atsushi Nemoto wrote:

> >  Thanks a lot -- it makes things work for my SWARM as expected.  But 
> > doesn't it break systems where there actually is a PCI-(E)ISA bridge and a 
> > legacy IDE interface?
> 
> Yes, it would breaks such systems, but that is exactly we have been
> doing for years, isn't it?

 Not quite so.  The test for the PCI-(E)ISA bridge is there so that they 
are handled.  Now I gather the use of no_pci_devices() in 
ide_probe_legacy() effectively disables the test entirely (thus making it 
a candidate for removal).  Or am I missing something?

> And if such a platform was really exists, mach-specific ide.h could be
> used as final workaround.

 I think Malta qualifies -- it has an onboard PCI IDE interface (coupled 
in a single chip with a PCI-ISA bridge) which uses legacy I/O ports.  No 
ISA slots though, if you are looking for a diehard example. ;-)

  Maciej

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Atsushi Nemoto]

On Tue, 11 Sep 2007 15:28:04 +0100 (BST), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
>  Not quite so.  The test for the PCI-(E)ISA bridge is there so that they 
> are handled.  Now I gather the use of no_pci_devices() in 
> ide_probe_legacy() effectively disables the test entirely (thus making it 
> a candidate for removal).  Or am I missing something?

Well, I missed your point...  please elaborate?

---
Atsushi Nemoto

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Maciej W. Rozycki]

On Thu, 13 Sep 2007, Atsushi Nemoto wrote:

> >  Not quite so.  The test for the PCI-(E)ISA bridge is there so that they 
> > are handled.  Now I gather the use of no_pci_devices() in 
> > ide_probe_legacy() effectively disables the test entirely (thus making it 
> > a candidate for removal).  Or am I missing something?
> 
> Well, I missed your point...  please elaborate?

 I gather the problem is ide_probe_legacy() is called too early for PCI to 
have been initialised.  With the old code ide_probe_legacy() called 
pci_get_class(), which in turn triggered PCI initialisation, which enabled 
interrupts prematurely and the failure scenario happened.  To rectify Ralf 
resurrected yet older code that reserved the legacy ports unconditionally.  
You have put the code that calls pci_get_class() back and introduced this 
call to no_pci_devices() beforehand.  Please correct me if I have been 
wrong anywhere here.

 Now because at the point ide_probe_legacy() is called, PCI has not been 
initialised yet, no_pci_devices() returns true and calls to 
pci_get_class() are skipped preventing PCI initialisation from triggering 
at this point.  But the end result is they are not going to be called, 
because if they were, it would mean no_pci_devices() had returned false 
and would have been unnecessary in the first place.

 I hope I have been clearer now.

  Maciej

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Atsushi Nemoto]

On Wed, 12 Sep 2007 16:54:08 +0100 (BST), "Maciej W. Rozycki" <macro@linux-mips.org> wrote:
>  I gather the problem is ide_probe_legacy() is called too early for PCI to 
> have been initialised.  With the old code ide_probe_legacy() called 
> pci_get_class(), which in turn triggered PCI initialisation, which enabled 
> interrupts prematurely and the failure scenario happened.  To rectify Ralf 
> resurrected yet older code that reserved the legacy ports unconditionally.  
> You have put the code that calls pci_get_class() back and introduced this 
> call to no_pci_devices() beforehand.  Please correct me if I have been 
> wrong anywhere here.

Right.  That's exactly what I did.

>  Now because at the point ide_probe_legacy() is called, PCI has not been 
> initialised yet, no_pci_devices() returns true and calls to 
> pci_get_class() are skipped preventing PCI initialisation from triggering 
> at this point.  But the end result is they are not going to be called, 
> because if they were, it would mean no_pci_devices() had returned false 
> and would have been unnecessary in the first place.

The pci_get_class() failure was happened only if ide_probe_legacy() was
called too early.  That can happen if you specified some IDE boot
options, such as "idebus=" option.

So if you do not add any ide boot option, there should be no problem.

If you meant "ide_probe_legacy() has been broken with ide boot options
for long years", I agree.

And my recent patch is not to solve this problem.  Just avoid adding
legacy ide0/ide1 unconditionally in normal usage.

>  I hope I have been clearer now.

Thank you!

---
Atsushi Nemoto

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Maciej W. Rozycki]

On Thu, 13 Sep 2007, Atsushi Nemoto wrote:

> The pci_get_class() failure was happened only if ide_probe_legacy() was
> called too early.  That can happen if you specified some IDE boot
> options, such as "idebus=" option.
> 
> So if you do not add any ide boot option, there should be no problem.
> 
> If you meant "ide_probe_legacy() has been broken with ide boot options
> for long years", I agree.

 You mean the point ide_probe_legacy() is called at during bootstrap 
depends on whether some IDE boot options have been specified or not?  
Well, that is an... interesting approach indeed.

> And my recent patch is not to solve this problem.  Just avoid adding
> legacy ide0/ide1 unconditionally in normal usage.

 Of course and it makes perfect sense then.  Thanks for your effort.

  Maciej

Re: [MIPS] SMTC: Fix crash on bootup with idebus= command line argument.

[Ralf Baechle]

On Wed, Sep 12, 2007 at 04:54:08PM +0100, Maciej W. Rozycki wrote:

>  I gather the problem is ide_probe_legacy() is called too early for PCI to 
> have been initialised.  With the old code ide_probe_legacy() called 
> pci_get_class(), which in turn triggered PCI initialisation, which enabled 
> interrupts prematurely and the failure scenario happened.  To rectify Ralf 
> resurrected yet older code that reserved the legacy ports unconditionally.  
> You have put the code that calls pci_get_class() back and introduced this 
> call to no_pci_devices() beforehand.  Please correct me if I have been 
> wrong anywhere here.

Pci_get_class doesn't trigger PCI initialization and I don't think it
should ...

  Ralf