Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581194 > unrolled thread

[patch 1/2] x86/platform/goldfish: Prevent unconditional loading

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [patch 1/2] x86/platform/goldfish: Prevent unconditional loading Thomas Gleixner <tglx@linutronix.de> - 2017-02-15 11:30 +0100
    Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional  loading Thomas Gleixner <tglx@linutronix.de> - 2017-02-15 12:00 +0100
      Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading Alan Cox <alan@linux.intel.com> - 2017-02-15 13:20 +0100
        Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional  loading Thomas Gleixner <tglx@linutronix.de> - 2017-02-15 16:00 +0100
    Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading Peter Zijlstra <peterz@infradead.org> - 2017-02-15 12:00 +0100

#1581194 — [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

The goldfish platform code registers the platform device unconditionally
which causes havoc in several ways if the goldfish_pdev_bus driver is
enabled:

 - Access to the hardcoded physical memory region, which is either not
   available or contains stuff which is completely unrelated.

 - Prevents that the interrupt of the serial port can be requested

 - In case of a spurious interrupt it goes into a infinite loop in the
   interrupt handler of the pdev_bus driver (which needs to be fixed
   seperately).

Add a 'goldfish' command line option to make the registration opt-in when
the platform is compiled in.

I'm seriously grumpy about this engineering trainwreck, which has seven
SOBs from Intel developers for 50 lines of code. And none of them figured
out that this is broken. Impressive fail!

Fixes: ddd70cf93d78 ("goldfish: platform device for x86")
Reported-by: Gabriel C <nix.or.die@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 Documentation/admin-guide/kernel-parameters.txt |    4 ++++
 arch/x86/platform/goldfish/goldfish.c           |   14 +++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1192,6 +1192,10 @@
 			When zero, profiling data is discarded and associated
 			debugfs files are removed at module unload time.
 
+	goldfish	[X86] Enable the goldfish android emulator platform.
+			Don't use this when you are not running on the
+			android emulator
+
 	gpt		[EFI] Forces disk with valid GPT signature but
 			invalid Protective MBR to be treated as GPT. If the
 			primary GPT is corrupted, it enables the backup/alternate
--- a/arch/x86/platform/goldfish/goldfish.c
+++ b/arch/x86/platform/goldfish/goldfish.c
@@ -42,10 +42,22 @@ static struct resource goldfish_pdev_bus
 	}
 };
 
+static bool goldfish_enable __initdata;
+
+static int __init goldfish_setup(char *str)
+{
+	goldfish_enable = true;
+	return 0;
+}
+__setup("goldfish", goldfish_setup);
+
 static int __init goldfish_init(void)
 {
+	if (!goldfish_enable)
+		return -ENODEV;
+
 	platform_device_register_simple("goldfish_pdev_bus", -1,
-						goldfish_pdev_bus_resources, 2);
+					goldfish_pdev_bus_resources, 2);
 	return 0;
 }
 device_initcall(goldfish_init);

[toc] | [next] | [standalone]

#1581211 — Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

On Wed, 15 Feb 2017, Peter Zijlstra wrote:
> On Wed, Feb 15, 2017 at 11:11:50AM +0100, Thomas Gleixner wrote:
> > The goldfish platform code registers the platform device unconditionally
> > which causes havoc in several ways if the goldfish_pdev_bus driver is
> > enabled:
> > 
> >  - Access to the hardcoded physical memory region, which is either not
> >    available or contains stuff which is completely unrelated.
> > 
> >  - Prevents that the interrupt of the serial port can be requested
> > 
> >  - In case of a spurious interrupt it goes into a infinite loop in the
> >    interrupt handler of the pdev_bus driver (which needs to be fixed
> >    seperately).
> > 
> > Add a 'goldfish' command line option to make the registration opt-in when
> > the platform is compiled in.
> > 
> > I'm seriously grumpy about this engineering trainwreck, which has seven
> > SOBs from Intel developers for 50 lines of code. And none of them figured
> > out that this is broken. Impressive fail!
> > 
> > Fixes: ddd70cf93d78 ("goldfish: platform device for x86")
> > Reported-by: Gabriel C <nix.or.die@gmail.com>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: stable@vger.kernel.org
> 
> Just thinking, could we not simply delete this entire driver and use
> x86-DT support to setup this platform?

That would be the proper solution and that's what ARM probably does already.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1581266

> > > I'm seriously grumpy about this engineering trainwreck, which has
> > > seven
> > > SOBs from Intel developers for 50 lines of code. And none of them
> > > figured
> > > out that this is broken. Impressive fail!

It was discussed at the time, documented at the time. Unfortunately the
people who did the emulator didn't feel the urge to provide a way to
detect the platform was Goldfish.

Historically it also used its own custom device discovery scheme. Given
the limited use of older versions of Goldfish it might well make sense
to remove support for the older emulator versions.

Alan

[toc] | [prev] | [next] | [standalone]

#1581380 — Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

On Wed, 15 Feb 2017, Alan Cox wrote:

> > > > I'm seriously grumpy about this engineering trainwreck, which has
> > > > seven
> > > > SOBs from Intel developers for 50 lines of code. And none of them
> > > > figured
> > > > out that this is broken. Impressive fail!
> 
> It was discussed at the time, documented at the time.

I just have a hard time to find that documentation. It's definitely not in
the kernel source, unless you qualify the help text of CONFIG_GOLDFISH as
such:

     Enable support for the Goldfish virtual platform used primarily
     for Android development. Unless you are building for the Android
     Goldfish emulator say N here.

which does not help for randconfig and other builds and does not prevent
users from enabling it accidentaly. That all wouldn't be as bad if at least
the minimal provisioning of damage prevention would have been done.

> Unfortunately the people who did the emulator didn't feel the urge to
> provide a way to detect the platform was Goldfish.

Sure, and the people shoving it into the kernel didn't feel the urge to
enforce that.

> Historically it also used its own custom device discovery scheme. Given
> the limited use of older versions of Goldfish it might well make sense
> to remove support for the older emulator versions.

I'm all for it.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1581218

On Wed, Feb 15, 2017 at 11:11:50AM +0100, Thomas Gleixner wrote:
> The goldfish platform code registers the platform device unconditionally
> which causes havoc in several ways if the goldfish_pdev_bus driver is
> enabled:
> 
>  - Access to the hardcoded physical memory region, which is either not
>    available or contains stuff which is completely unrelated.
> 
>  - Prevents that the interrupt of the serial port can be requested
> 
>  - In case of a spurious interrupt it goes into a infinite loop in the
>    interrupt handler of the pdev_bus driver (which needs to be fixed
>    seperately).
> 
> Add a 'goldfish' command line option to make the registration opt-in when
> the platform is compiled in.
> 
> I'm seriously grumpy about this engineering trainwreck, which has seven
> SOBs from Intel developers for 50 lines of code. And none of them figured
> out that this is broken. Impressive fail!
> 
> Fixes: ddd70cf93d78 ("goldfish: platform device for x86")
> Reported-by: Gabriel C <nix.or.die@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org

Just thinking, could we not simply delete this entire driver and use
x86-DT support to setup this platform?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web