Patchwork tty/powerpc: fix build break with ehv_bytechan.c on allyesconfig

login
register
mail settings
Submitter Timur Tabi
Date Aug. 25, 2011, 4:20 p.m.
Message ID <1314289245-14946-1-git-send-email-timur@freescale.com>
Download mbox | patch
Permalink /patch/111617/
State Superseded
Headers show

Comments

Timur Tabi - Aug. 25, 2011, 4:20 p.m.
The Kconfig for the ePAPR hypervisor byte channel driver has a "depends on PPC",
which means it would compile on all PowerPC platforms, even though it's
only been tested on Freescale platforms.  Change the Kconfig to depend on
FSL_SOC instead.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/tty/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Greg KH - Aug. 25, 2011, 4:32 p.m.
On Thu, Aug 25, 2011 at 11:20:45AM -0500, Timur Tabi wrote:
> The Kconfig for the ePAPR hypervisor byte channel driver has a "depends on PPC",
> which means it would compile on all PowerPC platforms, even though it's
> only been tested on Freescale platforms.  Change the Kconfig to depend on
> FSL_SOC instead.

tested doesn't mean that it shouldn't still build properly for other
platforms, right?

What is keeping the driver from building on all PPC, or even all arches
today?

greg k-h
Timur Tabi - Aug. 25, 2011, 6:02 p.m.
Greg KH wrote:
> tested doesn't mean that it shouldn't still build properly for other
> platforms, right?

The problem is the dependency on MSR_GS, which is defined only for Book-E
PowerPC chips, not all PowerPC.

So I gave it some more thought, and technically ePAPR extends beyond Book-E, so
it's wrong for the driver to depend on anything specific to Book-E.  I've
removed the code that breaks:

	/* Check if we're running as a guest of a hypervisor */
	if (!(mfmsr() & MSR_GS))
		return;

> What is keeping the driver from building on all PPC, or even all arches
> today?

I've made a few changes, and it builds on all PPC now.  I'll post a new patch.
Greg KH - Aug. 25, 2011, 6:46 p.m.
On Thu, Aug 25, 2011 at 01:02:01PM -0500, Timur Tabi wrote:
> Greg KH wrote:
> > tested doesn't mean that it shouldn't still build properly for other
> > platforms, right?
> 
> The problem is the dependency on MSR_GS, which is defined only for Book-E
> PowerPC chips, not all PowerPC.
> 
> So I gave it some more thought, and technically ePAPR extends beyond Book-E, so
> it's wrong for the driver to depend on anything specific to Book-E.  I've
> removed the code that breaks:
> 
> 	/* Check if we're running as a guest of a hypervisor */
> 	if (!(mfmsr() & MSR_GS))
> 		return;

But don't you really want this type of check at runtime?  What happens
if you load this driver on a machine that is not a guest?  Will things
break?  Shouldn't you still refuse to load somehow?

thanks,

greg k-h
Timur Tabi - Aug. 25, 2011, 6:51 p.m.
Greg KH wrote:
> But don't you really want this type of check at runtime?  What happens
> if you load this driver on a machine that is not a guest?  Will things
> break?  Shouldn't you still refuse to load somehow?

This is in the udbg code, which falls under the category of, "turn this on only
if you know what you're doing."

The udbg code runs very early, before the device tree is available.  There's no
way of knowing at this point whether or not we're running under a hypervisor.
If you turn on udbg support, then it means that you're trying to do some very
specific debugging on a specific platform.

So I'm not removing this code just to fix the build break.  It really should
never have been there in the first place.
Greg KH - Aug. 25, 2011, 7:03 p.m.
On Thu, Aug 25, 2011 at 01:51:20PM -0500, Timur Tabi wrote:
> Greg KH wrote:
> > But don't you really want this type of check at runtime?  What happens
> > if you load this driver on a machine that is not a guest?  Will things
> > break?  Shouldn't you still refuse to load somehow?
> 
> This is in the udbg code, which falls under the category of, "turn this on only
> if you know what you're doing."
> 
> The udbg code runs very early, before the device tree is available.  There's no
> way of knowing at this point whether or not we're running under a hypervisor.
> If you turn on udbg support, then it means that you're trying to do some very
> specific debugging on a specific platform.
> 
> So I'm not removing this code just to fix the build break.  It really should
> never have been there in the first place.

Ok, thanks for the details, I'll queue up the patch in a bit.

greg k-h

Patch

diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index f1ea59b..535af0a 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -353,7 +353,7 @@  config TRACE_SINK
 
 config PPC_EPAPR_HV_BYTECHAN
 	tristate "ePAPR hypervisor byte channel driver"
-	depends on PPC
+	depends on FSL_SOC
 	help
 	  This driver creates /dev entries for each ePAPR hypervisor byte
 	  channel, thereby allowing applications to communicate with byte