* [RFC PATCH V1 2/7] cpuidle: replace xen access to x86 pm_idle and default_idle
From: Trinabh Gupta @ 2011-06-07 16:29 UTC (permalink / raw)
To: linux-pm, linuxppc-dev; +Cc: linux-kernel
In-Reply-To: <20110607162847.6848.44707.stgit@tringupt.in.ibm.com>
From: Len Brown <len.brown@intel.com>
When a Xen Dom0 kernel boots on a hypervisor, it gets access
to the raw-hardware ACPI tables. While it parses the idle tables
for the hypervisor's beneift, it uses HLT for its own idle.
Rather than have xen scribble on pm_idle and access default_idle,
have it simply disable_cpuidle() so acpi_idle will not load and
architecture default HLT will be used.
cc: xen-devel@lists.xensource.com
Signed-off-by: Len Brown <len.brown@intel.com>
---
arch/x86/xen/setup.c | 3 ++-
drivers/cpuidle/cpuidle.c | 4 ++++
include/linux/cpuidle.h | 2 ++
3 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index be1a464..ab1a916 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -9,6 +9,7 @@
#include <linux/mm.h>
#include <linux/pm.h>
#include <linux/memblock.h>
+#include <linux/cpuidle.h>
#include <asm/elf.h>
#include <asm/vdso.h>
@@ -424,7 +425,7 @@ void __init xen_arch_setup(void)
#ifdef CONFIG_X86_32
boot_cpu_data.hlt_works_ok = 1;
#endif
- pm_idle = default_idle;
+ disable_cpuidle();
boot_option_idle_override = IDLE_HALT;
fiddle_vdso();
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index a171b9e..8d7303b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -34,6 +34,10 @@ int cpuidle_disabled(void)
{
return off;
}
+void disable_cpuidle(void)
+{
+ off = 1;
+}
#if defined(CONFIG_ARCH_HAS_CPU_IDLE_WAIT)
static void cpuidle_kick_cpus(void)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 1e85538..2786787 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -127,6 +127,7 @@ struct cpuidle_driver {
};
#ifdef CONFIG_CPU_IDLE
+extern void disable_cpuidle(void);
extern int cpuidle_register_driver(struct cpuidle_driver *drv);
struct cpuidle_driver *cpuidle_get_driver(void);
@@ -140,6 +141,7 @@ extern int cpuidle_enable_device(struct cpuidle_device *dev);
extern void cpuidle_disable_device(struct cpuidle_device *dev);
#else
+static inline void disable_cpuidle(void) { }
static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
{return -ENODEV; }
^ permalink raw reply related
* [RFC PATCH V1 1/7] cpuidle: create bootparam "cpuidle.off=1"
From: Trinabh Gupta @ 2011-06-07 16:29 UTC (permalink / raw)
To: linux-pm, linuxppc-dev; +Cc: linux-kernel
In-Reply-To: <20110607162847.6848.44707.stgit@tringupt.in.ibm.com>
From: Len Brown <len.brown@intel.com>
useful for disabling cpuidle to fall back
to architecture-default idle loop
cpuidle drivers and governors will fail to register.
on x86 they'll say so:
intel_idle: intel_idle yielding to (null)
ACPI: acpi_idle yielding to (null)
Signed-off-by: Len Brown <len.brown@intel.com>
---
Documentation/kernel-parameters.txt | 3 +++
drivers/cpuidle/cpuidle.c | 10 ++++++++++
drivers/cpuidle/cpuidle.h | 1 +
drivers/cpuidle/driver.c | 3 +++
drivers/cpuidle/governor.c | 3 +++
5 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index d9a203b..5697faf 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -546,6 +546,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
/proc/<pid>/coredump_filter.
See also Documentation/filesystems/proc.txt.
+ cpuidle.off=1 [CPU_IDLE]
+ disable the cpuidle sub-system
+
cpcihp_generic= [HW,PCI] Generic port I/O CompactPCI driver
Format:
<first_slot>,<last_slot>,<port>,<enum_bit>[,<debug>]
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 406be83..a171b9e 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -28,6 +28,12 @@ LIST_HEAD(cpuidle_detected_devices);
static void (*pm_idle_old)(void);
static int enabled_devices;
+static int off __read_mostly;
+
+int cpuidle_disabled(void)
+{
+ return off;
+}
#if defined(CONFIG_ARCH_HAS_CPU_IDLE_WAIT)
static void cpuidle_kick_cpus(void)
@@ -397,6 +403,9 @@ static int __init cpuidle_init(void)
{
int ret;
+ if (cpuidle_disabled())
+ return -ENODEV;
+
pm_idle_old = pm_idle;
ret = cpuidle_add_class_sysfs(&cpu_sysdev_class);
@@ -408,4 +417,5 @@ static int __init cpuidle_init(void)
return 0;
}
+module_param(off, int, 0444);
core_initcall(cpuidle_init);
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index 33e50d5..38c3fd8 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -13,6 +13,7 @@ extern struct list_head cpuidle_governors;
extern struct list_head cpuidle_detected_devices;
extern struct mutex cpuidle_lock;
extern spinlock_t cpuidle_driver_lock;
+extern int cpuidle_disabled(void);
/* idle loop */
extern void cpuidle_install_idle_handler(void);
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 33e3189..284d7af 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -50,6 +50,9 @@ int cpuidle_register_driver(struct cpuidle_driver *drv)
if (!drv)
return -EINVAL;
+ if (cpuidle_disabled())
+ return -ENODEV;
+
spin_lock(&cpuidle_driver_lock);
if (cpuidle_curr_driver) {
spin_unlock(&cpuidle_driver_lock);
diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index 724c164..ea2f8e7 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -81,6 +81,9 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
if (!gov || !gov->select)
return -EINVAL;
+ if (cpuidle_disabled())
+ return -ENODEV;
+
mutex_lock(&cpuidle_lock);
if (__cpuidle_find_governor(gov->name) == NULL) {
ret = 0;
^ permalink raw reply related
* [RFC PATCH V1 0/7] cpuidle: (POWER) cpuidle driver for pSeries
From: Trinabh Gupta @ 2011-06-07 16:29 UTC (permalink / raw)
To: linux-pm, linuxppc-dev; +Cc: linux-kernel
This patch series ports the cpuidle framework for ppc64 platform and
implements a cpuidle back-end driver for ppc64 (pSeries) platform.
Currently idle states are managed by pseries_{dedicated,shared}_idle_sleep()
routines in arch/powerpc/platforms/pseries/setup.c. There are
generally two idle states (snooze and cede) that are exploited by
these routines based on simple heuristics.
Moving the idle states over to cpuidle framework can take advantage of
the advanced heuristics, tunables, and features provided by cpuidle
framework. Additional idle states like extended cede with hints would be
included and exploited using the cpuidle framework. The statistics and
tracing infrastructure provided by the cpuidle framework also helps in
enabling power management related tools and help tune the system and
applications.
This series aims to maintain compatibility and functionality to
existing pSeries idle cpu management code. There are no new functions
or idle states added as part of this series. The first three patch
in the series are taken from "idle cleanup - v3" posted by
Len Brown (https://lkml.org/lkml/2011/4/2/8). These are required to
eliminate usage of pm_idle.
[1/7] cpuidle: create bootparam "cpuidle.off=1"
[2/7] cpuidle: replace xen access to x86 pm_idle and default_idle
[3/7] cpuidle: stop using pm_idle
[4/7] - Provides arch specific cpu_idle_wait() function required by cpuidle
subsystem.
[5/7] - pseries_idle cpuidle driver
[6/7] - Enables cpuidle for pSeries and directly calls cpuidle_idle_call()
[7/7] - Handles powersave=off kernel boot parameter and disables registration
of pseries_idle cpuidle driver.
This patch series has the following dependencies:
(1) Base kernel tree: 3.0.0-rc2
(2) Patch: cpuidle: global registration patch
https://lkml.org/lkml/2011/6/6/259
This series is tested on ppc64 pSeries POWER7 system with the snooze
and cede states.
Thanks,
-Trinabh
^ permalink raw reply
* Re: [PATCH] Fix doorbell type shift
From: Kumar Gala @ 2011-06-07 12:59 UTC (permalink / raw)
To: Michael Neuling; +Cc: Milton Miller, linuxppc-dev
In-Reply-To: <23877.1307328527@neuling.org>
On Jun 5, 2011, at 9:48 PM, Michael Neuling wrote:
> doorbell type is defined as bits 32:36 so should be shifted by 63-36 =
> 27 rather than 28.
>
> We never noticed this bug as we've only every used type PPC_DBELL = 0.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
> arch/powerpc/include/asm/dbell.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Kumar Gala <galak@kernel.crashing.org>
>
> Index: linux-ozlabs/arch/powerpc/include/asm/dbell.h
> ===================================================================
> --- linux-ozlabs.orig/arch/powerpc/include/asm/dbell.h
> +++ linux-ozlabs/arch/powerpc/include/asm/dbell.h
> @@ -18,7 +18,7 @@
> #include <asm/ppc-opcode.h>
>
> #define PPC_DBELL_MSG_BRDCAST (0x04000000)
> -#define PPC_DBELL_TYPE(x) (((x) & 0xf) << 28)
> +#define PPC_DBELL_TYPE(x) (((x) & 0xf) << (63-36))
> enum ppc_dbell {
> PPC_DBELL = 0, /* doorbell */
> PPC_DBELL_CRIT = 1, /* critical doorbell */
^ permalink raw reply
* [gianfar]bandwidth management problem on mpc8313 based board
From: Vijay Nikam @ 2011-06-07 13:02 UTC (permalink / raw)
To: linuxppc-dev
Dear All,
I have mpc8313 powerpc based board with silicon revision 2.1. the
processor has two ETH ports (eTsec1 and eTsec2) i.e. eth0 and eth1.
eth0 is 1Gbps port and eth1 is 100Mbps port. On board there is L2
switch from TANTOS2G (psb6972) supports one port 1Gbps,
and from switch there are 4 more eth ports derived which are 100Mbps
ports and port based VLAN is configured for this purpose.
The interface between switch and eth0 (port of processor) is RGMII. So
the processor port and switch port are connected on 1Gbps Link.
The other 4 derived ports (100Mbps) are used to connect to external world.
On this board Embedded Linux is running of kernel version 2.6.23 with HRT patch.
The ethernet controller driver in use is "gianfar" version 1.3
The driver is configured properly as it determines both links 1000Mbps
(eth0) and 100Mbps (eth1) also verified with ethtool.
After this I started to perform bandwidth test using iperf tool.
When I performed this test on one port out of 4 derived ports I am
getting bandwidth in the range of 80-85Mbps
but when the same test is performed on 2 ports simultaneously then the
per port bandwidth is reduced to 40-45Mbps.
But my understanding is all of the 4 ports should support 100Mbps
bandwidth simultaneously (as base port is 1Gbps).
Then why bandwidth gets reduced when more than one port are
communicating simultaneously?
Any reason or suggestion I should check for this problem?
Kindly Please acknowledge, thanks
Kind Regards,
Vijay Nikam
^ permalink raw reply
* [git pull] Please pull powerpc.git merge branch (fwd)
From: Kumar Gala @ 2011-06-07 12:55 UTC (permalink / raw)
To: linuxppc-dev
[forgot to CC the list]
---------- Forwarded message ----------
Date: Mon, 6 Jun 2011 16:17:08 -0500 (CDT)
From: Kumar Gala <galak@kernel.crashing.org>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Subject: [git pull] Please pull powerpc.git merge branch
The following changes since commit 9693ebd4815eefa2b7c8fcc699061a0c8da0c1e7:
Merge remote branch 'kumar/merge' into merge (2011-05-27 09:58:22 +1000)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git merge
Kumar Gala (2):
powerpc/fsl_rio: Fix compile error when CONFIG_FSL_RIO not set
powerpc/book3e: Fix CPU feature handling on e5500 in 32-bit mode
Shaohui Xie (1):
powerpc/85xx: fix race bug of calling request_irq after enable elbc interrupts
arch/powerpc/include/asm/rio.h | 2 +-
arch/powerpc/kernel/cputable.c | 2 +-
arch/powerpc/sysdev/fsl_lbc.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-07 7:08 UTC (permalink / raw)
To: linuxppc-dev
Cc: Konrad Rzeszutek Wilk, greg, kumar.gala, linux-kernel,
Chris Metcalf, akpm, Deepak Saxena, linux-console, Timur Tabi
In-Reply-To: <4DED5D08.3070704@tilera.com>
On Tuesday 07 June 2011 01:04:40 Chris Metcalf wrote:
> For context, the most recent patch for the tile driver in question is here:
>
> https://patchwork.kernel.org/patch/843892/
>
> On 6/6/2011 5:23 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jun 06, 2011 at 05:01:36PM -0400, Chris Metcalf wrote:
>
> >> a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
> >> well. If it's intended more for drivers that guests running under a
> >> hypervisor can use to talk to the hypervisor itself (e.g. managing
> > I believe that the code that deals with specific subsystem (so block API
> > for example) would reside in subsystem directory (so drivers/block would have
> > your virtualization block driver). This allows the maintainer of block
> > to make sure your driver is OK.
>
> Sure, makes sense. The new push (as I understand it) is to group primarily
> by function, not by bus or architecture.
Yes.
> >> notifications that a hypervisor delivers to a guest to cause it to shut
> >> down or take other actions), then it doesn't seem like the Tilera
> > That looks to be arch/<x>/tilera/virt/ candidate?
>
> Arnd, among others, has suggested that all drivers live in "drivers"
> somewhere, so "arch/tile" may not be the best place. (To be fair, I
> originally had this driver in arch/tile/drivers/, so your idea is certainly
> reasonable!)
>
> >> paravirtualized device drivers belong there, since they're just using the
> >> Tilera hypervisor synchronously to do I/O or get/set device and driver state.
> > Well, I/O sounds like block API or network API. But then you are also
> > doing management ioctl - which implies "drivers". "drivers/tilera" does not
> > work?
>
> There is certainly precedent for drivers that don't fit cleanly into an
> existing category to go in drivers/<arch>, e.g. drivers/s390,
> drivers/parisc, etc. There is also drivers/platform/x86, though that seems
> to be for the bus "platform drivers" rather than just a random character
> driver like the one in question.
>
> I don't have a particular opinion here; I'm just hoping to develop enough
> consensus that I can ask Linus to pull the driver without generating
> controversy :-)
The drivers/s390 and drivers/parisc directories are from a distant past,
we should not add new ones like them. drivers/platform is controversial,
but I think it's ok for stuff that manages platform specific quirks.
The main problem with that is that it doesn't work for embedded systems,
by extension every ARM specific driver could go into drivers/platform/...
and we don't want that.
You can probably argue that the tile drivers do fit in here as long as
they are specific to the hypervisor and not to some SOC specific hardware.
Arnd
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Chris Metcalf @ 2011-06-06 23:04 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Arnd Bergmann, kumar.gala, linux-kernel, akpm, Deepak Saxena,
linux-console, greg, linuxppc-dev, Timur Tabi
In-Reply-To: <20110606212356.GA20112@dumpdata.com>
For context, the most recent patch for the tile driver in question is here:
https://patchwork.kernel.org/patch/843892/
On 6/6/2011 5:23 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Jun 06, 2011 at 05:01:36PM -0400, Chris Metcalf wrote:
>> On 6/6/2011 12:24 PM, Arnd Bergmann wrote:
>>> On Monday 06 June 2011, Timur Tabi wrote:.
>>>> And what about my concern that my driver will be the only one in drivers/virt?
>>> I have no doubt that more of these will come. Chris Metcalf is currently
>>> looking for a home for his tilera hypervisor drivers, and we have the
>>> microsoft hyperv drivers in drivers/staging, so they will hopefully
>>> move to a proper place later. We also have similar drivers in other
>>> places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
>>> or parts of drivers/xen.
>> It might help if someone (Arnd?) wanted to propose a statement of what
>> drivers/virt was really for. If it's for any Linux driver that upcalls to
> Was for? I am not seeing it in v3.0-rc2?
Sorry, maybe a questionable idiom, but please read past tense in the quoted
text as meaning present tense :-)
>> a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
>> well. If it's intended more for drivers that guests running under a
>> hypervisor can use to talk to the hypervisor itself (e.g. managing
> I believe that the code that deals with specific subsystem (so block API
> for example) would reside in subsystem directory (so drivers/block would have
> your virtualization block driver). This allows the maintainer of block
> to make sure your driver is OK.
Sure, makes sense. The new push (as I understand it) is to group primarily
by function, not by bus or architecture.
>> notifications that a hypervisor delivers to a guest to cause it to shut
>> down or take other actions), then it doesn't seem like the Tilera
> That looks to be arch/<x>/tilera/virt/ candidate?
Arnd, among others, has suggested that all drivers live in "drivers"
somewhere, so "arch/tile" may not be the best place. (To be fair, I
originally had this driver in arch/tile/drivers/, so your idea is certainly
reasonable!)
>> paravirtualized device drivers belong there, since they're just using the
>> Tilera hypervisor synchronously to do I/O or get/set device and driver state.
> Well, I/O sounds like block API or network API. But then you are also
> doing management ioctl - which implies "drivers". "drivers/tilera" does not
> work?
There is certainly precedent for drivers that don't fit cleanly into an
existing category to go in drivers/<arch>, e.g. drivers/s390,
drivers/parisc, etc. There is also drivers/platform/x86, though that seems
to be for the bus "platform drivers" rather than just a random character
driver like the one in question.
I don't have a particular opinion here; I'm just hoping to develop enough
consensus that I can ask Linus to pull the driver without generating
controversy :-)
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply
* [PATCH] powerpc/5200: dts: digsy_mtc.dts: add timer0 and timer1 gpio properties
From: Anatolij Gustschin @ 2011-06-06 22:27 UTC (permalink / raw)
To: linuxppc-dev, grant.likely; +Cc: Anatolij Gustschin
In-Reply-To: <1305561764-5942-1-git-send-email-agust@denx.de>
timer0 and timer1 pins are used as simple GPIO on this board.
Add gpio-controller and #gpio-cells properties to timer nodes
so that we can control gpio lines using available MPC52xx
GPT driver.
Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
arch/powerpc/boot/dts/digsy_mtc.dts | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/boot/dts/digsy_mtc.dts b/arch/powerpc/boot/dts/digsy_mtc.dts
index e205d17..2aad7ae 100644
--- a/arch/powerpc/boot/dts/digsy_mtc.dts
+++ b/arch/powerpc/boot/dts/digsy_mtc.dts
@@ -23,7 +23,14 @@
soc5200@f0000000 {
timer@600 { // General Purpose Timer
+ #gpio-cells = <2>;
fsl,has-wdt;
+ gpio-controller;
+ };
+
+ timer@610 {
+ #gpio-cells = <2>;
+ gpio-controller;
};
rtc@800 {
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Konrad Rzeszutek Wilk @ 2011-06-06 21:23 UTC (permalink / raw)
To: Chris Metcalf
Cc: Arnd Bergmann, kumar.gala, linux-kernel, akpm, Deepak Saxena,
linux-console, greg, linuxppc-dev, Timur Tabi
In-Reply-To: <4DED4030.9050503@tilera.com>
On Mon, Jun 06, 2011 at 05:01:36PM -0400, Chris Metcalf wrote:
> On 6/6/2011 12:24 PM, Arnd Bergmann wrote:
> > On Monday 06 June 2011, Timur Tabi wrote:.
> >> And what about my concern that my driver will be the only one in drivers/virt?
> > I have no doubt that more of these will come. Chris Metcalf is currently
> > looking for a home for his tilera hypervisor drivers, and we have the
> > microsoft hyperv drivers in drivers/staging, so they will hopefully
> > move to a proper place later. We also have similar drivers in other
> > places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
> > or parts of drivers/xen.
>
> It might help if someone (Arnd?) wanted to propose a statement of what
> drivers/virt was really for. If it's for any Linux driver that upcalls to
Was for? I am not seeing it in v3.0-rc2?
> a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
> well. If it's intended more for drivers that guests running under a
> hypervisor can use to talk to the hypervisor itself (e.g. managing
I believe that the code that deals with specific subsystem (so block API
for example) would reside in subsystem directory (so drivers/block would have
your virtualization block driver). This allows the maintainer of block
to make sure your driver is OK.
> notifications that a hypervisor delivers to a guest to cause it to shut
> down or take other actions), then it doesn't seem like the Tilera
That looks to be arch/<x>/tilera/virt/ candidate?
> paravirtualized device drivers belong there, since they're just using the
> Tilera hypervisor synchronously to do I/O or get/set device and driver state.
Well, I/O sounds like block API or network API. But then you are also
doing management ioctl - which implies "drivers". "drivers/tilera" does not
work?
>
> --
> Chris Metcalf, Tilera Corp.
> http://www.tilera.com
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH -v2] Audit: push audit success and retcode into arch ptrace.h
From: David Miller @ 2011-06-06 20:17 UTC (permalink / raw)
To: eparis
Cc: linux-mips, linux-ia64, linux-sh, heiko.carstens, oleg, paulus,
hpa, sparclinux, linux-s390, richard, x86, mingo, fenghua.yu,
user-mode-linux-devel, microblaze-uclinux, jdike, viro, tglx,
monstr, tony.luck, linux-kernel, ralf, lethal, schwidefsky,
linux390, akpm, linuxppc-dev
In-Reply-To: <20110603220451.23134.47368.stgit@paris.rdu.redhat.com>
From: Eric Paris <eparis@redhat.com>
Date: Fri, 03 Jun 2011 18:04:51 -0400
...
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Acked-by: Acked-by: H. Peter Anvin <hpa@zytor.com> [for x86 portion]
For sparc parts:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Chris Metcalf @ 2011-06-06 21:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kumar.gala, linux-kernel, akpm, Deepak Saxena, linux-console,
greg, linuxppc-dev, Timur Tabi
In-Reply-To: <201106061824.53844.arnd@arndb.de>
On 6/6/2011 12:24 PM, Arnd Bergmann wrote:
> On Monday 06 June 2011, Timur Tabi wrote:.
>> And what about my concern that my driver will be the only one in drivers/virt?
> I have no doubt that more of these will come. Chris Metcalf is currently
> looking for a home for his tilera hypervisor drivers, and we have the
> microsoft hyperv drivers in drivers/staging, so they will hopefully
> move to a proper place later. We also have similar drivers in other
> places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
> or parts of drivers/xen.
It might help if someone (Arnd?) wanted to propose a statement of what
drivers/virt was really for. If it's for any Linux driver that upcalls to
a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
well. If it's intended more for drivers that guests running under a
hypervisor can use to talk to the hypervisor itself (e.g. managing
notifications that a hypervisor delivers to a guest to cause it to shut
down or take other actions), then it doesn't seem like the Tilera
paravirtualized device drivers belong there, since they're just using the
Tilera hypervisor synchronously to do I/O or get/set device and driver state.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-06 19:48 UTC (permalink / raw)
To: Scott Wood
Cc: greg, kumar.gala, linux-kernel, akpm, linux-console, linuxppc-dev,
Timur Tabi
In-Reply-To: <20110606131516.69b9f361@schlenkerla.am.freescale.net>
On Monday 06 June 2011 20:15:16 Scott Wood wrote:
> On Mon, 6 Jun 2011 17:53:09 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > You can't delete anything.
> > > >
> > > > rm, rmdir
> > > >
> > > > > You can't create empty nodes.
> > > >
> > > > mkdir
> > >
> > > I know how to operate a filesystem. You can't do these operations *on
> > > another guest's device tree through the hv interface*.
> >
> > Why not?
>
> Because the hypervisor does not support it. It provides only getprop and
> setprop. I think you took my "you can't do that" statements to be a
> statement about limitations of using a filesystem interfcae -- quite the
> opposite, I was saying the hv functionality is too limited to support a
> filesystem interface with normal semantics.
Ok, sorry for the confusion on my part. It makes a lot more sense now.
> > > And what would be the benefit of this major restructuring and added
> > > complexity?
> >
> > I think it would be a slightly better abstraction, and the complexity
> > is not as big as you make it sound. I'm mainly countering your statement
> > that it would be a bad interface or that would not possible to do.
> >
> > I'm not that opposed to having an ioctl interface for your hypervisor
> > interface, but I am opposed to making design choices based on
> > a bad representations of facts or not having considered the options
> > that other people suggest.
>
> I don't really see how a filesystem is a better abstraction for a wrapper
> around a procedural interface. A somewhat better argument is that ioctls
> are a pain, and Linux doesn't have a better way to expose a procedural
> interface, that doesn't require a wrapper program -- though as the wrapper
> already exists, and the fs interface would probably be sufficiently awkward
> that people would still use a wrapper, that doesn't buy us too much either.
>
> This is not being proposed as any sort of standard kernel API, just a way
> for userspace to get access to implementation-specific hcalls.
> Implementation-specific backdoor is practically the definition of ioctl. :-)
>
> I would be interested to see a concrete proposal for what this would look
> like as a filesystem, though, based on the actual operations that are
> available. How would you deal with getting all the parameters in,
> performing the operation, and getting the results back? What about when
> multiple processes are doing this at the same time? What would the memcpy
> hcall look like?
In fs/libfs.c, we have support for "simple transaction files", where you
write input parameters into a file and then read it back to get the
results. They are very rarely used, but can serve as a way to replace ioctls
with file operations where that makes sense.
For an inter-partition memcpy, a better interface might be a pipe
representation: You have a namespace that is shared between two
partitions, so each side can create directory entries with arbitrary
names in one of the subdirectories of the file system. Then you can
open the file for reading on one side and writing on the other side
and when both sides have started the respective operation, the hypervisor
will copy data. The possible ways to use that functionality are countless.
Similarly, you can mmap a file to get inter-partition shared memory.
Arnd
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Scott Wood @ 2011-06-06 18:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: greg, kumar.gala, linux-kernel, akpm, linux-console, linuxppc-dev,
Timur Tabi
In-Reply-To: <201106061753.10154.arnd@arndb.de>
On Mon, 6 Jun 2011 17:53:09 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 03 June 2011, Scott Wood wrote:
> > On Fri, 3 Jun 2011 17:28:43 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > > On Thursday 02 June 2011, Scott Wood wrote:
> > > > I wanted to have the hypervisor take an update dtb (we already have special
> > > > meta-properties for things like deletion as part of the hv config
> > > > mechanism). But others on the project wanted to keep it simple, and so
> > > > get/set property it was. :-/
> > > >
> > > > It's unlikely to change at this point without a real need.
> > > >
> > > > As for a filesystem interface, it's not a good match either.
> > > > You can't iterate over anything to read out the full tree from the hv.
> > >
> > > kexec iterates over /proc/device-tree to create a dts blob.
> >
> > That's irrelevant, because we're not talking about that device tree. We're
> > talking about the device tree of another hypervisor guest.
>
> I understand that it's a different device tree. That doesn't mean we
> can't use the same tools.
The kernel does not have the same sort of access to this tree.
> > > > You can't delete anything.
> > >
> > > rm, rmdir
> > >
> > > > You can't create empty nodes.
> > >
> > > mkdir
> >
> > I know how to operate a filesystem. You can't do these operations *on
> > another guest's device tree through the hv interface*.
>
> Why not?
Because the hypervisor does not support it. It provides only getprop and
setprop. I think you took my "you can't do that" statements to be a
statement about limitations of using a filesystem interfcae -- quite the
opposite, I was saying the hv functionality is too limited to support a
filesystem interface with normal semantics.
> > And what would be the benefit of this major restructuring and added
> > complexity?
>
> I think it would be a slightly better abstraction, and the complexity
> is not as big as you make it sound. I'm mainly countering your statement
> that it would be a bad interface or that would not possible to do.
>
> I'm not that opposed to having an ioctl interface for your hypervisor
> interface, but I am opposed to making design choices based on
> a bad representations of facts or not having considered the options
> that other people suggest.
I don't really see how a filesystem is a better abstraction for a wrapper
around a procedural interface. A somewhat better argument is that ioctls
are a pain, and Linux doesn't have a better way to expose a procedural
interface, that doesn't require a wrapper program -- though as the wrapper
already exists, and the fs interface would probably be sufficiently awkward
that people would still use a wrapper, that doesn't buy us too much either.
This is not being proposed as any sort of standard kernel API, just a way
for userspace to get access to implementation-specific hcalls.
Implementation-specific backdoor is practically the definition of ioctl. :-)
I would be interested to see a concrete proposal for what this would look
like as a filesystem, though, based on the actual operations that are
available. How would you deal with getting all the parameters in,
performing the operation, and getting the results back? What about when
multiple processes are doing this at the same time? What would the memcpy
hcall look like?
-Scott
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Timur Tabi @ 2011-06-06 16:27 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kumar.gala, linux-kernel, Chris Metcalf, akpm, Deepak Saxena,
linux-console, greg, linuxppc-dev
In-Reply-To: <201106061824.53844.arnd@arndb.de>
Arnd Bergmann wrote:
> I have no doubt that more of these will come. Chris Metcalf is currently
> looking for a home for his tilera hypervisor drivers, and we have the
> microsoft hyperv drivers in drivers/staging, so they will hopefully
> move to a proper place later. We also have similar drivers in other
> places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
> or parts of drivers/xen.
Alright, drivers/virt it is. I'll post a v4 once everyone else has had a chance
to comment on v3.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-06 16:24 UTC (permalink / raw)
To: Timur Tabi
Cc: kumar.gala, linux-kernel, Chris Metcalf, akpm, Deepak Saxena,
linux-console, greg, linuxppc-dev
In-Reply-To: <4DECFBC5.6070205@freescale.com>
On Monday 06 June 2011, Timur Tabi wrote:
> Arnd Bergmann wrote:
> > When we talked about the situation of drivers/misc and drivers/char at
> > one of the recent conferences, a broad consensus was that they are in
> > need of a maintainer, which I foolishly signed up for. Deepak wanted
> > to send an update to the MAINTAINERS file for this (I guess I can do
> > that too, since he must have forgotten about it), but the main idea is
> > that I'm there to say no to any driver that someone tries to add there,
> > unless there are really good reasons why it is actually a good place
> > to live for that driver.
>
> Can you give me an example of a driver that does belong in drivers/misc?
Not really.
> Frankly, I just don't see what's wrong with a repository of various drivers that
> don't really belong anywhere else.
The main problem is that for the most part it's a pile of crap that
nobody wants to look at, so drivers getting added there see basically
no real review.
> And what about my concern that my driver will be the only one in drivers/virt?
I have no doubt that more of these will come. Chris Metcalf is currently
looking for a home for his tilera hypervisor drivers, and we have the
microsoft hyperv drivers in drivers/staging, so they will hopefully
move to a proper place later. We also have similar drivers in other
places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
or parts of drivers/xen.
Arnd
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Timur Tabi @ 2011-06-06 16:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kumar.gala, linux-kernel, akpm, Deepak Saxena, linux-console,
greg, linuxppc-dev
In-Reply-To: <201106061803.58531.arnd@arndb.de>
Arnd Bergmann wrote:
> When we talked about the situation of drivers/misc and drivers/char at
> one of the recent conferences, a broad consensus was that they are in
> need of a maintainer, which I foolishly signed up for. Deepak wanted
> to send an update to the MAINTAINERS file for this (I guess I can do
> that too, since he must have forgotten about it), but the main idea is
> that I'm there to say no to any driver that someone tries to add there,
> unless there are really good reasons why it is actually a good place
> to live for that driver.
Can you give me an example of a driver that *does* belong in drivers/misc?
Frankly, I just don't see what's wrong with a repository of various drivers that
don't really belong anywhere else.
And what about my concern that my driver will be the only one in drivers/virt?
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-06 16:03 UTC (permalink / raw)
To: Timur Tabi, Deepak Saxena
Cc: kumar.gala, linux-kernel, akpm, linux-console, greg, linuxppc-dev
In-Reply-To: <4DECF6B3.5020700@freescale.com>
On Monday 06 June 2011, Timur Tabi wrote:
> Arnd Bergmann wrote:
> > Sorry, I misread your first sentence above. I thought you said that you prefer
> > drivers/firmware over virt/fsl. drivers/misc is definitely the wrong
> > place for this, please choose a better one. Maybe drivers/virt/ ?
>
> I'll be more than happy to go with the consensus, but I don't think it makes
> sense to create a new directory just for this one, limited-use driver. You're
> the only person who's complained about drivers/misc. I'm pretty sure that if I
> put it in drivers/virt, I'll get more complaints.
>
> I still don't understand what's wrong with drivers/misc, especially since my
> driver registers as a "misc" driver.
I basically think that drivers/misc is wrong for most of the stuff that is
already in there, either because the drivers actually fit into a subsystem
together with other drivers or because they contain rather horrible code.
The idea that drivers using misc_register belong into drivers/misc is a
common misconception. Traditionally they go to drivers/char, which would
still be a better choice, and most "misc" drivers are actually part of a
proper subsystem, while most drivers in drivers/misc don't have a character
device interface.
When we talked about the situation of drivers/misc and drivers/char at
one of the recent conferences, a broad consensus was that they are in
need of a maintainer, which I foolishly signed up for. Deepak wanted
to send an update to the MAINTAINERS file for this (I guess I can do
that too, since he must have forgotten about it), but the main idea is
that I'm there to say no to any driver that someone tries to add there,
unless there are really good reasons why it is actually a good place
to live for that driver.
Arnd
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-06 15:53 UTC (permalink / raw)
To: Scott Wood
Cc: greg, kumar.gala, linux-kernel, akpm, linux-console, linuxppc-dev,
Timur Tabi
In-Reply-To: <20110603112213.4f4d1b4e@schlenkerla.am.freescale.net>
On Friday 03 June 2011, Scott Wood wrote:
> On Fri, 3 Jun 2011 17:28:43 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > On Thursday 02 June 2011, Scott Wood wrote:
> > > I wanted to have the hypervisor take an update dtb (we already have special
> > > meta-properties for things like deletion as part of the hv config
> > > mechanism). But others on the project wanted to keep it simple, and so
> > > get/set property it was. :-/
> > >
> > > It's unlikely to change at this point without a real need.
> > >
> > > As for a filesystem interface, it's not a good match either.
> > > You can't iterate over anything to read out the full tree from the hv.
> >
> > kexec iterates over /proc/device-tree to create a dts blob.
>
> That's irrelevant, because we're not talking about that device tree. We're
> talking about the device tree of another hypervisor guest.
I understand that it's a different device tree. That doesn't mean we
can't use the same tools.
> > > You can't delete anything.
> >
> > rm, rmdir
> >
> > > You can't create empty nodes.
> >
> > mkdir
>
> I know how to operate a filesystem. You can't do these operations *on
> another guest's device tree through the hv interface*.
Why not? From a device driver perspective, it's not much of a difference
whether you export a device (or hypervisor, firmware, ...) setting
as an ioctl or an inode operation.
> > > There would still be other ioctls needed for starting/stopping the
> > > partition, etc.
> >
> > Right, although you could model them as a file interface as well.
> > KVMfs is one example doing that.
>
> And what would be the benefit of this major restructuring and added
> complexity?
I think it would be a slightly better abstraction, and the complexity
is not as big as you make it sound. I'm mainly countering your statement
that it would be a bad interface or that would not possible to do.
I'm not that opposed to having an ioctl interface for your hypervisor
interface, but I am opposed to making design choices based on
a bad representations of facts or not having considered the options
that other people suggest.
Arnd
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Timur Tabi @ 2011-06-06 15:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kumar.gala, linux-kernel, akpm, linux-console, greg, linuxppc-dev
In-Reply-To: <201106061742.01974.arnd@arndb.de>
Arnd Bergmann wrote:
> Sorry, I misread your first sentence above. I thought you said that you prefer
> drivers/firmware over virt/fsl. drivers/misc is definitely the wrong
> place for this, please choose a better one. Maybe drivers/virt/ ?
I'll be more than happy to go with the consensus, but I don't think it makes
sense to create a new directory just for this one, limited-use driver. You're
the only person who's complained about drivers/misc. I'm pretty sure that if I
put it in drivers/virt, I'll get more complaints.
I still don't understand what's wrong with drivers/misc, especially since my
driver registers as a "misc" driver.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 7/7] [v2] drivers/misc: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-06 15:42 UTC (permalink / raw)
To: Timur Tabi
Cc: kumar.gala, linux-kernel, akpm, linux-console, greg, linuxppc-dev
In-Reply-To: <4DE8FD85.7000703@freescale.com>
On Friday 03 June 2011, Timur Tabi wrote:
> Arnd Bergmann wrote:
> >> > I don't think it's correct to think of a hypervisor as firmware, so I don't
> >> > think drivers/firmware is better.
> >> >
> >> > I'm not sure that creating virt/fsl and putting the driver in there is a good
> >> > idea, because it will be the only driver in that directory. Unlike KVM, this
> >> > driver is just a collection of front-ends to our hypervisor API. The actual
> >> > hypervisor is completely separate. That's why I put it in drivers/misc, because
> >> > it's just a single driver with a miscellaneous collection of interfaces.
>
> > Ok, fair enough. If nobody has a strong preference any other way, just make it
> > drivers/firmware then.
>
> Did you mean to say drivers/misc?
Sorry, I misread your first sentence above. I thought you said that you prefer
drivers/firmware over virt/fsl. drivers/misc is definitely the wrong
place for this, please choose a better one. Maybe drivers/virt/ ?
Arnd
^ permalink raw reply
* Re: [RFC][PATCH] powerpc: Use the #address-cells information to parsememory/reg
From: Suzuki Poulose @ 2011-06-06 11:29 UTC (permalink / raw)
To: David Laight
Cc: Sebastian Andrzej Siewior, kexec, lkml, Simon Horman,
linux ppc dev, Vivek Goyal
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6D8AD73@saturn3.aculab.com>
On 06/06/11 14:30, David Laight wrote:
>>> Changed the add_usable_mem_property() to accept FILE* fp instead of
> int fd,
>>> as most of the other users of read_memory_region_limits() deals with
> FILE*.
>>>
>>> Signed-off-by: Suzuki K. Poulose<suzuki@in.ibm.com>
>>
>> Could you please let me know your thoughts/comments about this patch ?
>
> Is the change to use 'FILE *' actually progress?
> I'd have thought that the randomly aligned read/lseek system calls
> that this allows to happen are not desirable for anything that
> isn't a true file.
I will revert the other users back to 'fd'
>
> I'd also suggest that the sizeof's should be applied to the
> actual type of the variable being read/written, not arbitrarily
> 'long' or 'int', and this probably ought to be some fixed size type.
I have used 'unsigned long'(for word sized values) or 'unsigned long long'
(for double words) just to make sure we get the right values. Is this OK ?
Thanks
Suzuki
^ permalink raw reply
* Re: [RFC][PATCH] powerpc: Use the #address-cells information to parse memory/reg
From: Suzuki Poulose @ 2011-06-06 11:02 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: kexec@lists.infradead.org, lkml, Simon Horman, linux ppc dev,
Vivek Goyal
In-Reply-To: <4DEC950E.1020105@linutronix.de>
On 06/06/11 14:21, Sebastian Andrzej Siewior wrote:
> Suzuki Poulose wrote:
>> Could you please let me know your thoughts/comments about this patch ?
>
> I'm mostly fine with it.
>
> Maaxim copied fs2dt.c from ppc64 to ppc. So I guess ppc64 has the same
> problem.
Yes, you are right. Porting this patch over to ppc64 is in my TODO list.
> ARM and MIPS is soon having DT support and kexec is probably also
> on their list so I would hate to see them to either copy the DT parsing
> file or having their own implementation.
>
> Maybe we should try to use libfdt for dt parsing. It has /proc import
> support so it should be fine for our needs. It is already in tree and used
> by ppc32 if a basic dtb is specified. I'm not sure if the /proc interface
> is part of dtc or libfdt.
>
> I'm not saying this has to be done now but maybe later before ARM and/or
> MIPS comes along needs something similar for their needs. If the libfdt is
> too complex for sucking in the dtb from /proc then maybe something else
> that generic and can be shared between booth ppc architectures and the
> other ones.
OK
>>> Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>>> ===================================================================
>>> --- kexec-tools-2.0.4.orig/kexec/arch/ppc/kexec-ppc.c
>>> +++ kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>>> @@ -34,6 +35,92 @@ unsigned int rtas_base, rtas_size;
>>> int max_memory_ranges;
>>> const char *ramdisk;
>>>
>>> +/*
>>> + * Reads the #address-cells and #size-cells on this platform.
>>> + * This is used to parse the memory/reg info from the device-tree
>>> + */
>>> +int init_memory_region_info()
>>> +{
>>> + size_t res = 0;
>>> + FILE *fp;
>>> + char *file;
>>> +
>>> + file = "/proc/device-tree/#address-cells";
>>> + fp = fopen(file, "r");
>>> + if (!fp) {
>>> + fprintf(stderr,"Unable to open %s\n", file);
>>> + return -1;
>>> + }
>>> +
>>> + res = fread(&dt_address_cells,sizeof(unsigned long),1,fp);
>>> + if (res != 1) {
>>> + fprintf(stderr,"Error reading %s\n", file);
>>> + return -1;
>>> + }
>>> + fclose(fp);
>>> + dt_address_cells *= sizeof(unsigned long);
>
> This should be sizeof(unsigned int). I know we on 32bit.
>
OK. I was using (unsigned long) to get the word size on the machine. Given
this code is duplicated in ppc64, thought of having a generic code which works
fine for all ppcXX. As you mentioned, if we go about moving to a single copy of
fdt code, using long would help us.
>>> + file = "/proc/device-tree/#size-cells";
>>> + fp = fopen(file, "r");
>>> + if (!fp) {
>>> + fprintf(stderr,"Unable to open %s\n", file);
>>> + return -1;
>>> + }
>>> +
>>> + res = fread(&dt_size_cells,sizeof(unsigned long),1,fp);
>>> + if (res != 1) {
>>> + fprintf(stderr,"Error reading %s\n", file);
>>> + return -1;
>>> + }
>>> + fclose(fp);
>>> + dt_size_cells *= sizeof(unsigned long);
>
> same here.
Thanks
Suzuki
^ permalink raw reply
* RE: [RFC][PATCH] powerpc: Use the #address-cells information to parsememory/reg
From: David Laight @ 2011-06-06 9:00 UTC (permalink / raw)
To: Suzuki Poulose, Simon Horman
Cc: linux ppc dev, Sebastian Andrzej Siewior, kexec, lkml,
Vivek Goyal
In-Reply-To: <4DEC7F49.8060000@in.ibm.com>
> > Changed the add_usable_mem_property() to accept FILE* fp instead of
int fd,
> > as most of the other users of read_memory_region_limits() deals with
FILE*.
> >
> > Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
>=20
> Could you please let me know your thoughts/comments about this patch ?
Is the change to use 'FILE *' actually progress?
I'd have thought that the randomly aligned read/lseek system calls
that this allows to happen are not desirable for anything that
isn't a true file.
I'd also suggest that the sizeof's should be applied to the
actual type of the variable being read/written, not arbitrarily
'long' or 'int', and this probably ought to be some fixed size type.
David
^ permalink raw reply
* Re: [RFC][PATCH] powerpc: Use the #address-cells information to parse memory/reg
From: Sebastian Andrzej Siewior @ 2011-06-06 8:51 UTC (permalink / raw)
To: Suzuki Poulose
Cc: kexec@lists.infradead.org, lkml, Simon Horman, linux ppc dev,
Vivek Goyal
In-Reply-To: <4DEC7F49.8060000@in.ibm.com>
Suzuki Poulose wrote:
> Could you please let me know your thoughts/comments about this patch ?
I'm mostly fine with it.
Maaxim copied fs2dt.c from ppc64 to ppc. So I guess ppc64 has the same
problem. ARM and MIPS is soon having DT support and kexec is probably also
on their list so I would hate to see them to either copy the DT parsing
file or having their own implementation.
Maybe we should try to use libfdt for dt parsing. It has /proc import
support so it should be fine for our needs. It is already in tree and used
by ppc32 if a basic dtb is specified. I'm not sure if the /proc interface
is part of dtc or libfdt.
I'm not saying this has to be done now but maybe later before ARM and/or
MIPS comes along needs something similar for their needs. If the libfdt is
too complex for sucking in the dtb from /proc then maybe something else
that generic and can be shared between booth ppc architectures and the
other ones.
> Thanks
> Suzuki
>
>>
>> Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>> ===================================================================
>> --- kexec-tools-2.0.4.orig/kexec/arch/ppc/kexec-ppc.c
>> +++ kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>> @@ -34,6 +35,92 @@ unsigned int rtas_base, rtas_size;
>> int max_memory_ranges;
>> const char *ramdisk;
>>
>> +/*
>> + * Reads the #address-cells and #size-cells on this platform.
>> + * This is used to parse the memory/reg info from the device-tree
>> + */
>> +int init_memory_region_info()
>> +{
>> + size_t res = 0;
>> + FILE *fp;
>> + char *file;
>> +
>> + file = "/proc/device-tree/#address-cells";
>> + fp = fopen(file, "r");
>> + if (!fp) {
>> + fprintf(stderr,"Unable to open %s\n", file);
>> + return -1;
>> + }
>> +
>> + res = fread(&dt_address_cells,sizeof(unsigned long),1,fp);
>> + if (res != 1) {
>> + fprintf(stderr,"Error reading %s\n", file);
>> + return -1;
>> + }
>> + fclose(fp);
>> + dt_address_cells *= sizeof(unsigned long);
This should be sizeof(unsigned int). I know we on 32bit.
>> + file = "/proc/device-tree/#size-cells";
>> + fp = fopen(file, "r");
>> + if (!fp) {
>> + fprintf(stderr,"Unable to open %s\n", file);
>> + return -1;
>> + }
>> +
>> + res = fread(&dt_size_cells,sizeof(unsigned long),1,fp);
>> + if (res != 1) {
>> + fprintf(stderr,"Error reading %s\n", file);
>> + return -1;
>> + }
>> + fclose(fp);
>> + dt_size_cells *= sizeof(unsigned long);
same here.
>> +
>> + return 0;
>> +}
>> +
Sebastian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox