[RFC PATCH 0/9] Fixes and Enablement of ibm,drc-info property

[RFC PATCH 0/9] Fixes and Enablement of ibm,drc-info property

[Tyrel Datwyler]

There was an initial previous effort yo add support for the PAPR
architected ibm,drc-info property. This property provides a more
memory compact representation of a paritions Dynamic Reconfig
Connectors (DRC). These can otherwise be thought of the currently
partitioned, or available, but yet to be partitioned, system resources
such as cpus, memory, and physical/logical IOA devices.

The initial implementation proved buggy and was fully turned of by
disabling the bit in the appropriate CAS support vector. We now have
PowerVM firmware in the field that supports this new property, and 
further to suppport partitions with 24TB+ or possible memory this
property is required to perform platform migration.

This serious fixup the short comings of the previous implementation
in the areas of general implementation, cpu hotplug, and IOA hotplug.

Tyrel Datwyler (9):
  powerpc/pseries: add cpu DLPAR support for drc-info property
  powerpc/pseries: fix bad drc_index_start value parsing of drc-info
    entry
  powerpc/pseries: fix drc-info mappings of logical cpus to drc-index
  PCI: rpaphp: fix up pointer to first drc-info entry
  PCI: rpaphp: don't rely on firmware feature to imply drc-info support
  PCI: rpaphp: add drc-info support for hotplug slot registration
  PCI: rpaphp: annotate and correctly byte swap DRC properties
  PCI: rpaphp: correctly match ibm,my-drc-index to drc-name when using
    drc-info
  powerpc: Enable support for ibm,drc-info property

 arch/powerpc/kernel/prom_init.c                 |   2 +-
 arch/powerpc/platforms/pseries/hotplug-cpu.c    | 117 ++++++++++++++++------
 arch/powerpc/platforms/pseries/of_helpers.c     |   8 +-
 arch/powerpc/platforms/pseries/pseries_energy.c |  23 ++---
 drivers/pci/hotplug/rpaphp_core.c               | 124 +++++++++++++++++-------
 5 files changed, 191 insertions(+), 83 deletions(-)

-- 
2.7.4

[RFC PATCH 1/9] powerpc/pseries: add cpu DLPAR support for drc-info property

[Tyrel Datwyler]

From: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

Older firmwares provided information about Dynamic Reconfig
Connectors (DRC) through several device tree properties, namely
ibm,drc-types, ibm,drc-indexes, ibm,drc-names, and
ibm,drc-power-domains. New firmwares have the ability to present this
same information in a much condensed format through a device tree
property called ibm,drc-info.

The existing cpu DLPAR hotplug code only understands the older DRC
property format when validating the drc-index of a cpu during a
hotplug add. This updates those code paths to use the ibm,drc-info
property, when present, instead for validation.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-cpu.c | 117 ++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index bbda646..a2b6cd1 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -407,25 +407,61 @@ static bool dlpar_cpu_exists(struct device_node *parent, u32 drc_index)
 	return found;
 }
 
-static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+static bool drc_info_valid_index(struct device_node *parent, u32 drc_index)
 {
-	bool found = false;
-	int rc, index;
+	struct property *info;
+	struct of_drc_info drc;
+	const __be32 *value;
+	int count, i, j;
 
-	index = 0;
-	while (!found) {
-		u32 drc;
+	info = of_find_property(parent, "ibm,drc-info", NULL);
+	if (!info)
+		return false;
 
-		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
-						index++, &drc);
-		if (rc)
+	value = of_prop_next_u32(info, NULL, &count);
+
+	/* First value of ibm,drc-info is number of drc-info records */
+	if (value)
+		value++;
+	else
+		return false;
+
+	for (i = 0; i < count; i++) {
+		if (of_read_drc_info_cell(&info, &value, &drc))
+			return false;
+
+		if (strncmp(drc.drc_type, "CPU", 3))
 			break;
 
-		if (drc == drc_index)
-			found = true;
+		if (drc_index > drc.last_drc_index)
+			continue;
+
+		for (j = 0; j < drc.num_sequential_elems; j++)
+			if (drc_index == (drc.drc_index_start + (drc.sequential_inc * j)))
+					return true;
 	}
 
-	return found;
+	return false;
+}
+
+static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+{
+	const __be32 *indexes;
+	int i;
+
+	if (of_find_property(parent, "ibm,drc-info", NULL))
+		return drc_info_valid_index(parent, drc_index);
+
+	indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
+	if (!indexes)
+		return false;
+
+	for (i = 0; i < indexes[0]; i++) {
+		if (be32_to_cpu(indexes[i + 1]) == drc_index)
+			return true;
+	}
+
+	return false;
 }
 
 static ssize_t dlpar_cpu_add(u32 drc_index)
@@ -720,8 +756,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
 static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
 {
 	struct device_node *parent;
+	struct property *info;
+	const __be32 *indexes;
 	int cpus_found = 0;
-	int index, rc;
+	int i, j;
+	u32 drc_index;
 
 	parent = of_find_node_by_path("/cpus");
 	if (!parent) {
@@ -730,24 +769,46 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
 		return -1;
 	}
 
-	/* Search the ibm,drc-indexes array for possible CPU drcs to
-	 * add. Note that the format of the ibm,drc-indexes array is
-	 * the number of entries in the array followed by the array
-	 * of drc values so we start looking at index = 1.
-	 */
-	index = 1;
-	while (cpus_found < cpus_to_add) {
-		u32 drc;
+	info = of_find_property(parent, "ibm,drc-info", NULL);
+	if (info) {
+		struct of_drc_info drc;
+		const __be32 *value;
+		int count;
 
-		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
-						index++, &drc);
-		if (rc)
-			break;
+		value = of_prop_next_u32(info, NULL, &count);
+		if (value)
+			value++;
 
-		if (dlpar_cpu_exists(parent, drc))
-			continue;
+		for (i = 0; i < count; i++) {
+			of_read_drc_info_cell(&info, &value, &drc);
+			if (strncmp(drc.drc_type, "CPU", 3))
+				break;
+
+			for (j = 0; j < drc.num_sequential_elems; j++) {
+				drc_index = drc.drc_index_start + (drc.sequential_inc * j);
+
+				if (dlpar_cpu_exists(parent, drc_index))
+					continue;
 
-		cpu_drcs[cpus_found++] = drc;
+				cpu_drcs[cpus_found++] = drc_index;
+			}
+		}
+	} else {
+		indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
+
+		/* Search the ibm,drc-indexes array for possible CPU drcs to
+	 	* add. Note that the format of the ibm,drc-indexes array is
+	 	* the number of entries in the array followed by the array
+	 	* of drc values so we start looking at index = 1.
+	 	*/
+		for (i = 1; i < indexes[0]; i++) {
+			drc_index = be32_to_cpu(indexes[i]);
+
+			if (dlpar_cpu_exists(parent, drc_index))
+				continue;
+
+			cpu_drcs[cpus_found++] = drc_index;
+		}
 	}
 
 	of_node_put(parent);
-- 
2.7.4

Re: [RFC PATCH 1/9] powerpc/pseries: add cpu DLPAR support for drc-info property

[Nathan Lynch]

Hi Tyrel,

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
> +{
> +	const __be32 *indexes;
> +	int i;
> +
> +	if (of_find_property(parent, "ibm,drc-info", NULL))
> +		return drc_info_valid_index(parent, drc_index);
> +
> +	indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
> +	if (!indexes)
> +		return false;
> +
> +	for (i = 0; i < indexes[0]; i++) {

should this be:

        for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
?


> +		if (be32_to_cpu(indexes[i + 1]) == drc_index)
> +			return true;
> +	}
> +
> +	return false;
>  }

It looks like this rewrites valid_cpu_drc_index()'s existing code for
parsing ibm,drc-indexes but I don't see the need for this.

This patch would be easier to review if that were dropped or split out.

>  
>  static ssize_t dlpar_cpu_add(u32 drc_index)
> @@ -720,8 +756,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>  static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>  {
>  	struct device_node *parent;
> +	struct property *info;
> +	const __be32 *indexes;
>  	int cpus_found = 0;
> -	int index, rc;
> +	int i, j;
> +	u32 drc_index;
>  
>  	parent = of_find_node_by_path("/cpus");
>  	if (!parent) {
> @@ -730,24 +769,46 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>  		return -1;
>  	}
>  
> -	/* Search the ibm,drc-indexes array for possible CPU drcs to
> -	 * add. Note that the format of the ibm,drc-indexes array is
> -	 * the number of entries in the array followed by the array
> -	 * of drc values so we start looking at index = 1.
> -	 */
> -	index = 1;
> -	while (cpus_found < cpus_to_add) {
> -		u32 drc;
> +	info = of_find_property(parent, "ibm,drc-info", NULL);
> +	if (info) {
> +		struct of_drc_info drc;
> +		const __be32 *value;
> +		int count;
>  
> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
> -						index++, &drc);
> -		if (rc)
> -			break;
> +		value = of_prop_next_u32(info, NULL, &count);
> +		if (value)
> +			value++;
>  
> -		if (dlpar_cpu_exists(parent, drc))
> -			continue;
> +		for (i = 0; i < count; i++) {
> +			of_read_drc_info_cell(&info, &value, &drc);
> +			if (strncmp(drc.drc_type, "CPU", 3))
> +				break;
> +
> +			for (j = 0; j < drc.num_sequential_elems; j++) {
> +				drc_index = drc.drc_index_start + (drc.sequential_inc * j);
> +
> +				if (dlpar_cpu_exists(parent, drc_index))
> +					continue;
>  
> -		cpu_drcs[cpus_found++] = drc;
> +				cpu_drcs[cpus_found++] = drc_index;

I am failing to see how this loop is limited by the cpus_to_add
parameter as it was before this change. It looks like this will overflow
the cpu_drcs array when cpus_to_add is less than the number of cpus
found.

As an aside I don't understand how the add_by_count()/dlpar_cpu_exists()
algorithm could be correct as it currently stands. It seems to pick the
first X indexes for which a corresponding cpu node is absent, but that
set of indexes does not necessarily match the set that is available to
configure. Something to address separately I suppose.

> +			}
> +		}
> +	} else {
> +		indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
> +
> +		/* Search the ibm,drc-indexes array for possible CPU drcs to
> +	 	* add. Note that the format of the ibm,drc-indexes array is
> +	 	* the number of entries in the array followed by the array
> +	 	* of drc values so we start looking at index = 1.
> +	 	*/
> +		for (i = 1; i < indexes[0]; i++) {
> +			drc_index = be32_to_cpu(indexes[i]);
> +
> +			if (dlpar_cpu_exists(parent, drc_index))
> +				continue;
> +
> +			cpu_drcs[cpus_found++] = drc_index;
> +		}
>  	}

As above, not sure why this was rewritten, and similar comments as
before apply.

Re: [RFC PATCH 1/9] powerpc/pseries: add cpu DLPAR support for drc-info property

[Tyrel Datwyler]

On 10/10/19 11:56 AM, Nathan Lynch wrote:
> Hi Tyrel,
> 
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> +{
>> +	const __be32 *indexes;
>> +	int i;
>> +
>> +	if (of_find_property(parent, "ibm,drc-info", NULL))
>> +		return drc_info_valid_index(parent, drc_index);
>> +
>> +	indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
>> +	if (!indexes)
>> +		return false;
>> +
>> +	for (i = 0; i < indexes[0]; i++) {
> 
> should this be:
> 
>         for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> ?

Yes!

> 
> 
>> +		if (be32_to_cpu(indexes[i + 1]) == drc_index)
>> +			return true;
>> +	}
>> +
>> +	return false;
>>  }
> 
> It looks like this rewrites valid_cpu_drc_index()'s existing code for
> parsing ibm,drc-indexes but I don't see the need for this.
> 
> This patch would be easier to review if that were dropped or split out.

Yeah, I'll split it out. There are multiple places where we iterate over the
drc_indexes, and it is implemented several different ways. I basically picked an
implementation to use across the board. I think a better way would be just to
implement a for_each_drc_index(dn, drc_index) macro to abstract away iterator
implementation.

> 
>>  
>>  static ssize_t dlpar_cpu_add(u32 drc_index)
>> @@ -720,8 +756,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>>  static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>>  {
>>  	struct device_node *parent;
>> +	struct property *info;
>> +	const __be32 *indexes;
>>  	int cpus_found = 0;
>> -	int index, rc;
>> +	int i, j;
>> +	u32 drc_index;
>>  
>>  	parent = of_find_node_by_path("/cpus");
>>  	if (!parent) {
>> @@ -730,24 +769,46 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>>  		return -1;
>>  	}
>>  
>> -	/* Search the ibm,drc-indexes array for possible CPU drcs to
>> -	 * add. Note that the format of the ibm,drc-indexes array is
>> -	 * the number of entries in the array followed by the array
>> -	 * of drc values so we start looking at index = 1.
>> -	 */
>> -	index = 1;
>> -	while (cpus_found < cpus_to_add) {
>> -		u32 drc;
>> +	info = of_find_property(parent, "ibm,drc-info", NULL);
>> +	if (info) {
>> +		struct of_drc_info drc;
>> +		const __be32 *value;
>> +		int count;
>>  
>> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> -						index++, &drc);
>> -		if (rc)
>> -			break;
>> +		value = of_prop_next_u32(info, NULL, &count);
>> +		if (value)
>> +			value++;
>>  
>> -		if (dlpar_cpu_exists(parent, drc))
>> -			continue;
>> +		for (i = 0; i < count; i++) {
>> +			of_read_drc_info_cell(&info, &value, &drc);
>> +			if (strncmp(drc.drc_type, "CPU", 3))
>> +				break;
>> +
>> +			for (j = 0; j < drc.num_sequential_elems; j++) {
>> +				drc_index = drc.drc_index_start + (drc.sequential_inc * j);
>> +
>> +				if (dlpar_cpu_exists(parent, drc_index))
>> +					continue;
>>  
>> -		cpu_drcs[cpus_found++] = drc;
>> +				cpu_drcs[cpus_found++] = drc_index;
> 
> I am failing to see how this loop is limited by the cpus_to_add
> parameter as it was before this change. It looks like this will overflow
> the cpu_drcs array when cpus_to_add is less than the number of cpus
> found.

You are right. The code is picking every non-present drc_index which will
overflow the supplied buffer as you stated when there are more available indexes
than requested cpus. Will fix to bound the search.

> 
> As an aside I don't understand how the add_by_count()/dlpar_cpu_exists()
> algorithm could be correct as it currently stands. It seems to pick the
> first X indexes for which a corresponding cpu node is absent, but that
> set of indexes does not necessarily match the set that is available to
> configure. Something to address separately I suppose.

I'm not sure I follow?

> 
>> +			}
>> +		}
>> +	} else {
>> +		indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
>> +
>> +		/* Search the ibm,drc-indexes array for possible CPU drcs to
>> +	 	* add. Note that the format of the ibm,drc-indexes array is
>> +	 	* the number of entries in the array followed by the array
>> +	 	* of drc values so we start looking at index = 1.
>> +	 	*/
>> +		for (i = 1; i < indexes[0]; i++) {
>> +			drc_index = be32_to_cpu(indexes[i]);
>> +
>> +			if (dlpar_cpu_exists(parent, drc_index))
>> +				continue;
>> +
>> +			cpu_drcs[cpus_found++] = drc_index;
>> +		}
>>  	}
> 
> As above, not sure why this was rewritten, and similar comments as
> before apply.
> 

Again, wanted to use a single implementation everywere. Obviously, as pointed
out in the previous comment missed a byte swap. Will split out into a separate
patch for consideration.

-Tyrel

Re: [RFC PATCH 1/9] powerpc/pseries: add cpu DLPAR support for drc-info property

[Nathan Lynch]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> On 10/10/19 11:56 AM, Nathan Lynch wrote:
>> As an aside I don't understand how the add_by_count()/dlpar_cpu_exists()
>> algorithm could be correct as it currently stands. It seems to pick the
>> first X indexes for which a corresponding cpu node is absent, but that
>> set of indexes does not necessarily match the set that is available to
>> configure. Something to address separately I suppose.
>
> I'm not sure I follow?

Don't worry about it for this patchset, it's just something I noticed
when reviewing. I'm wondering why the cpu add algorithm doesn't work
more like the one for memory, which actually queries the platform for
connector status via dlpar_acquire_drc(). It's possible I'm
misunderstanding something though. Like I said, I think it's something
to address separately if there is indeed an issue.

[RFC PATCH 2/9] powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry

[Tyrel Datwyler]

The ibm,drc-info property is an array property that contains drc-info
entries such that each entry is made up of 2 string encoded elements
followed by 5 int encoded elements. The of_read_drc_info_cell()
helper contains comments that correctly name the expected elements
and their encoding. However, the usage of of_prop_next_string() and
of_prop_next_u32() introduced a subtle skippage of the first u32.
This is a result of of_prop_next_string() returns a pointer to the
next property value which is not a string, but actually a (__be32 *).
As, a result the following call to of_prop_next_u32() passes over the
current int encoded value and actually stores the next one wrongly.

Simply endian swap the current value in place after reading the first
two string values. The remaining int encoded values can then be read
correctly using of_prop_next_u32().

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/of_helpers.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 6df192f..66dfd82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -45,14 +45,14 @@ struct device_node *pseries_of_derive_parent(const char *path)
 int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
 			struct of_drc_info *data)
 {
-	const char *p;
+	const char *p = (char *)(*curval);
 	const __be32 *p2;
 
 	if (!data)
 		return -EINVAL;
 
 	/* Get drc-type:encode-string */
-	p = data->drc_type = (char*) (*curval);
+	data->drc_type = (char *)p;
 	p = of_prop_next_string(*prop, p);
 	if (!p)
 		return -EINVAL;
@@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
 
 	/* Get drc-index-start:encode-int */
 	p2 = (const __be32 *)p;
-	p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start);
-	if (!p2)
-		return -EINVAL;
+	data->drc_index_start = be32_to_cpu(*p2);
 
 	/* Get drc-name-suffix-start:encode-int */
 	p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start);
-- 
2.7.4

Re: [RFC PATCH 2/9] powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry

[Nathan Lynch]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> The ibm,drc-info property is an array property that contains drc-info
> entries such that each entry is made up of 2 string encoded elements
> followed by 5 int encoded elements. The of_read_drc_info_cell()
> helper contains comments that correctly name the expected elements
> and their encoding. However, the usage of of_prop_next_string() and
> of_prop_next_u32() introduced a subtle skippage of the first u32.
> This is a result of of_prop_next_string() returns a pointer to the
> next property value which is not a string, but actually a (__be32 *).
> As, a result the following call to of_prop_next_u32() passes over the
> current int encoded value and actually stores the next one wrongly.
>
> Simply endian swap the current value in place after reading the first
> two string values. The remaining int encoded values can then be read
> correctly using of_prop_next_u32().

Good but I think it would make more sense for a fix for
of_read_drc_info_cell() to precede any patch in the series which
introduces new callers, such as patch #1.

powerpc/405GP, cuImage and PCI support

[Carlo Pisani]

hi
I wrote here (1) a couple of years ago, I am still working with kernel
4.11.0 and there is broken support for initializing the PCI.

arch/powerpc/book/cuimage-walnut.c requires "/plb" compatible with
"fsl,pq2-localbus", while the device-tree file (walnut.dts) defines
"/plb" compatible with "ibm,plb3"

I am not an expert, but "fsl,pq2-localbus" != "ibm,plb3"

Therefore the PCI initialization of the PPC405GP seems wrong and every
kernel >= 2.6.26 is not able to correctly address the PDC20265

(1) https://bugzilla.kernel.org/show_bug.cgi?id=195933

an interesting not is:
kernel 2.6.26 can be compiled with arch=ppc and arch=powerpc

with arch=ppc the promise PDC20265 chip is correctly managed
with arch=powerpc the PDC20265 is not correctly managed


any idea? help? suggestion?

thanks
Carlo


--------------------------------------------------------------------------------------
    bus_node = finddevice("/plb");
    if (!bus_node)
    {
        notify_error(module, id, "device /plb not found");
        return;
    }
    if (!dt_is_compatible(bus_node, "fsl,pq2-localbus"))
    {
        notify_warn(module, id, "device fsl,pq2-localbus");
        notify_error(module, id, "device /plb is not compatible");
--------------------------------------------------------------------------------------
plb
        {
                /*
                 * Processor Local Bus (PLB)
                 */
                compatible = "ibm,plb3";
--------------------------------------------------------------------------------------


ide0 at 0x1f0-0x1f7,0x3f6 on irq 31
ide1 at 0x170-0x177,0x376 on irq 31

Re: [RFC PATCH 2/9] powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry

[Tyrel Datwyler]

On 10/10/19 12:04 PM, Nathan Lynch wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> The ibm,drc-info property is an array property that contains drc-info
>> entries such that each entry is made up of 2 string encoded elements
>> followed by 5 int encoded elements. The of_read_drc_info_cell()
>> helper contains comments that correctly name the expected elements
>> and their encoding. However, the usage of of_prop_next_string() and
>> of_prop_next_u32() introduced a subtle skippage of the first u32.
>> This is a result of of_prop_next_string() returns a pointer to the
>> next property value which is not a string, but actually a (__be32 *).
>> As, a result the following call to of_prop_next_u32() passes over the
>> current int encoded value and actually stores the next one wrongly.
>>
>> Simply endian swap the current value in place after reading the first
>> two string values. The remaining int encoded values can then be read
>> correctly using of_prop_next_u32().
> 
> Good but I think it would make more sense for a fix for
> of_read_drc_info_cell() to precede any patch in the series which
> introduces new callers, such as patch #1.
> 

Not sure it matters that much since everything in the series is required to
properly enable a working drc-info implementation and the call already exists so
it doesn't break bisectability. It ended up second in the series since testing
patch 1 exposed the flaw.

I'll go ahead and move it first, and add the appropriate fixes tag as well which
is currently missing.

-Tyrel

[RFC PATCH 3/9] powerpc/pseries: fix drc-info mappings of logical cpus to drc-index

[Tyrel Datwyler]

There are a couple subtle errors in the mapping between cpu-ids and a
cpus associated drc-index when using the new ibm,drc-info property.

The first is we while drc-info may have been a supported firmware
feature at boot it is possible we have migrated to a CEC with older
firmware that doesn't support the ibm,drc-info property. In that case
the device tree would have been updated after migration to remove the
ibm,drc-info property and replace it with the older style ibm,drc-*
properties for types, indexes, names, and power-domains.

The second is that the first value of the ibm,drc-info property is
the int encoded count of drc-info entries. As such "value" returned
by of_prop_next_u32() is pointing at that count, and not the first
element of the first drc-info entry as is expected by the
of_read_drc_info_cell() helper.

Fix the first by ignoring DRC-INFO firmware feature and instead
testing directly for ibm,drc-info, and then falling back to the
old style ibm,drc-indexes in the case it doesn't exit.

Fix the second by incrementing value to the next element prior to
parsing drc-info entries.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
index a96874f..09e98d3 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -36,6 +36,7 @@ static int sysfs_entries;
 static u32 cpu_to_drc_index(int cpu)
 {
 	struct device_node *dn = NULL;
+	struct property *info;
 	int thread_index;
 	int rc = 1;
 	u32 ret = 0;
@@ -47,20 +48,18 @@ static u32 cpu_to_drc_index(int cpu)
 	/* Convert logical cpu number to core number */
 	thread_index = cpu_core_index_of_thread(cpu);
 
-	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
-		struct property *info = NULL;
+	info = of_find_property(dn, "ibm,drc-info", NULL);
+	if (info) {
 		struct of_drc_info drc;
 		int j;
 		u32 num_set_entries;
 		const __be32 *value;
 
-		info = of_find_property(dn, "ibm,drc-info", NULL);
-		if (info == NULL)
-			goto err_of_node_put;
-
 		value = of_prop_next_u32(info, NULL, &num_set_entries);
 		if (!value)
 			goto err_of_node_put;
+		else
+			value++;
 
 		for (j = 0; j < num_set_entries; j++) {
 
@@ -110,6 +109,7 @@ static u32 cpu_to_drc_index(int cpu)
 static int drc_index_to_cpu(u32 drc_index)
 {
 	struct device_node *dn = NULL;
+	struct property *info;
 	const int *indexes;
 	int thread_index = 0, cpu = 0;
 	int rc = 1;
@@ -117,21 +117,18 @@ static int drc_index_to_cpu(u32 drc_index)
 	dn = of_find_node_by_path("/cpus");
 	if (dn == NULL)
 		goto err;
-
-	if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
-		struct property *info = NULL;
+	info = of_find_property(dn, "ibm,drc-info", NULL);
+	if (info) {
 		struct of_drc_info drc;
 		int j;
 		u32 num_set_entries;
 		const __be32 *value;
 
-		info = of_find_property(dn, "ibm,drc-info", NULL);
-		if (info == NULL)
-			goto err_of_node_put;
-
 		value = of_prop_next_u32(info, NULL, &num_set_entries);
 		if (!value)
 			goto err_of_node_put;
+		else
+			value++;
 
 		for (j = 0; j < num_set_entries; j++) {
 
-- 
2.7.4

[RFC PATCH 4/9] PCI: rpaphp: fix up pointer to first drc-info entry

[Tyrel Datwyler]

The first entry of the ibm,drc-info property is an int encoded count
of the number of drc-info entries that follow. The "value" pointer
returned by of_prop_next_u32() is still pointing at the this value
when we call of_read_drc_info_cell().

Fix up by incrementing the "value" pointer to point at the first
element of the first drc-info entry prior.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 drivers/pci/hotplug/rpaphp_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index 18627bb..e350264 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -239,6 +239,8 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 	value = of_prop_next_u32(info, NULL, &entries);
 	if (!value)
 		return -EINVAL;
+	else
+		value++;
 
 	for (j = 0; j < entries; j++) {
 		of_read_drc_info_cell(&info, &value, &drc);
-- 
2.7.4

[RFC PATCH 5/9] PCI: rpaphp: don't rely on firmware feature to imply drc-info support

[Tyrel Datwyler]

In the event that the partition is migrated to a platform with older
firmware that doesn't support the ibm,drc-info property the device
tree is modified to remove the ibm,drc-info property and replace it
with the older style ibm,drc-* properties for types, names, indexes,
and power-domains. One of the requirements of the drc-info firmware
feature is the the client is able to handle both the new property,
and old properties. Therefore we can't rely on the firmware feature
alone to dictate which property is currently present in the device
tree.

Fix this short coming by checking explicitly for the ibm,drc-info
property, and falling back to the older ibm,drc-* properties if it
doesn't exist.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 drivers/pci/hotplug/rpaphp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index e350264..e18e9a0 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -275,7 +275,7 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
 		return -EINVAL;
 	}
 
-	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
+	if (of_find_property(dn->parent, "ibm,drc-info", NULL))
 		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
 						*my_index);
 	else
-- 
2.7.4

[RFC PATCH 6/9] PCI: rpaphp: add drc-info support for hotplug slot registration

[Tyrel Datwyler]

Split physical PCI slot registration scanning into seperate routines
that support the old ibm,drc-* properties and one that supports the
new compressed ibm,drc-info property.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 drivers/pci/hotplug/rpaphp_core.c | 89 ++++++++++++++++++++++++++++++---------
 1 file changed, 69 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index e18e9a0..75d5771 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -328,23 +328,48 @@ static int is_php_dn(struct device_node *dn, const int **indexes,
 	return 1;
 }
 
-/**
- * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
- * @dn: device node of slot
- *
- * This subroutine will register a hotpluggable slot with the
- * PCI hotplug infrastructure. This routine is typically called
- * during boot time, if the hotplug slots are present at boot time,
- * or is called later, by the dlpar add code, if the slot is
- * being dynamically added during runtime.
- *
- * If the device node points at an embedded (built-in) slot, this
- * routine will just return without doing anything, since embedded
- * slots cannot be hotplugged.
- *
- * To remove a slot, it suffices to call rpaphp_deregister_slot().
- */
-int rpaphp_add_slot(struct device_node *dn)
+static int rpaphp_drc_info_add_slot(struct device_node *dn)
+{
+	struct slot *slot;
+	struct property *info;
+	struct of_drc_info drc;
+	char drc_name[MAX_DRC_NAME_LEN];
+	const __be32 *cur;
+	u32 count;
+	int retval = 0;
+
+	info = of_find_property(dn, "ibm,drc-info", NULL);
+	if (!info)
+		return 0;
+
+	cur = of_prop_next_u32(info, NULL, &count);
+	if (cur)
+		cur++;
+	else
+		return 0;
+
+	of_read_drc_info_cell(&info, &cur, &drc);
+	if (!is_php_type(drc.drc_type))
+		return 0;
+
+	sprintf(drc_name, "%s%d", drc.drc_name_prefix, drc.drc_name_suffix_start);
+
+	slot = alloc_slot_struct(dn, drc.drc_index_start, drc_name, drc.drc_power_domain);
+	if (!slot)
+		return -ENOMEM;
+
+	slot->type = simple_strtoul(drc.drc_type, NULL, 10);
+	retval = rpaphp_enable_slot(slot);
+	if (!retval)
+		retval = rpaphp_register_slot(slot);
+
+	if (retval)
+		dealloc_slot_struct(slot);
+
+	return retval;
+}
+
+static int rpaphp_drc_add_slot(struct device_node *dn)
 {
 	struct slot *slot;
 	int retval = 0;
@@ -352,9 +377,6 @@ int rpaphp_add_slot(struct device_node *dn)
 	const int *indexes, *names, *types, *power_domains;
 	char *name, *type;
 
-	if (!dn->name || strcmp(dn->name, "pci"))
-		return 0;
-
 	/* If this is not a hotplug slot, return without doing anything. */
 	if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
 		return 0;
@@ -393,6 +415,33 @@ int rpaphp_add_slot(struct device_node *dn)
 	/* XXX FIXME: reports a failure only if last entry in loop failed */
 	return retval;
 }
+
+/**
+ * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
+ * @dn: device node of slot
+ *
+ * This subroutine will register a hotpluggable slot with the
+ * PCI hotplug infrastructure. This routine is typically called
+ * during boot time, if the hotplug slots are present at boot time,
+ * or is called later, by the dlpar add code, if the slot is
+ * being dynamically added during runtime.
+ *
+ * If the device node points at an embedded (built-in) slot, this
+ * routine will just return without doing anything, since embedded
+ * slots cannot be hotplugged.
+ *
+ * To remove a slot, it suffices to call rpaphp_deregister_slot().
+ */
+int rpaphp_add_slot(struct device_node *dn)
+{
+	if (!dn->name || strcmp(dn->name, "pci"))
+		return 0;
+
+	if (of_find_property(dn, "ibm,drc-info", NULL))
+		return rpaphp_drc_info_add_slot(dn);
+	else
+		return rpaphp_drc_add_slot(dn);
+}
 EXPORT_SYMBOL_GPL(rpaphp_add_slot);
 
 static void __exit cleanup_slots(void)
-- 
2.7.4

[RFC PATCH 7/9] PCI: rpaphp: annotate and correctly byte swap DRC properties

[Tyrel Datwyler]

The device tree is in bid endian format and any properties directly
retrieved using OF helpers that don't explicitly byte swap should
be annotated. In particular there are several places where we grab
the opaque property value for the old ibm,drc-* properties and the
ibm,my-drc-index property.

Fix this for better static checking by annotating values we know to
explicitly big endian, and byte swap were appropriate.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 drivers/pci/hotplug/rpaphp_core.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index 75d5771..eabc0c51 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -154,9 +154,9 @@ static enum pci_bus_speed get_max_bus_speed(struct slot *slot)
 	return speed;
 }
 
-static int get_children_props(struct device_node *dn, const int **drc_indexes,
-		const int **drc_names, const int **drc_types,
-		const int **drc_power_domains)
+static int get_children_props(struct device_node *dn, const __be32 **drc_indexes,
+		const __be32 **drc_names, const __be32 **drc_types,
+		const __be32 **drc_power_domains)
 {
 	const int *indexes, *names, *types, *domains;
 
@@ -194,8 +194,8 @@ static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
 				char *drc_type, unsigned int my_index)
 {
 	char *name_tmp, *type_tmp;
-	const int *indexes, *names;
-	const int *types, *domains;
+	const __be32 *indexes, *names;
+	const __be32 *types, *domains;
 	int i, rc;
 
 	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
@@ -208,7 +208,7 @@ static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
 
 	/* Iterate through parent properties, looking for my-drc-index */
 	for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
-		if ((unsigned int) indexes[i + 1] == my_index)
+		if (be32_to_cpu(indexes[i + 1]) == my_index)
 			break;
 
 		name_tmp += (strlen(name_tmp) + 1);
@@ -267,7 +267,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
 			char *drc_type)
 {
-	const unsigned int *my_index;
+	const __be32 *my_index;
 
 	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
 	if (!my_index) {
@@ -277,10 +277,10 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
 
 	if (of_find_property(dn->parent, "ibm,drc-info", NULL))
 		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
-						*my_index);
+						be32_to_cpu(*my_index));
 	else
 		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
-						*my_index);
+						be32_to_cpu(*my_index));
 }
 EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
 
@@ -311,10 +311,10 @@ static int is_php_type(char *drc_type)
  * for built-in pci slots (even when the built-in slots are
  * dlparable.)
  */
-static int is_php_dn(struct device_node *dn, const int **indexes,
-		const int **names, const int **types, const int **power_domains)
+static int is_php_dn(struct device_node *dn, const __be32 **indexes,
+		const __be32 **names, const __be32 **types, const __be32 **power_domains)
 {
-	const int *drc_types;
+	const __be32 *drc_types;
 	int rc;
 
 	rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
@@ -374,7 +374,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
 	struct slot *slot;
 	int retval = 0;
 	int i;
-	const int *indexes, *names, *types, *power_domains;
+	const __be32 *indexes, *names, *types, *power_domains;
 	char *name, *type;
 
 	/* If this is not a hotplug slot, return without doing anything. */
-- 
2.7.4

[RFC PATCH 8/9] PCI: rpaphp: correctly match ibm, my-drc-index to drc-name when using drc-info

[Tyrel Datwyler]

The newer ibm,drc-info property is a condensed description of the old
ibm,drc-* properties (ie. names, types, indexes, and power-domains).
When matching a drc-index to a drc-name we need to verify that the
index is within the start and last drc-index range and map it to a
drc-name using the drc-name-prefix and logical index.

Fix the mapping by checking that the index is within the range of the
current drc-info entry, and build the name form the drc-name-prefix
and by adding the starting drc-name-suffix value with the sequential
index of subtracting ibm,my-drc-index from this entries
drc-start-index.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 drivers/pci/hotplug/rpaphp_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index eabc0c51..5327606 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -248,9 +248,10 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 		/* Should now know end of current entry */
 
 		/* Found it */
-		if (my_index <= drc.last_drc_index) {
+		if (my_index >= drc.drc_index_start && my_index <= drc.last_drc_index) {
+			int index = my_index - drc.drc_index_start;
 			sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
-				my_index);
+				drc.drc_name_suffix_start + index);
 			break;
 		}
 	}
-- 
2.7.4

[RFC PATCH 9/9] powerpc: Enable support for ibm,drc-info property

[Tyrel Datwyler]

Advertise client support for the PAPR architected ibm,drc-info device
tree property during CAS handshake.

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 arch/powerpc/kernel/prom_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index a4e7762..2ca9966 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1053,7 +1053,7 @@ static const struct ibm_arch_vec ibm_architecture_vec_template __initconst = {
 		.reserved2 = 0,
 		.reserved3 = 0,
 		.subprocessors = 1,
-		.byte22 = OV5_FEAT(OV5_DRMEM_V2),
+		.byte22 = OV5_FEAT(OV5_DRMEM_V2) | OV5_FEAT(OV5_DRC_INFO),
 		.intarch = 0,
 		.mmu = 0,
 		.hash_ext = 0,
-- 
2.7.4

Re: [RFC PATCH 0/9] Fixes and Enablement of ibm,drc-info property

[Bjorn Helgaas]

On Tue, Oct 01, 2019 at 01:12:05AM -0500, Tyrel Datwyler wrote:
> There was an initial previous effort yo add support for the PAPR
> architected ibm,drc-info property. This property provides a more
> memory compact representation of a paritions Dynamic Reconfig
> Connectors (DRC). These can otherwise be thought of the currently
> partitioned, or available, but yet to be partitioned, system resources
> such as cpus, memory, and physical/logical IOA devices.
> 
> The initial implementation proved buggy and was fully turned of by
> disabling the bit in the appropriate CAS support vector. We now have
> PowerVM firmware in the field that supports this new property, and 
> further to suppport partitions with 24TB+ or possible memory this
> property is required to perform platform migration.
> 
> This serious fixup the short comings of the previous implementation
> in the areas of general implementation, cpu hotplug, and IOA hotplug.
> 
> Tyrel Datwyler (9):
>   powerpc/pseries: add cpu DLPAR support for drc-info property
>   powerpc/pseries: fix bad drc_index_start value parsing of drc-info
>     entry
>   powerpc/pseries: fix drc-info mappings of logical cpus to drc-index
>   PCI: rpaphp: fix up pointer to first drc-info entry
>   PCI: rpaphp: don't rely on firmware feature to imply drc-info support
>   PCI: rpaphp: add drc-info support for hotplug slot registration
>   PCI: rpaphp: annotate and correctly byte swap DRC properties
>   PCI: rpaphp: correctly match ibm,my-drc-index to drc-name when using
>     drc-info
>   powerpc: Enable support for ibm,drc-info property
> 
>  arch/powerpc/kernel/prom_init.c                 |   2 +-
>  arch/powerpc/platforms/pseries/hotplug-cpu.c    | 117 ++++++++++++++++------
>  arch/powerpc/platforms/pseries/of_helpers.c     |   8 +-
>  arch/powerpc/platforms/pseries/pseries_energy.c |  23 ++---
>  drivers/pci/hotplug/rpaphp_core.c               | 124 +++++++++++++++++-------
>  5 files changed, 191 insertions(+), 83 deletions(-)

Michael, I assume you'll take care of this.  If I were applying, I
would capitalize the commit subjects and fix the typos in the commit
logs, e.g.,

  s/the this/the/
  s/the the/that the/
  s/short coming/shortcoming/
  s/seperate/separate/
  s/bid endian/big endian/
  s/were appropriate/where appropriate/
  s/name form/name from/

etc.  git am also complains about space before tab whitespace errors.
And it adds a few lines >80 chars.

Re: [RFC PATCH 0/9] Fixes and Enablement of ibm,drc-info property

[Tyrel Datwyler]

On 10/1/19 1:02 PM, Bjorn Helgaas wrote:
> On Tue, Oct 01, 2019 at 01:12:05AM -0500, Tyrel Datwyler wrote:
>> There was an initial previous effort yo add support for the PAPR
>> architected ibm,drc-info property. This property provides a more
>> memory compact representation of a paritions Dynamic Reconfig
>> Connectors (DRC). These can otherwise be thought of the currently
>> partitioned, or available, but yet to be partitioned, system resources
>> such as cpus, memory, and physical/logical IOA devices.
>>
>> The initial implementation proved buggy and was fully turned of by
>> disabling the bit in the appropriate CAS support vector. We now have
>> PowerVM firmware in the field that supports this new property, and 
>> further to suppport partitions with 24TB+ or possible memory this
>> property is required to perform platform migration.
>>
>> This serious fixup the short comings of the previous implementation
>> in the areas of general implementation, cpu hotplug, and IOA hotplug.
>>
>> Tyrel Datwyler (9):
>>   powerpc/pseries: add cpu DLPAR support for drc-info property
>>   powerpc/pseries: fix bad drc_index_start value parsing of drc-info
>>     entry
>>   powerpc/pseries: fix drc-info mappings of logical cpus to drc-index
>>   PCI: rpaphp: fix up pointer to first drc-info entry
>>   PCI: rpaphp: don't rely on firmware feature to imply drc-info support
>>   PCI: rpaphp: add drc-info support for hotplug slot registration
>>   PCI: rpaphp: annotate and correctly byte swap DRC properties
>>   PCI: rpaphp: correctly match ibm,my-drc-index to drc-name when using
>>     drc-info
>>   powerpc: Enable support for ibm,drc-info property
>>
>>  arch/powerpc/kernel/prom_init.c                 |   2 +-
>>  arch/powerpc/platforms/pseries/hotplug-cpu.c    | 117 ++++++++++++++++------
>>  arch/powerpc/platforms/pseries/of_helpers.c     |   8 +-
>>  arch/powerpc/platforms/pseries/pseries_energy.c |  23 ++---
>>  drivers/pci/hotplug/rpaphp_core.c               | 124 +++++++++++++++++-------
>>  5 files changed, 191 insertions(+), 83 deletions(-)
> 
> Michael, I assume you'll take care of this.  If I were applying, I
> would capitalize the commit subjects and fix the typos in the commit
> logs, e.g.,
> 
>   s/the this/the/
>   s/the the/that the/
>   s/short coming/shortcoming/
>   s/seperate/separate/
>   s/bid endian/big endian/
>   s/were appropriate/where appropriate/
>   s/name form/name from/
> 
> etc.  git am also complains about space before tab whitespace errors.
> And it adds a few lines >80 chars.
> 

I'll fix all those up in the next spin.

-Tyrel