[PATCH v2 0/3] fix bugs in mobility RTAS calls

[PATCH v2 0/3] fix bugs in mobility RTAS calls

[Cyril Bur]

This patchset addresses endian issues and bugs in device tree update for
ibm,update-nodes and ibm,update-properties RTAS calls.

A subseqent patchset will deal with issues in device tree node addition
(ibm,configure-connector RTAS call) as well as more robust handling of
deleting critical device tree nodes.

v1 attempted to keep the structure of the existing code.
v2 rewrites the relevent sections of mobility.c.

Cyril Bur (3):
  drivers/of: add of_changeset_apply_locked
  powerpc/pseries: create rtas buffer accessor
  powerpc/pseries: fix bugs in mobility RTAS calls

 arch/powerpc/platforms/pseries/Makefile      |   4 +-
 arch/powerpc/platforms/pseries/mobility.c    | 406 ++++++++++++++++-----------
 arch/powerpc/platforms/pseries/pseries.h     |  41 +++
 arch/powerpc/platforms/pseries/rtas_buffer.c | 126 +++++++++
 drivers/of/dynamic.c                         |  11 +
 include/linux/of.h                           |   1 +
 6 files changed, 425 insertions(+), 164 deletions(-)
 create mode 100644 arch/powerpc/platforms/pseries/rtas_buffer.c

-- 
1.9.1

[PATCH v2 1/3] drivers/of: add of_changeset_apply_locked

[Cyril Bur]

Due to the requirement of of_changesets that of_changeset_apply be called
holding the of_mutex and that the of_mutex cannot be accessed nicely outside
the of code, added a wrapper which grabs the lock and called
of_changeset_apply.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 drivers/of/dynamic.c | 11 +++++++++++
 include/linux/of.h   |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 54fecc4..cbff2a2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -542,6 +542,17 @@ void of_changeset_destroy(struct of_changeset *ocs)
 		__of_changeset_entry_destroy(ce);
 }
 
+int of_changeset_apply_locked(struct of_changeset *ocs)
+{
+	int ret;
+
+	mutex_lock(&of_mutex);
+	ret = of_changeset_apply(ocs);
+	mutex_unlock(&of_mutex);
+
+	return ret;
+}
+
 /**
  * of_changeset_apply - Applies a changeset
  *
diff --git a/include/linux/of.h b/include/linux/of.h
index 6c4363b..f5c48fa 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -827,6 +827,7 @@ struct of_changeset {
 extern void of_changeset_init(struct of_changeset *ocs);
 extern void of_changeset_destroy(struct of_changeset *ocs);
 extern int of_changeset_apply(struct of_changeset *ocs);
+extern int of_changeset_apply_locked(struct of_changeset *ocs);
 extern int of_changeset_revert(struct of_changeset *ocs);
 extern int of_changeset_action(struct of_changeset *ocs,
 		unsigned long action, struct device_node *np,
-- 
1.9.1

Re: [PATCH v2 1/3] drivers/of: add of_changeset_apply_locked

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

Hi Cyril,

Nit:

On Thu, 25 Sep 2014 16:41:28 +1000 Cyril Bur <cyril.bur@au1.ibm.com> wrote:
>
> Due to the requirement of of_changesets that of_changeset_apply be called
> holding the of_mutex and that the of_mutex cannot be accessed nicely outside
> the of code, added a wrapper which grabs the lock and called
                                                        ^^^^^^
calls

> of_changeset_apply.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] drivers/of: add of_changeset_apply_locked

[Tyrel Datwyler]

On 09/24/2014 11:41 PM, Cyril Bur wrote:
> Due to the requirement of of_changesets that of_changeset_apply be called
> holding the of_mutex and that the of_mutex cannot be accessed nicely outside
> the of code, added a wrapper which grabs the lock and called
> of_changeset_apply.
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  drivers/of/dynamic.c | 11 +++++++++++
>  include/linux/of.h   |  1 +
>  2 files changed, 12 insertions(+)

This patch needs to go through Grant Likely and the device tree list.
When unsure scripts/get_maintainer.pl can help point the way.

-Tyrel

> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 54fecc4..cbff2a2 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -542,6 +542,17 @@ void of_changeset_destroy(struct of_changeset *ocs)
>  		__of_changeset_entry_destroy(ce);
>  }
>  
> +int of_changeset_apply_locked(struct of_changeset *ocs)
> +{
> +	int ret;
> +
> +	mutex_lock(&of_mutex);
> +	ret = of_changeset_apply(ocs);
> +	mutex_unlock(&of_mutex);
> +
> +	return ret;
> +}
> +
>  /**
>   * of_changeset_apply - Applies a changeset
>   *
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 6c4363b..f5c48fa 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -827,6 +827,7 @@ struct of_changeset {
>  extern void of_changeset_init(struct of_changeset *ocs);
>  extern void of_changeset_destroy(struct of_changeset *ocs);
>  extern int of_changeset_apply(struct of_changeset *ocs);
> +extern int of_changeset_apply_locked(struct of_changeset *ocs);
>  extern int of_changeset_revert(struct of_changeset *ocs);
>  extern int of_changeset_action(struct of_changeset *ocs,
>  		unsigned long action, struct device_node *np,
> 

[PATCH v2 2/3] powerpc/pseries: create rtas buffer accessor

[Cyril Bur]

Added simple accessor functions for rtas in memory buffers which performs
accesses of appropriate type and performs endian conversions.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 arch/powerpc/platforms/pseries/Makefile      |   4 +-
 arch/powerpc/platforms/pseries/pseries.h     |  41 +++++++++
 arch/powerpc/platforms/pseries/rtas_buffer.c | 126 +++++++++++++++++++++++++++
 3 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/platforms/pseries/rtas_buffer.c

diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile
index 0348079..7eb7c46 100644
--- a/arch/powerpc/platforms/pseries/Makefile
+++ b/arch/powerpc/platforms/pseries/Makefile
@@ -3,7 +3,9 @@ ccflags-$(CONFIG_PPC_PSERIES_DEBUG)	+= -DDEBUG
 
 obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
 			   setup.o iommu.o event_sources.o ras.o \
-			   firmware.o power.o dlpar.o mobility.o rng.o
+			   firmware.o power.o dlpar.o mobility.o \
+			   rng.o rtas_buffer.o
+
 obj-$(CONFIG_SMP)	+= smp.o
 obj-$(CONFIG_SCANLOG)	+= scanlog.o
 obj-$(CONFIG_EEH)	+= eeh_pseries.o
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 361add6..f24e352 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -66,4 +66,45 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge);
 
 unsigned long pseries_memory_block_size(void);
 
+/* Manipulation of the in memory data returned from an RTAS call */
+
+/* Data pointed to by ptr is in big endian */
+struct rtas_buffer {
+	void *ptr;
+	int len;
+	int pos;
+};
+
+/* Buffer is already zeroed */
+int make_rtas_buf(struct rtas_buffer *b, size_t size);
+void free_rtas_buf(struct rtas_buffer *b);
+
+/* Return pointer to the buffer being used */
+void *get_rtas_buf(struct rtas_buffer *b);
+
+/* Checks if the buffer exists and the read position is less than the length*/
+bool check_rtas_buf(struct rtas_buffer *b);
+size_t get_rtas_buf_size(struct rtas_buffer *b);
+
+/* Advance the internal position of the buffer by size bytes */
+bool advance_rtas_buf(struct rtas_buffer *b, size_t size);
+
+/* Put a value val into the buffer at position pos. Function expect val in cpu
+ * endian. Returns true if the write to the buffer was successful.
+ */
+bool put_rtas_buf_32(struct rtas_buffer *b, u32 val, int pos);
+
+/* Grab the byte at the current position of the buffer without incrementing
+ * the internal position of the buffer */
+bool peek_rtas_buf(struct rtas_buffer *b, u8 *c);
+
+/* Accessor functions return true if access succeeded and value is written to
+ * val in cpu endian. Automatically advances its reference into the buffer by
+ * the requested amount.
+ */
+bool get_rtas_buf_32(struct rtas_buffer *b, u32 *val);
+bool get_rtas_buf_64(struct rtas_buffer *b, u64 *val);
+bool get_rtas_buf_mem(struct rtas_buffer *b, void **p, size_t len);
+bool get_rtas_buf_str(struct rtas_buffer *b, char **s);
+
 #endif /* _PSERIES_PSERIES_H */
diff --git a/arch/powerpc/platforms/pseries/rtas_buffer.c b/arch/powerpc/platforms/pseries/rtas_buffer.c
new file mode 100644
index 0000000..f06b73c
--- /dev/null
+++ b/arch/powerpc/platforms/pseries/rtas_buffer.c
@@ -0,0 +1,126 @@
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+#include "pseries.h"
+
+
+int make_rtas_buf(struct rtas_buffer *b, size_t sz)
+{
+	b->ptr = kzalloc(sz, GFP_KERNEL);
+	b->len = sz;
+	b->pos = 0;
+	return (!b->ptr) ? -ENOMEM : 0;
+}
+
+void free_rtas_buf(struct rtas_buffer *b)
+{
+	kfree(b->ptr);
+}
+
+void *get_rtas_buf(struct rtas_buffer *b)
+{
+	return (b) ? b->ptr : NULL;
+}
+
+size_t get_rtas_buf_size(struct rtas_buffer *b)
+{
+	return (b) ? b->len : 0;
+}
+
+bool check_rtas_buf(struct rtas_buffer *b)
+{
+	return (b && b->ptr && b->pos < b->len);
+}
+
+static inline void *buf_pos(struct rtas_buffer *b)
+{
+	return (b && b->ptr) ? b->ptr + b->pos : NULL;
+}
+
+bool peek_rtas_buf(struct rtas_buffer *b, u8 *c)
+{
+	if (!b || !c || b->pos >= b->len)
+		return false;
+
+	*c = *(u8 *)buf_pos(b);
+
+	return true;
+}
+
+bool put_rtas_buf_32(struct rtas_buffer *b, u32 val, int pos)
+{
+	if (!b || b->pos >= b->len)
+		return false;
+
+	*((__be32 *)buf_pos(b)) = cpu_to_be32(val);
+
+	return true;
+}
+
+bool get_rtas_buf_32(struct rtas_buffer *b, u32 *val)
+{
+	if (!b || !val || b->len - b->pos < sizeof(u32))
+		return false;
+
+	*val = be32_to_cpu(*((__be32 *)buf_pos(b)));
+	b->pos += sizeof(u32);
+	return true;
+}
+
+bool get_rtas_buf_64(struct rtas_buffer *b, u64 *val)
+{
+	if (!b || !val || b->len - b->pos < sizeof(u64))
+		return false;
+
+	*val = be64_to_cpu(*((__be64 *)buf_pos(b)));
+	b->pos += sizeof(u64);
+	return true;
+}
+
+bool get_rtas_buf_str(struct rtas_buffer *b, char **s)
+{
+	int i;
+	if (!b || !s)
+		return false;
+
+	/* Get length of string */
+	i = b->pos;
+	while (i < b->len) {
+		if (*(char *)(b->ptr + i) == '\0') {
+			*s = (char *)buf_pos(b);
+			if (!*s)
+				return false;
+
+			b->pos = i + 1;
+			return true;
+		}
+		i++;
+	}
+
+	return false;
+}
+
+bool get_rtas_buf_mem(struct rtas_buffer *b, void **p, size_t len)
+{
+	if (!b || !p || b->len - b->pos < len)
+		return false;
+
+	*p = buf_pos(b);
+	if (!*p)
+		return false;
+
+	b->pos += len;
+
+	return true;
+}
+
+bool advance_rtas_buf(struct rtas_buffer *b, size_t len)
+{
+	if (!b || b->len - b->pos < len)
+		return false;
+
+	b->pos += len;
+
+	return true;
+}
+
-- 
1.9.1

Re: [PATCH v2 2/3] powerpc/pseries: create rtas buffer accessor

[Tyrel Datwyler]

On 09/24/2014 11:41 PM, Cyril Bur wrote:
> Added simple accessor functions for rtas in memory buffers which performs
> accesses of appropriate type and performs endian conversions.
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/Makefile      |   4 +-
>  arch/powerpc/platforms/pseries/pseries.h     |  41 +++++++++
>  arch/powerpc/platforms/pseries/rtas_buffer.c | 126 +++++++++++++++++++++++++++
>  3 files changed, 170 insertions(+), 1 deletion(-)
>  create mode 100644 arch/powerpc/platforms/pseries/rtas_buffer.c

Maybe Michael has an opinion here, but seems to me since this is all
RTAS related it would make sense for this code to belong in
kernel/rtas.c and include/asm/rtas.h.

-Tyrel

> 
> diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile
> index 0348079..7eb7c46 100644
> --- a/arch/powerpc/platforms/pseries/Makefile
> +++ b/arch/powerpc/platforms/pseries/Makefile
> @@ -3,7 +3,9 @@ ccflags-$(CONFIG_PPC_PSERIES_DEBUG)	+= -DDEBUG
>  
>  obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
>  			   setup.o iommu.o event_sources.o ras.o \
> -			   firmware.o power.o dlpar.o mobility.o rng.o
> +			   firmware.o power.o dlpar.o mobility.o \
> +			   rng.o rtas_buffer.o
> +
>  obj-$(CONFIG_SMP)	+= smp.o
>  obj-$(CONFIG_SCANLOG)	+= scanlog.o
>  obj-$(CONFIG_EEH)	+= eeh_pseries.o
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index 361add6..f24e352 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -66,4 +66,45 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge);
>  
>  unsigned long pseries_memory_block_size(void);
>  
> +/* Manipulation of the in memory data returned from an RTAS call */
> +
> +/* Data pointed to by ptr is in big endian */
> +struct rtas_buffer {
> +	void *ptr;
> +	int len;
> +	int pos;
> +};
> +
> +/* Buffer is already zeroed */
> +int make_rtas_buf(struct rtas_buffer *b, size_t size);
> +void free_rtas_buf(struct rtas_buffer *b);
> +
> +/* Return pointer to the buffer being used */
> +void *get_rtas_buf(struct rtas_buffer *b);
> +
> +/* Checks if the buffer exists and the read position is less than the length*/
> +bool check_rtas_buf(struct rtas_buffer *b);
> +size_t get_rtas_buf_size(struct rtas_buffer *b);
> +
> +/* Advance the internal position of the buffer by size bytes */
> +bool advance_rtas_buf(struct rtas_buffer *b, size_t size);
> +
> +/* Put a value val into the buffer at position pos. Function expect val in cpu
> + * endian. Returns true if the write to the buffer was successful.
> + */
> +bool put_rtas_buf_32(struct rtas_buffer *b, u32 val, int pos);
> +
> +/* Grab the byte at the current position of the buffer without incrementing
> + * the internal position of the buffer */
> +bool peek_rtas_buf(struct rtas_buffer *b, u8 *c);
> +
> +/* Accessor functions return true if access succeeded and value is written to
> + * val in cpu endian. Automatically advances its reference into the buffer by
> + * the requested amount.
> + */
> +bool get_rtas_buf_32(struct rtas_buffer *b, u32 *val);
> +bool get_rtas_buf_64(struct rtas_buffer *b, u64 *val);
> +bool get_rtas_buf_mem(struct rtas_buffer *b, void **p, size_t len);
> +bool get_rtas_buf_str(struct rtas_buffer *b, char **s);
> +
>  #endif /* _PSERIES_PSERIES_H */
> diff --git a/arch/powerpc/platforms/pseries/rtas_buffer.c b/arch/powerpc/platforms/pseries/rtas_buffer.c
> new file mode 100644
> index 0000000..f06b73c
> --- /dev/null
> +++ b/arch/powerpc/platforms/pseries/rtas_buffer.c
> @@ -0,0 +1,126 @@
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +#include "pseries.h"
> +
> +
> +int make_rtas_buf(struct rtas_buffer *b, size_t sz)
> +{
> +	b->ptr = kzalloc(sz, GFP_KERNEL);
> +	b->len = sz;
> +	b->pos = 0;
> +	return (!b->ptr) ? -ENOMEM : 0;
> +}
> +
> +void free_rtas_buf(struct rtas_buffer *b)
> +{
> +	kfree(b->ptr);
> +}
> +
> +void *get_rtas_buf(struct rtas_buffer *b)
> +{
> +	return (b) ? b->ptr : NULL;
> +}
> +
> +size_t get_rtas_buf_size(struct rtas_buffer *b)
> +{
> +	return (b) ? b->len : 0;
> +}
> +
> +bool check_rtas_buf(struct rtas_buffer *b)
> +{
> +	return (b && b->ptr && b->pos < b->len);
> +}
> +
> +static inline void *buf_pos(struct rtas_buffer *b)
> +{
> +	return (b && b->ptr) ? b->ptr + b->pos : NULL;
> +}
> +
> +bool peek_rtas_buf(struct rtas_buffer *b, u8 *c)
> +{
> +	if (!b || !c || b->pos >= b->len)
> +		return false;
> +
> +	*c = *(u8 *)buf_pos(b);
> +
> +	return true;
> +}
> +
> +bool put_rtas_buf_32(struct rtas_buffer *b, u32 val, int pos)
> +{
> +	if (!b || b->pos >= b->len)
> +		return false;
> +
> +	*((__be32 *)buf_pos(b)) = cpu_to_be32(val);
> +
> +	return true;
> +}
> +
> +bool get_rtas_buf_32(struct rtas_buffer *b, u32 *val)
> +{
> +	if (!b || !val || b->len - b->pos < sizeof(u32))
> +		return false;
> +
> +	*val = be32_to_cpu(*((__be32 *)buf_pos(b)));
> +	b->pos += sizeof(u32);
> +	return true;
> +}
> +
> +bool get_rtas_buf_64(struct rtas_buffer *b, u64 *val)
> +{
> +	if (!b || !val || b->len - b->pos < sizeof(u64))
> +		return false;
> +
> +	*val = be64_to_cpu(*((__be64 *)buf_pos(b)));
> +	b->pos += sizeof(u64);
> +	return true;
> +}
> +
> +bool get_rtas_buf_str(struct rtas_buffer *b, char **s)
> +{
> +	int i;
> +	if (!b || !s)
> +		return false;
> +
> +	/* Get length of string */
> +	i = b->pos;
> +	while (i < b->len) {
> +		if (*(char *)(b->ptr + i) == '\0') {
> +			*s = (char *)buf_pos(b);
> +			if (!*s)
> +				return false;
> +
> +			b->pos = i + 1;
> +			return true;
> +		}
> +		i++;
> +	}
> +
> +	return false;
> +}
> +
> +bool get_rtas_buf_mem(struct rtas_buffer *b, void **p, size_t len)
> +{
> +	if (!b || !p || b->len - b->pos < len)
> +		return false;
> +
> +	*p = buf_pos(b);
> +	if (!*p)
> +		return false;
> +
> +	b->pos += len;
> +
> +	return true;
> +}
> +
> +bool advance_rtas_buf(struct rtas_buffer *b, size_t len)
> +{
> +	if (!b || b->len - b->pos < len)
> +		return false;
> +
> +	b->pos += len;
> +
> +	return true;
> +}
> +
> 

Re: [PATCH v2 2/3] powerpc/pseries: create rtas buffer accessor

[Cyril Bur]

On Thu, 2014-09-25 at 15:47 -0700, Tyrel Datwyler wrote:
> On 09/24/2014 11:41 PM, Cyril Bur wrote:
> > Added simple accessor functions for rtas in memory buffers which performs
> > accesses of appropriate type and performs endian conversions.
> > 
> > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> > ---
> >  arch/powerpc/platforms/pseries/Makefile      |   4 +-
> >  arch/powerpc/platforms/pseries/pseries.h     |  41 +++++++++
> >  arch/powerpc/platforms/pseries/rtas_buffer.c | 126 +++++++++++++++++++++++++++
> >  3 files changed, 170 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/powerpc/platforms/pseries/rtas_buffer.c
> 
> Maybe Michael has an opinion here, but seems to me since this is all
> RTAS related it would make sense for this code to belong in
> kernel/rtas.c and include/asm/rtas.h.
> 
Could quite possibly go there, I wanted to get the code looked at first
but I'll take a look if it could be used more generally and if so move.

Thanks

> -Tyrel
> 
> > 
> > diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile
> > index 0348079..7eb7c46 100644
> > --- a/arch/powerpc/platforms/pseries/Makefile
> > +++ b/arch/powerpc/platforms/pseries/Makefile
> > @@ -3,7 +3,9 @@ ccflags-$(CONFIG_PPC_PSERIES_DEBUG)	+= -DDEBUG
> >  
> >  obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
> >  			   setup.o iommu.o event_sources.o ras.o \
> > -			   firmware.o power.o dlpar.o mobility.o rng.o
> > +			   firmware.o power.o dlpar.o mobility.o \
> > +			   rng.o rtas_buffer.o
> > +
> >  obj-$(CONFIG_SMP)	+= smp.o
> >  obj-$(CONFIG_SCANLOG)	+= scanlog.o
> >  obj-$(CONFIG_EEH)	+= eeh_pseries.o
> > diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> > index 361add6..f24e352 100644
> > --- a/arch/powerpc/platforms/pseries/pseries.h
> > +++ b/arch/powerpc/platforms/pseries/pseries.h
> > @@ -66,4 +66,45 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge);
> >  
> >  unsigned long pseries_memory_block_size(void);
> >  
> > +/* Manipulation of the in memory data returned from an RTAS call */
> > +
> > +/* Data pointed to by ptr is in big endian */
> > +struct rtas_buffer {
> > +	void *ptr;
> > +	int len;
> > +	int pos;
> > +};
> > +
> > +/* Buffer is already zeroed */
> > +int make_rtas_buf(struct rtas_buffer *b, size_t size);
> > +void free_rtas_buf(struct rtas_buffer *b);
> > +
> > +/* Return pointer to the buffer being used */
> > +void *get_rtas_buf(struct rtas_buffer *b);
> > +
> > +/* Checks if the buffer exists and the read position is less than the length*/
> > +bool check_rtas_buf(struct rtas_buffer *b);
> > +size_t get_rtas_buf_size(struct rtas_buffer *b);
> > +
> > +/* Advance the internal position of the buffer by size bytes */
> > +bool advance_rtas_buf(struct rtas_buffer *b, size_t size);
> > +
> > +/* Put a value val into the buffer at position pos. Function expect val in cpu
> > + * endian. Returns true if the write to the buffer was successful.
> > + */
> > +bool put_rtas_buf_32(struct rtas_buffer *b, u32 val, int pos);
> > +
> > +/* Grab the byte at the current position of the buffer without incrementing
> > + * the internal position of the buffer */
> > +bool peek_rtas_buf(struct rtas_buffer *b, u8 *c);
> > +
> > +/* Accessor functions return true if access succeeded and value is written to
> > + * val in cpu endian. Automatically advances its reference into the buffer by
> > + * the requested amount.
> > + */
> > +bool get_rtas_buf_32(struct rtas_buffer *b, u32 *val);
> > +bool get_rtas_buf_64(struct rtas_buffer *b, u64 *val);
> > +bool get_rtas_buf_mem(struct rtas_buffer *b, void **p, size_t len);
> > +bool get_rtas_buf_str(struct rtas_buffer *b, char **s);
> > +
> >  #endif /* _PSERIES_PSERIES_H */
> > diff --git a/arch/powerpc/platforms/pseries/rtas_buffer.c b/arch/powerpc/platforms/pseries/rtas_buffer.c
> > new file mode 100644
> > index 0000000..f06b73c
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/pseries/rtas_buffer.c
> > @@ -0,0 +1,126 @@
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +
> > +#include "pseries.h"
> > +
> > +
> > +int make_rtas_buf(struct rtas_buffer *b, size_t sz)
> > +{
> > +	b->ptr = kzalloc(sz, GFP_KERNEL);
> > +	b->len = sz;
> > +	b->pos = 0;
> > +	return (!b->ptr) ? -ENOMEM : 0;
> > +}
> > +
> > +void free_rtas_buf(struct rtas_buffer *b)
> > +{
> > +	kfree(b->ptr);
> > +}
> > +
> > +void *get_rtas_buf(struct rtas_buffer *b)
> > +{
> > +	return (b) ? b->ptr : NULL;
> > +}
> > +
> > +size_t get_rtas_buf_size(struct rtas_buffer *b)
> > +{
> > +	return (b) ? b->len : 0;
> > +}
> > +
> > +bool check_rtas_buf(struct rtas_buffer *b)
> > +{
> > +	return (b && b->ptr && b->pos < b->len);
> > +}
> > +
> > +static inline void *buf_pos(struct rtas_buffer *b)
> > +{
> > +	return (b && b->ptr) ? b->ptr + b->pos : NULL;
> > +}
> > +
> > +bool peek_rtas_buf(struct rtas_buffer *b, u8 *c)
> > +{
> > +	if (!b || !c || b->pos >= b->len)
> > +		return false;
> > +
> > +	*c = *(u8 *)buf_pos(b);
> > +
> > +	return true;
> > +}
> > +
> > +bool put_rtas_buf_32(struct rtas_buffer *b, u32 val, int pos)
> > +{
> > +	if (!b || b->pos >= b->len)
> > +		return false;
> > +
> > +	*((__be32 *)buf_pos(b)) = cpu_to_be32(val);
> > +
> > +	return true;
> > +}
> > +
> > +bool get_rtas_buf_32(struct rtas_buffer *b, u32 *val)
> > +{
> > +	if (!b || !val || b->len - b->pos < sizeof(u32))
> > +		return false;
> > +
> > +	*val = be32_to_cpu(*((__be32 *)buf_pos(b)));
> > +	b->pos += sizeof(u32);
> > +	return true;
> > +}
> > +
> > +bool get_rtas_buf_64(struct rtas_buffer *b, u64 *val)
> > +{
> > +	if (!b || !val || b->len - b->pos < sizeof(u64))
> > +		return false;
> > +
> > +	*val = be64_to_cpu(*((__be64 *)buf_pos(b)));
> > +	b->pos += sizeof(u64);
> > +	return true;
> > +}
> > +
> > +bool get_rtas_buf_str(struct rtas_buffer *b, char **s)
> > +{
> > +	int i;
> > +	if (!b || !s)
> > +		return false;
> > +
> > +	/* Get length of string */
> > +	i = b->pos;
> > +	while (i < b->len) {
> > +		if (*(char *)(b->ptr + i) == '\0') {
> > +			*s = (char *)buf_pos(b);
> > +			if (!*s)
> > +				return false;
> > +
> > +			b->pos = i + 1;
> > +			return true;
> > +		}
> > +		i++;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +bool get_rtas_buf_mem(struct rtas_buffer *b, void **p, size_t len)
> > +{
> > +	if (!b || !p || b->len - b->pos < len)
> > +		return false;
> > +
> > +	*p = buf_pos(b);
> > +	if (!*p)
> > +		return false;
> > +
> > +	b->pos += len;
> > +
> > +	return true;
> > +}
> > +
> > +bool advance_rtas_buf(struct rtas_buffer *b, size_t len)
> > +{
> > +	if (!b || b->len - b->pos < len)
> > +		return false;
> > +
> > +	b->pos += len;
> > +
> > +	return true;
> > +}
> > +
> > 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH v2 3/3] powerpc/pseries: fix bugs in mobility RTAS calls

[Cyril Bur]

These calls use a shared memory buffer to communicate device tree updates and
PAPR specifies that RTAS buffers are to be written in big endian.  Used the
rtas buffer accessor to help solve both endian problems and standard buffer
access problems.

It seems more sane to stop trying to parse a buffer on errors. Attempting to
continue opens up the possibility of overflows and/or garbage reads. Used
of_changesets throughout to avoid leaving the device tree in an inconsitent
state on error.

Don't warn about failed allcations when the amount was taken from the buffer,
assume the value was incorrect, don't needlessly concern the user.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 406 ++++++++++++++++++------------
 1 file changed, 243 insertions(+), 163 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index e7cb6d4..37d16da 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -20,16 +20,11 @@
 
 #include <asm/machdep.h>
 #include <asm/rtas.h>
+
 #include "pseries.h"
 
-static struct kobject *mobility_kobj;
 
-struct update_props_workarea {
-	u32 phandle;
-	u32 state;
-	u64 reserved;
-	u32 nprops;
-} __packed;
+static struct kobject *mobility_kobj;
 
 #define NODE_ACTION_MASK	0xff000000
 #define NODE_COUNT_MASK		0x00ffffff
@@ -40,7 +35,7 @@ struct update_props_workarea {
 
 #define MIGRATION_SCOPE	(1)
 
-static int mobility_rtas_call(int token, char *buf, s32 scope)
+static int mobility_rtas_call(int token, void *buf, s32 scope)
 {
 	int rc;
 
@@ -54,171 +49,193 @@ static int mobility_rtas_call(int token, char *buf, s32 scope)
 	return rc;
 }
 
-static int delete_dt_node(u32 phandle)
+static int create_property(struct rtas_buffer *rtas_buf, struct property **prop,
+                           const char *name, int length)
 {
-	struct device_node *dn;
-
-	dn = of_find_node_by_phandle(phandle);
-	if (!dn)
-		return -ENOENT;
+	void *prop_value;
+	void *new_value;
 
-	dlpar_detach_node(dn);
-	of_node_put(dn);
-	return 0;
-}
+	struct property *working_prop = *prop;
 
-static int update_dt_property(struct device_node *dn, struct property **prop,
-			      const char *name, u32 vd, char *value)
-{
-	struct property *new_prop = *prop;
-	int more = 0;
+	if (!get_rtas_buf_mem(rtas_buf, &prop_value, length))
+		return -EOVERFLOW;
 
-	/* A negative 'vd' value indicates that only part of the new property
-	 * value is contained in the buffer and we need to call
-	 * ibm,update-properties again to get the rest of the value.
-	 *
-	 * A negative value is also the two's compliment of the actual value.
-	 */
-	if (vd & 0x80000000) {
-		vd = ~vd + 1;
-		more = 1;
-	}
-
-	if (new_prop) {
-		/* partial property fixup */
-		char *new_data = kzalloc(new_prop->length + vd, GFP_KERNEL);
-		if (!new_data)
-			return -ENOMEM;
-
-		memcpy(new_data, new_prop->value, new_prop->length);
-		memcpy(new_data + new_prop->length, value, vd);
-
-		kfree(new_prop->value);
-		new_prop->value = new_data;
-		new_prop->length += vd;
-	} else {
-		new_prop = kzalloc(sizeof(*new_prop), GFP_KERNEL);
-		if (!new_prop)
-			return -ENOMEM;
-
-		new_prop->name = kstrdup(name, GFP_KERNEL);
-		if (!new_prop->name) {
-			kfree(new_prop);
+	if (!working_prop) {
+		working_prop = kzalloc(sizeof(*working_prop), GFP_KERNEL);
+		if (!working_prop) {
 			return -ENOMEM;
 		}
 
-		new_prop->length = vd;
-		new_prop->value = kzalloc(new_prop->length, GFP_KERNEL);
-		if (!new_prop->value) {
-			kfree(new_prop->name);
-			kfree(new_prop);
+		working_prop->name = kstrdup(name, GFP_KERNEL | __GFP_NOWARN);
+		if (!working_prop->name) {
+			kfree(working_prop);
 			return -ENOMEM;
 		}
-
-		memcpy(new_prop->value, value, vd);
-		*prop = new_prop;
+		*prop = working_prop;
 	}
 
-	if (!more) {
-		of_update_property(dn, new_prop);
+	new_value = krealloc(working_prop->value, working_prop->length + length, GFP_KERNEL | __GFP_NOWARN);
+	if (!new_value) {
+		kfree(working_prop->value);
+		kfree(working_prop->name);
+		kfree(working_prop);
 		*prop = NULL;
+		return -ENOMEM;
 	}
 
+	working_prop->value = new_value;
+	memcpy(working_prop->value + working_prop->length, prop_value, length);
+	working_prop->length += length;
+
 	return 0;
 }
 
-static int update_dt_node(u32 phandle, s32 scope)
+static int parse_ibm_update_properties(struct rtas_buffer *rtas_buf, struct of_changeset *cs,
+                                       struct device_node *dn, struct property **r_prop)
 {
-	struct update_props_workarea *upwa;
-	struct device_node *dn;
+	int i;
+	int rc = 0;
+
 	struct property *prop = NULL;
-	int i, rc, rtas_rc;
-	char *prop_data;
-	char *rtas_buf;
-	int update_properties_token;
+
+	char peek;
+	u32 nprops;
 	u32 vd;
 
-	update_properties_token = rtas_token("ibm,update-properties");
-	if (update_properties_token == RTAS_UNKNOWN_SERVICE)
+	if (!rtas_buf || !cs || !dn || !r_prop)
 		return -EINVAL;
 
-	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
-		return -ENOMEM;
+	/* Advance the buffer by 16 bytes which contain the phandle (4 bytes),
+	 * 4 bytes of state variable and 8 bytes of reserved (zero) bytes.
+	 * As the state variable must be sent on subsequent RTAS calls ensure
+	 * don't touch it so the caller can reuse this buffer.
+	 */
+	if (!advance_rtas_buf(rtas_buf, 16) || !get_rtas_buf_32(rtas_buf, &nprops))
+		return -EOVERFLOW;
 
-	dn = of_find_node_by_phandle(phandle);
-	if (!dn) {
-		kfree(rtas_buf);
-		return -ENOENT;
+	/* On the first call to ibm,update-properties for a node the
+	 * the first property value descriptor contains an empty
+	 * property name, the property value length encoded as u32,
+	 * and the property value is the node path being updated.
+	 *
+	 * Discard this information as the node was looked up by its phandle and
+	 * passed in.
+	 */
+	if (!peek_rtas_buf(rtas_buf, &peek))
+		return -EOVERFLOW;
+	if (peek == '\0') {
+		if (!advance_rtas_buf(rtas_buf, sizeof(u32)) ||
+		    !get_rtas_buf_32(rtas_buf, &vd) ||
+		    !advance_rtas_buf(rtas_buf, vd))
+			return -EOVERFLOW;
+		nprops--;
 	}
 
-	upwa = (struct update_props_workarea *)&rtas_buf[0];
-	upwa->phandle = phandle;
+	for (i = 0; i < nprops; i++) {
+		char *prop_name = NULL;
+		int prop_len;
 
-	do {
-		rtas_rc = mobility_rtas_call(update_properties_token, rtas_buf,
-					scope);
-		if (rtas_rc < 0)
+		if (!get_rtas_buf_str(rtas_buf, &prop_name) ||
+		    !get_rtas_buf_32(rtas_buf, &vd))
+				return -EOVERFLOW;
+
+		switch (vd) {
+		case 0x00000000:
+			/* name only property, nothing to do */
 			break;
 
-		prop_data = rtas_buf + sizeof(*upwa);
-
-		/* On the first call to ibm,update-properties for a node the
-		 * the first property value descriptor contains an empty
-		 * property name, the property value length encoded as u32,
-		 * and the property value is the node path being updated.
-		 */
-		if (*prop_data == 0) {
-			prop_data++;
-			vd = *(u32 *)prop_data;
-			prop_data += vd + sizeof(vd);
-			upwa->nprops--;
+		case 0x80000000:
+			prop = of_find_property(dn, prop_name, NULL);
+			of_changeset_remove_property(cs, dn, prop);
+			prop = NULL;
+			break;
+
+		default:
+			/* A negative 'vd' value indicates that only part of the new property
+			 * value is contained in the buffer and we need to call
+			 * ibm,update-properties again to get the rest of the value.
+			 *
+			 * A negative value is also the two's compliment of the actual value.
+			 */
+			prop_len = vd & 0x80000000 ? ~vd + 1 : vd;
+
+			rc = create_property(rtas_buf, &prop, prop_name, prop_len);
+			if (rc) {
+				printk(KERN_ERR "Could not update %s property\n", prop_name);
+				/* Could try to continue but failure in that function likely means
+				 * we have bigger problems.
+				 */
+				return rc;
+			}
+
+			if (prop && !(vd & 0x80000000)) {
+				of_changeset_update_property(cs, dn, prop);
+				prop = NULL;
+			}
 		}
+	}
 
-		for (i = 0; i < upwa->nprops; i++) {
-			char *prop_name;
+	if (prop)
+		*r_prop = prop;
+	return rc;
+}
 
-			prop_name = prop_data;
-			prop_data += strlen(prop_name) + 1;
-			vd = *(u32 *)prop_data;
-			prop_data += sizeof(vd);
+static int pseries_devicetree_update_props(struct of_changeset *cs, struct device_node *dn,
+                                           s32 scope)
+{
+	int rc;
+	int error;
+	int update_properties_token;
 
-			switch (vd) {
-			case 0x00000000:
-				/* name only property, nothing to do */
-				break;
+	void *buf;
+	struct rtas_buffer rtas_buf;
 
-			case 0x80000000:
-				prop = of_find_property(dn, prop_name, NULL);
-				of_remove_property(dn, prop);
-				prop = NULL;
-				break;
+	struct property *prop = NULL;
 
-			default:
-				rc = update_dt_property(dn, &prop, prop_name,
-							vd, prop_data);
-				if (rc) {
-					printk(KERN_ERR "Could not update %s"
-					       " property\n", prop_name);
-				}
+	if (!cs || !dn)
+		return -EINVAL;
 
-				prop_data += vd;
-			}
+	update_properties_token = rtas_token("ibm,update-properties");
+	if (update_properties_token == RTAS_UNKNOWN_SERVICE)
+		return -EINVAL;
+
+	rc = make_rtas_buf(&rtas_buf, RTAS_DATA_BUF_SIZE);
+	if (rc)
+		return -ENOMEM;
+
+	put_rtas_buf_32(&rtas_buf, dn->phandle, 0);
+
+	buf = get_rtas_buf(&rtas_buf);
+	if (!buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	do {
+
+		rc = mobility_rtas_call(update_properties_token, buf, scope);
+		if (rc < 0)
+			goto out;
+
+		error = parse_ibm_update_properties(&rtas_buf, cs, dn, &prop);
+		if (error) {
+			rc = error;
+			goto out;
 		}
-	} while (rtas_rc == 1);
 
-	of_node_put(dn);
-	kfree(rtas_buf);
-	return 0;
+	} while (rc == 1);
+
+out:
+	free_rtas_buf(&rtas_buf);
+	return rc;
 }
 
-static int add_dt_node(u32 parent_phandle, u32 drc_index)
+static int pseries_devicetree_update_node(struct of_changeset *cs, struct device_node *parent_dn,
+                                          u32 drc_index)
 {
 	struct device_node *dn;
-	struct device_node *parent_dn;
 	int rc;
 
-	parent_dn = of_find_node_by_phandle(parent_phandle);
 	if (!parent_dn)
 		return -ENOENT;
 
@@ -230,59 +247,122 @@ static int add_dt_node(u32 parent_phandle, u32 drc_index)
 	if (rc)
 		dlpar_free_cc_nodes(dn);
 
-	of_node_put(parent_dn);
 	return rc;
 }
 
+static int parse_ibm_update_nodes(struct rtas_buffer *rtas_buf, struct of_changeset *cs, s32 scope)
+{
+	u32 node;
+	int rc = 0;
+
+	if (!rtas_buf || !cs)
+		return -EINVAL;
+
+	/* Advance the buffer by 16 bytes which encompases 4 bytes of state
+	 * variable and 12 reserved (zeroed) bytes.
+	 * As the state variable must be sent on subsequent RTAS calls ensure
+	 * don't touch it so the caller can reuse this buffer.
+	 */
+	if (!advance_rtas_buf(rtas_buf, 16) ||
+	    !get_rtas_buf_32(rtas_buf, &node))
+		goto out;
+
+	while (node & NODE_ACTION_MASK) {
+		int i;
+		u32 action = node & NODE_ACTION_MASK;
+		int node_count = node & NODE_COUNT_MASK;
+		struct device_node *dn;
+
+		for (i = 0; i < node_count; i++) {
+			u32 phandle;
+			u32 drc_index;
+
+			if (!get_rtas_buf_32(rtas_buf, &phandle))
+				goto out;
+
+			dn = of_find_node_by_phandle(phandle);
+			if (!dn)
+				return -ENOENT;
+
+			switch (action) {
+			case DELETE_DT_NODE:
+				of_changeset_detach_node(cs, dn);
+				break;
+			case UPDATE_DT_NODE:
+				rc = pseries_devicetree_update_props(cs, dn, scope);
+				break;
+			case ADD_DT_NODE:
+				if (!get_rtas_buf_32(rtas_buf, &drc_index)) {
+					of_node_put(dn);
+					goto out;
+				}
+				rc = pseries_devicetree_update_node(cs, dn, drc_index);
+				break;
+			default:
+				of_node_put(dn);
+				/* Bogus action */
+				return -EINVAL; /* Break */
+			}
+			of_node_put(dn);
+		}
+		if (!get_rtas_buf_32(rtas_buf, &node))
+			goto out;
+	}
+
+	return rc;
+out:
+	printk(KERN_ERR "RTAS buffer length exceeded\n");
+	return -EOVERFLOW;
+}
+
 int pseries_devicetree_update(s32 scope)
 {
-	char *rtas_buf;
-	u32 *data;
+	struct rtas_buffer rtas_buf;
+	struct of_changeset cs;
+	void *buf;
 	int update_nodes_token;
 	int rc;
+	int error;
 
 	update_nodes_token = rtas_token("ibm,update-nodes");
 	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
 		return -EINVAL;
 
-	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
-		return -ENOMEM;
+	of_changeset_init(&cs);
+
+	rc = make_rtas_buf(&rtas_buf, RTAS_DATA_BUF_SIZE);
+	if (rc) {
+		of_changeset_destroy(&cs);
+		return rc;
+	}
+
+	buf = get_rtas_buf(&rtas_buf);
+	if (!buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
 	do {
-		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
+		rc = mobility_rtas_call(update_nodes_token, buf, scope);
 		if (rc && rc != 1)
 			break;
 
-		data = (u32 *)rtas_buf + 4;
-		while (*data & NODE_ACTION_MASK) {
-			int i;
-			u32 action = *data & NODE_ACTION_MASK;
-			int node_count = *data & NODE_COUNT_MASK;
-
-			data++;
-
-			for (i = 0; i < node_count; i++) {
-				u32 phandle = *data++;
-				u32 drc_index;
-
-				switch (action) {
-				case DELETE_DT_NODE:
-					delete_dt_node(phandle);
-					break;
-				case UPDATE_DT_NODE:
-					update_dt_node(phandle, scope);
-					break;
-				case ADD_DT_NODE:
-					drc_index = *data++;
-					add_dt_node(phandle, drc_index);
-					break;
-				}
-			}
-		}
+		error = parse_ibm_update_nodes(&rtas_buf, &cs, scope);
+		if (error)
+			break;
+
 	} while (rc == 1);
 
-	kfree(rtas_buf);
+	if (error)
+		rc = error;
+
+	if (rc == 0)
+		rc = of_changeset_apply_locked(&cs);
+
+out:
+	of_changeset_destroy(&cs);
+	free_rtas_buf(&rtas_buf);
+
 	return rc;
 }
 
-- 
1.9.1