Re: Resend: [PATCH V5 3/4] hotplug/drc-info: Add code to search ibm,drc-info property

[Michael Bringmann <mwb@linux.vnet.ibm.com>]

See below.

On 11/30/2017 01:51 PM, Nathan Fontenot wrote:
> 
> 
> On 11/28/2017 05:07 PM, Michael Bringmann wrote:
>> rpadlpar_core.c: Provide parallel routines to search the older device-
>> tree properties ("ibm,drc-indexes", "ibm,drc-names", "ibm,drc-types"
>> and "ibm,drc-power-domains"), or the new property "ibm,drc-info".
>>
>> The interface to examine the DRC information is changed from a "get"
>> function that returns values for local verification elsewhere, to a
>> "check" function that validates the 'name' and/or 'type' of a device
>> node.  This update hides the format of the underlying device-tree
>> properties, and concentrates the value checks into a single function
>> without requiring the user to verify whether a search was successful.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> Changes in V5:
>>   -- Simplify of_prop_next_u32 invocation
>>   -- Fix some spacing within arguments
>> ---
>>  drivers/pci/hotplug/rpadlpar_core.c |   13 ++--
>>  drivers/pci/hotplug/rpaphp.h        |    4 +
>>  drivers/pci/hotplug/rpaphp_core.c   |  109 +++++++++++++++++++++++++++--------
>>  3 files changed, 91 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
>> index a3449d7..fc01d7d 100644
>> --- a/drivers/pci/hotplug/rpadlpar_core.c
>> +++ b/drivers/pci/hotplug/rpadlpar_core.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/mutex.h>
>>  #include <asm/rtas.h>
>>  #include <asm/vio.h>
>> +#include <linux/firmware.h>
>>
>>  #include "../pci.h"
>>  #include "rpaphp.h"
>> @@ -44,15 +45,14 @@ static struct device_node *find_vio_slot_node(char *drc_name)
>>  {
>>  	struct device_node *parent = of_find_node_by_name(NULL, "vdevice");
>>  	struct device_node *dn = NULL;
>> -	char *name;
>>  	int rc;
>>
>>  	if (!parent)
>>  		return NULL;
>>
>>  	while ((dn = of_get_next_child(parent, dn))) {
>> -		rc = rpaphp_get_drc_props(dn, NULL, &name, NULL, NULL);
>> -		if ((rc == 0) && (!strcmp(drc_name, name)))
>> +		rc = rpaphp_check_drc_props(dn, drc_name, NULL);
>> +		if (rc == 0)
>>  			break;
>>  	}
>>
>> @@ -64,15 +64,12 @@ static struct device_node *find_php_slot_pci_node(char *drc_name,
>>  						  char *drc_type)
>>  {
>>  	struct device_node *np = NULL;
>> -	char *name;
>> -	char *type;
>>  	int rc;
>>
>>  	while ((np = of_find_node_by_name(np, "pci"))) {
>> -		rc = rpaphp_get_drc_props(np, NULL, &name, &type, NULL);
>> +		rc = rpaphp_check_drc_props(np, drc_name, drc_type);
>>  		if (rc == 0)
>> -			if (!strcmp(drc_name, name) && !strcmp(drc_type, type))
>> -				break;
>> +			break;
>>  	}
>>
>>  	return np;
>> diff --git a/drivers/pci/hotplug/rpaphp.h b/drivers/pci/hotplug/rpaphp.h
>> index 7db024e..8db5f2e 100644
>> --- a/drivers/pci/hotplug/rpaphp.h
>> +++ b/drivers/pci/hotplug/rpaphp.h
>> @@ -91,8 +91,8 @@ struct slot {
>>
>>  /* rpaphp_core.c */
>>  int rpaphp_add_slot(struct device_node *dn);
>> -int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
>> -		char **drc_name, char **drc_type, int *drc_power_domain);
>> +int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>> +		char *drc_type);
>>
>>  /* rpaphp_slot.c */
>>  void dealloc_slot_struct(struct slot *slot);
>> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
>> index 1e29aba..6da613a 100644
>> --- a/drivers/pci/hotplug/rpaphp_core.c
>> +++ b/drivers/pci/hotplug/rpaphp_core.c
>> @@ -30,6 +30,7 @@
>>  #include <linux/smp.h>
>>  #include <linux/init.h>
>>  #include <linux/vmalloc.h>
>> +#include <asm/firmware.h>
>>  #include <asm/eeh.h>       /* for eeh_add_device() */
>>  #include <asm/rtas.h>		/* rtas_call */
>>  #include <asm/pci-bridge.h>	/* for pci_controller */
>> @@ -196,25 +197,21 @@ static int get_children_props(struct device_node *dn, const int **drc_indexes,
>>  	return 0;
>>  }
>>
>> -/* To get the DRC props describing the current node, first obtain it's
>> - * my-drc-index property.  Next obtain the DRC list from it's parent.  Use
>> - * the my-drc-index for correlation, and obtain the requested properties.
>> +
>> +/* Verify the existence of 'drc_name' and/or 'drc_type' within the
>> + * current node.  First obtain it's my-drc-index property.  Next,
>> + * obtain the DRC info from it's parent.  Use the my-drc-index for
>> + * correlation, and obtain/validate the requested properties.
>>   */
>> -int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
>> -		char **drc_name, char **drc_type, int *drc_power_domain)
>> +
>> +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 unsigned int *my_index;
>> -	char *name_tmp, *type_tmp;
>>  	int i, rc;
>>
>> -	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
>> -	if (!my_index) {
>> -		/* Node isn't DLPAR/hotplug capable */
>> -		return -EINVAL;
>> -	}
>> -
>>  	rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
>>  	if (rc < 0) {
>>  		return -EINVAL;
>> @@ -225,24 +222,86 @@ int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,
>>
>>  	/* 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 (drc_name)
>> -				*drc_name = name_tmp;
>> -			if (drc_type)
>> -				*drc_type = type_tmp;
>> -			if (drc_index)
>> -				*drc_index = be32_to_cpu(*my_index);
>> -			if (drc_power_domain)
>> -				*drc_power_domain = be32_to_cpu(domains[i+1]);
>> -			return 0;
>> -		}
>> +		if ((unsigned int) indexes[i + 1] == my_index)
>> +			break;
>> +
>>  		name_tmp += (strlen(name_tmp) + 1);
>>  		type_tmp += (strlen(type_tmp) + 1);
>>  	}
>>
>> +	if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
>> +	    ((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
>> +		return 0;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
>> +				char *drc_type, unsigned int my_index)
>> +{
>> +	struct property *info;
>> +	unsigned int entries;
>> +	struct of_drc_info drc;
>> +	const __be32 *value;
>> +	int j;
>> +
>> +	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
>> +	if (info == NULL)
>> +		return -EINVAL;
>> +
>> +	value = (void *)of_prop_next_u32(info, NULL, &entries);
>> +	if (!value)
>> +		return -EINVAL;
>> +
>> +	for (j = 0; j < entries; j++) {
>> +		of_read_drc_info_cell(&info, &value, &drc);
>> +
>> +		/* Should now know end of current entry */
>> +
>> +		WARN_ON((my_index < drc.drc_index_start) ||
>> +			(((my_index - drc.drc_index_start) %
>> +				drc.sequential_inc) != 0));
>> +
>> +		if (my_index > drc.last_drc_index)
>> +			continue;
>> +
>> +		break;
>> +	}
>> +	/* Found it */
>> +
>> +	if (((drc_name == NULL) ||
>> +	     (drc_name && !strncmp(drc_name,
>> +				drc.drc_name_prefix,
>> +				strlen(drc.drc_name_prefix)))) &&
> 
> I'm still not convinced this is correct. If I understand correctly
> the new drc-info property has a name prefix field, which for the
> entries we are looking up here are likely "PHB", and a name suffix
> start filed for the first number to use as the suffix to the name in
> the current set.
> 
> The name we would be trying to compare to would be something like "PHB 34".
> What you have is just doing a compare on the prefix, or "PHB", part of
> the name and not the full name.

I reviewed the execution again, and I agree with you.  Updates to correct
the check will be in the next submission.

> 
> -Nathan

Michael

> 
>> +	    ((drc_type == NULL) ||
>> +	     (drc_type && !strncmp(drc_type,
>> +				drc.drc_type,
>> +				strlen(drc.drc_type)))))
>> +		return 0;
>> +
>>  	return -EINVAL;
>>  }
>> -EXPORT_SYMBOL_GPL(rpaphp_get_drc_props);
>> +
>> +int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
>> +			char *drc_type)
>> +{
>> +	const unsigned int *my_index;
>> +
>> +	my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
>> +	if (!my_index) {
>> +		/* Node isn't DLPAR/hotplug capable */
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (firmware_has_feature(FW_FEATURE_DRC_INFO))
>> +		return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
>> +						*my_index);
>> +	else
>> +		return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
>> +						*my_index);
>> +}
>> +EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
>> +
>>
>>  static int is_php_type(char *drc_type)
>>  {
>>
> 
> 

-- 
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line  363-5196
External: (512) 286-5196
Cell:       (512) 466-0650
mwb@linux.vnet.ibm.com