Re: [PATCH 1/6 v5] Kernel DLPAR Infrastructure
Benjamin Herrenschmidt wrote:
On Wed, 2009-10-28 at 15:53 -0500, Nathan Fontenot wrote:
This patch provides the kernel DLPAR infrastructure in a new filed named
dlpar.c. The functionality provided is for acquiring and releasing a resource
from firmware and the parsing of information returned from the
ibm,configure-connector rtas call. Additionally this exports the pSeries
reconfiguration notifier chain so that it can be invoked when device tree
updates are made.
Signed-off-by: Nathan Fontenot nfont at austin.ibm.com
---
Hi Nathan !
Finally I get to review this stuff :-)
Thanks!
+#define CFG_CONN_WORK_SIZE 4096
+static char workarea[CFG_CONN_WORK_SIZE];
+static DEFINE_SPINLOCK(workarea_lock);
So I'm not a huge fan of this workarea static. First a static is in
effect a global name (as far as System.map etc... are concerned) so it
would warrant a better name. Then, do we really want that 4K of BSS
taken even on platforms that don't do dlpar ? Any reason why you don't
just pop a free page with __get_free_page() inside of
configure_connector() ?
I'm not either, having a static buffer and a lock feels like overkill
for this. I tried kmalloc, but that didn't work. I'll try using
__get_free_page.
+struct cc_workarea {
+ u32 drc_index;
+ u32 zero;
+ u32 name_offset;
+ u32 prop_length;
+ u32 prop_offset;
+};
+
+static struct property *parse_cc_property(char *workarea)
+{
+ struct property *prop;
+ struct cc_workarea *ccwa;
+ char *name;
+ char *value;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop)
+ return NULL;
+
+ ccwa = (struct cc_workarea *)workarea;
+ name = workarea + ccwa-name_offset;
+ prop-name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+ if (!prop-name) {
+ kfree(prop);
+ return NULL;
+ }
+
+ strcpy(prop-name, name);
+
+ prop-length = ccwa-prop_length;
+ value = workarea + ccwa-prop_offset;
+ prop-value = kzalloc(prop-length, GFP_KERNEL);
+ if (!prop-value) {
+ kfree(prop-name);
+ kfree(prop);
+ return NULL;
+ }
+
+ memcpy(prop-value, value, prop-length);
+ return prop;
+}
+
+static void free_property(struct property *prop)
+{
+ kfree(prop-name);
+ kfree(prop-value);
+ kfree(prop);
+}
+
+static struct device_node *parse_cc_node(char *work_area)
+{
const char* maybe ?
sure.
+ struct device_node *dn;
+ struct cc_workarea *ccwa;
+ char *name;
+
+ dn = kzalloc(sizeof(*dn), GFP_KERNEL);
+ if (!dn)
+ return NULL;
+
+ ccwa = (struct cc_workarea *)work_area;
+ name = work_area + ccwa-name_offset;
I'm wondering whether work_area should be a struct cc_workarea * in the
first place with a char data[] at the end, but that would mean probably
tweaking the offsets... no big deal, up to you.
I'll look onto that. Anything that makes this easier to understand is good.
+ dn-full_name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+ if (!dn-full_name) {
+ kfree(dn);
+ return NULL;
+ }
+
+ strcpy(dn-full_name, name);
kstrdup ?
yep, should have used kstrdup.
.../...
+#define NEXT_SIBLING1
+#define NEXT_CHILD 2
+#define NEXT_PROPERTY 3
+#define PREV_PARENT 4
+#define MORE_MEMORY 5
+#define CALL_AGAIN -2
+#define ERR_CFG_USE -9003
+
+struct device_node *configure_connector(u32 drc_index)
+{
It's a global exported function, I'd rather you call it
dlpar_configure_connector()
ok.
+ struct device_node *dn;
+ struct device_node *first_dn = NULL;
+ struct device_node *last_dn = NULL;
+ struct property *property;
+ struct property *last_property = NULL;
+ struct cc_workarea *ccwa;
+ int cc_token;
+ int rc;
+
+ cc_token = rtas_token(ibm,configure-connector);
+ if (cc_token == RTAS_UNKNOWN_SERVICE)
+ return NULL;
+
+ spin_lock(workarea_lock);
+
+ ccwa = (struct cc_workarea *)workarea[0];
+ ccwa-drc_index = drc_index;
+ ccwa-zero = 0;
Popping a free page with gfp (or just kmalloc'ing 4K) would avoid the
need for the lock too.
yes, see comment at beginning.
+ rc = rtas_call(cc_token, 2, 1, NULL, workarea, NULL);
+ while (rc) {
+ switch (rc) {
+ case NEXT_SIBLING:
+ dn = parse_cc_node(workarea);
+ if (!dn)
+ goto cc_error;
+
+ dn-parent = last_dn-parent;
+ last_dn-sibling = dn;
+ last_dn = dn;
+ break;
+
+ case NEXT_CHILD:
+ dn = parse_cc_node(workarea);
+ if (!dn)
+ goto cc_error;
+
+
Re: [PATCH 1/6 v5] Kernel DLPAR Infrastructure
On Mon, Nov 2, 2009 at 9:27 AM, Nathan Fontenot nf...@austin.ibm.com wrote: I saw that Grant Likely is doing updates to all of the of_* stuff right now, would it be ok to have these routines here, renamed as dlpar_*, and look to merge them in with Grant's updates when he finishes? No because then we're stuck with renaming the API at a later date. Name it what it is, and put it where it belongs. I'll deal with any merge breakage as it occurs. g. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/6 v5] Kernel DLPAR Infrastructure
Grant Likely wrote: On Mon, Nov 2, 2009 at 9:27 AM, Nathan Fontenot nf...@austin.ibm.com wrote: I saw that Grant Likely is doing updates to all of the of_* stuff right now, would it be ok to have these routines here, renamed as dlpar_*, and look to merge them in with Grant's updates when he finishes? No because then we're stuck with renaming the API at a later date. Name it what it is, and put it where it belongs. I'll deal with any merge breakage as it occurs. ok. Would this be better off in powerpc code, or should I go ahead and put it in something like drivers/of/dynamic.c? -Nathan Fontenot ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/6 v5] Kernel DLPAR Infrastructure
On Mon, Nov 2, 2009 at 9:47 AM, Nathan Fontenot nf...@austin.ibm.com wrote: Grant Likely wrote: On Mon, Nov 2, 2009 at 9:27 AM, Nathan Fontenot nf...@austin.ibm.com wrote: I saw that Grant Likely is doing updates to all of the of_* stuff right now, would it be ok to have these routines here, renamed as dlpar_*, and look to merge them in with Grant's updates when he finishes? No because then we're stuck with renaming the API at a later date. Name it what it is, and put it where it belongs. I'll deal with any merge breakage as it occurs. ok. Would this be better off in powerpc code, or should I go ahead and put it in something like drivers/of/dynamic.c? drivers/of/dynamic.c sounds fine to me. I can always move them if it find a better place. Send the patch to me and cc: the devicetree-disc...@lists.ozlabs.org mailing list. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/6 v5] Kernel DLPAR Infrastructure
This patch provides the kernel DLPAR infrastructure in a new filed named
dlpar.c. The functionality provided is for acquiring and releasing a resource
from firmware and the parsing of information returned from the
ibm,configure-connector rtas call. Additionally this exports the pSeries
reconfiguration notifier chain so that it can be invoked when device tree
updates are made.
Signed-off-by: Nathan Fontenot nfont at austin.ibm.com
---
Index: powerpc/arch/powerpc/platforms/pseries/dlpar.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ powerpc/arch/powerpc/platforms/pseries/dlpar.c 2009-10-28
15:21:38.0 -0500
@@ -0,0 +1,414 @@
+/*
+ * dlpar.c - support for dynamic reconfiguration (including PCI
+ * Hotplug and Dynamic Logical Partitioning on RPA platforms).
+ *
+ * Copyright (C) 2009 Nathan Fontenot
+ * Copyright (C) 2009 IBM Corporation
+ *
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ */
+
+#include linux/kernel.h
+#include linux/kref.h
+#include linux/notifier.h
+#include linux/proc_fs.h
+#include linux/spinlock.h
+
+#include asm/prom.h
+#include asm/machdep.h
+#include asm/uaccess.h
+#include asm/rtas.h
+#include asm/pSeries_reconfig.h
+
+#define CFG_CONN_WORK_SIZE 4096
+static char workarea[CFG_CONN_WORK_SIZE];
+static DEFINE_SPINLOCK(workarea_lock);
+
+struct cc_workarea {
+ u32 drc_index;
+ u32 zero;
+ u32 name_offset;
+ u32 prop_length;
+ u32 prop_offset;
+};
+
+static struct property *parse_cc_property(char *workarea)
+{
+ struct property *prop;
+ struct cc_workarea *ccwa;
+ char *name;
+ char *value;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop)
+ return NULL;
+
+ ccwa = (struct cc_workarea *)workarea;
+ name = workarea + ccwa-name_offset;
+ prop-name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+ if (!prop-name) {
+ kfree(prop);
+ return NULL;
+ }
+
+ strcpy(prop-name, name);
+
+ prop-length = ccwa-prop_length;
+ value = workarea + ccwa-prop_offset;
+ prop-value = kzalloc(prop-length, GFP_KERNEL);
+ if (!prop-value) {
+ kfree(prop-name);
+ kfree(prop);
+ return NULL;
+ }
+
+ memcpy(prop-value, value, prop-length);
+ return prop;
+}
+
+static void free_property(struct property *prop)
+{
+ kfree(prop-name);
+ kfree(prop-value);
+ kfree(prop);
+}
+
+static struct device_node *parse_cc_node(char *work_area)
+{
+ struct device_node *dn;
+ struct cc_workarea *ccwa;
+ char *name;
+
+ dn = kzalloc(sizeof(*dn), GFP_KERNEL);
+ if (!dn)
+ return NULL;
+
+ ccwa = (struct cc_workarea *)work_area;
+ name = work_area + ccwa-name_offset;
+ dn-full_name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+ if (!dn-full_name) {
+ kfree(dn);
+ return NULL;
+ }
+
+ strcpy(dn-full_name, name);
+ return dn;
+}
+
+static void free_one_cc_node(struct device_node *dn)
+{
+ struct property *prop;
+
+ while (dn-properties) {
+ prop = dn-properties;
+ dn-properties = prop-next;
+ free_property(prop);
+ }
+
+ kfree(dn-full_name);
+ kfree(dn);
+}
+
+static void free_cc_nodes(struct device_node *dn)
+{
+ if (dn-child)
+ free_cc_nodes(dn-child);
+
+ if (dn-sibling)
+ free_cc_nodes(dn-sibling);
+
+ free_one_cc_node(dn);
+}
+
+#define NEXT_SIBLING1
+#define NEXT_CHILD 2
+#define NEXT_PROPERTY 3
+#define PREV_PARENT 4
+#define MORE_MEMORY 5
+#define CALL_AGAIN -2
+#define ERR_CFG_USE -9003
+
+struct device_node *configure_connector(u32 drc_index)
+{
+ struct device_node *dn;
+ struct device_node *first_dn = NULL;
+ struct device_node *last_dn = NULL;
+ struct property *property;
+ struct property *last_property = NULL;
+ struct cc_workarea *ccwa;
+ int cc_token;
+ int rc;
+
+ cc_token = rtas_token(ibm,configure-connector);
+ if (cc_token == RTAS_UNKNOWN_SERVICE)
+ return NULL;
+
+ spin_lock(workarea_lock);
+
+ ccwa = (struct cc_workarea *)workarea[0];
+ ccwa-drc_index = drc_index;
+ ccwa-zero = 0;
+
+ rc = rtas_call(cc_token, 2, 1, NULL, workarea, NULL);
+ while (rc) {
+ switch (rc) {
+ case NEXT_SIBLING:
+ dn = parse_cc_node(workarea);
+ if (!dn)
+ goto cc_error;
+
+ dn-parent = last_dn-parent;
+ last_dn-sibling =
Re: [PATCH 1/6 v5] Kernel DLPAR Infrastructure
On Wed, 2009-10-28 at 15:53 -0500, Nathan Fontenot wrote:
This patch provides the kernel DLPAR infrastructure in a new filed named
dlpar.c. The functionality provided is for acquiring and releasing a resource
from firmware and the parsing of information returned from the
ibm,configure-connector rtas call. Additionally this exports the pSeries
reconfiguration notifier chain so that it can be invoked when device tree
updates are made.
Signed-off-by: Nathan Fontenot nfont at austin.ibm.com
---
Hi Nathan !
Finally I get to review this stuff :-)
+#define CFG_CONN_WORK_SIZE 4096
+static char workarea[CFG_CONN_WORK_SIZE];
+static DEFINE_SPINLOCK(workarea_lock);
So I'm not a huge fan of this workarea static. First a static is in
effect a global name (as far as System.map etc... are concerned) so it
would warrant a better name. Then, do we really want that 4K of BSS
taken even on platforms that don't do dlpar ? Any reason why you don't
just pop a free page with __get_free_page() inside of
configure_connector() ?
+struct cc_workarea {
+ u32 drc_index;
+ u32 zero;
+ u32 name_offset;
+ u32 prop_length;
+ u32 prop_offset;
+};
+
+static struct property *parse_cc_property(char *workarea)
+{
+ struct property *prop;
+ struct cc_workarea *ccwa;
+ char *name;
+ char *value;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop)
+ return NULL;
+
+ ccwa = (struct cc_workarea *)workarea;
+ name = workarea + ccwa-name_offset;
+ prop-name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+ if (!prop-name) {
+ kfree(prop);
+ return NULL;
+ }
+
+ strcpy(prop-name, name);
+
+ prop-length = ccwa-prop_length;
+ value = workarea + ccwa-prop_offset;
+ prop-value = kzalloc(prop-length, GFP_KERNEL);
+ if (!prop-value) {
+ kfree(prop-name);
+ kfree(prop);
+ return NULL;
+ }
+
+ memcpy(prop-value, value, prop-length);
+ return prop;
+}
+
+static void free_property(struct property *prop)
+{
+ kfree(prop-name);
+ kfree(prop-value);
+ kfree(prop);
+}
+
+static struct device_node *parse_cc_node(char *work_area)
+{
const char* maybe ?
+ struct device_node *dn;
+ struct cc_workarea *ccwa;
+ char *name;
+
+ dn = kzalloc(sizeof(*dn), GFP_KERNEL);
+ if (!dn)
+ return NULL;
+
+ ccwa = (struct cc_workarea *)work_area;
+ name = work_area + ccwa-name_offset;
I'm wondering whether work_area should be a struct cc_workarea * in the
first place with a char data[] at the end, but that would mean probably
tweaking the offsets... no big deal, up to you.
+ dn-full_name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+ if (!dn-full_name) {
+ kfree(dn);
+ return NULL;
+ }
+
+ strcpy(dn-full_name, name);
kstrdup ?
.../...
+#define NEXT_SIBLING1
+#define NEXT_CHILD 2
+#define NEXT_PROPERTY 3
+#define PREV_PARENT 4
+#define MORE_MEMORY 5
+#define CALL_AGAIN -2
+#define ERR_CFG_USE -9003
+
+struct device_node *configure_connector(u32 drc_index)
+{
It's a global exported function, I'd rather you call it
dlpar_configure_connector()
+ struct device_node *dn;
+ struct device_node *first_dn = NULL;
+ struct device_node *last_dn = NULL;
+ struct property *property;
+ struct property *last_property = NULL;
+ struct cc_workarea *ccwa;
+ int cc_token;
+ int rc;
+
+ cc_token = rtas_token(ibm,configure-connector);
+ if (cc_token == RTAS_UNKNOWN_SERVICE)
+ return NULL;
+
+ spin_lock(workarea_lock);
+
+ ccwa = (struct cc_workarea *)workarea[0];
+ ccwa-drc_index = drc_index;
+ ccwa-zero = 0;
Popping a free page with gfp (or just kmalloc'ing 4K) would avoid the
need for the lock too.
+ rc = rtas_call(cc_token, 2, 1, NULL, workarea, NULL);
+ while (rc) {
+ switch (rc) {
+ case NEXT_SIBLING:
+ dn = parse_cc_node(workarea);
+ if (!dn)
+ goto cc_error;
+
+ dn-parent = last_dn-parent;
+ last_dn-sibling = dn;
+ last_dn = dn;
+ break;
+
+ case NEXT_CHILD:
+ dn = parse_cc_node(workarea);
+ if (!dn)
+ goto cc_error;
+
+ if (!first_dn)
+ first_dn = dn;
+ else {
+ dn-parent = last_dn;
+ if (last_dn)
+ last_dn-child = dn;
+ }
+
+ last_dn = dn;
+ break;
+
+ case NEXT_PROPERTY:
+
Re: [PATCH 1/6 v5] Kernel DLPAR Infrastructure
On Thu, 2009-10-29 at 14:08 +1100, Benjamin Herrenschmidt wrote: On Wed, 2009-10-28 at 15:53 -0500, Nathan Fontenot wrote: + struct device_node *dn; + struct device_node *first_dn = NULL; + struct device_node *last_dn = NULL; + struct property *property; + struct property *last_property = NULL; + struct cc_workarea *ccwa; + int cc_token; + int rc; + + cc_token = rtas_token(ibm,configure-connector); + if (cc_token == RTAS_UNKNOWN_SERVICE) + return NULL; + + spin_lock(workarea_lock); + + ccwa = (struct cc_workarea *)workarea[0]; + ccwa-drc_index = drc_index; + ccwa-zero = 0; Popping a free page with gfp (or just kmalloc'ing 4K) would avoid the need for the lock too. Not kmalloc -- the alignment of the buffer isn't guaranteed when slub/slab debug is on, and iirc the work area needs to be 4K-aligned. __get_free_page should be fine, I think. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
