[Skiboot] [PATCH 1/2] hdata/vpd: Rework vpd node creation logic
Oliver
oohall at gmail.com
Fri Oct 6 12:11:15 AEDT 2017
On Thu, Oct 5, 2017 at 8:10 PM, Vasant Hegde
<hegdevasant at linux.vnet.ibm.com> wrote:
> Presently we traverse SLCA structure to create various FRU nodes under /vpd
> node. We assumed that children are always contiguous. It happened to be
> contiguous in P8 and worked fine, but failed in P9 system. So it ended up
> populating duplicate node under wrong parent. Also failed to populate some
> of the nodes.
>
> Unfortunately there is no way to reach all the children of a given parent
> from parent node :-( Hence we have to rework vpd creation logic.
>
> This patch goes through all the SLCA entries serially and creates vpd node.
> Assumptions:
> - SLCA index is always serial (0..n)
> - When we traverse serially parent entry comes before child
> - Redundant resources are always consecutive
> - Populate node if SLCA has 'installed' and 'VPD collected' bit set
>
> Fixes : https://github.com/open-power/zaius-openpower/issues/36
> CC: Ananth N Mavinakayanahalli <ananth at linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> ---
> @Stewart,
> I've verified on P8 FSP system and P9 BMC system. It seems to be working fine.
> No regression issues.
>
> -Vasant
>
> hdata/vpd.c | 161 +++++++++++++++++++++++++++---------------------------------
> 1 file changed, 72 insertions(+), 89 deletions(-)
>
> diff --git a/hdata/vpd.c b/hdata/vpd.c
> index 836869a..c24ede8 100644
> --- a/hdata/vpd.c
> +++ b/hdata/vpd.c
> @@ -211,9 +211,6 @@ static const char *vpd_map_name(const char *vpd_name)
> return "Unknown";
> }
>
> -static struct dt_node *dt_create_vpd_node(struct dt_node *parent,
> - const struct slca_entry *entry);
> -
> static const struct card_info *card_info_lookup(char *ccin)
> {
> int i;
> @@ -340,85 +337,85 @@ static bool valid_child_entry(const struct slca_entry *entry)
> return false;
> }
>
> -static void vpd_add_children(struct dt_node *parent, uint16_t slca_index)
> +void vpd_data_parse(struct dt_node *node, const void *fruvpd, u32 fruvpd_sz)
> {
> - const struct slca_entry *s_entry, *child;
> - uint16_t current_child_index, max_index;
> -
> - s_entry = slca_get_entry(slca_index);
> - if (!s_entry || (s_entry->nr_child == 0))
> - return;
> -
> - /*
> - * This slca_entry has children. Parse the children array
> - * and add nodes for valid entries.
> - *
> - * A child entry is valid if all of the following criteria is met
> - * a. SLCA_INSTALL_INSTALLED is set in s_entry->install_indic
> - * b. SLCA_VPD_COLLECTED is set in s_entry->vpd_collected
> - * c. The SLCA is not a duplicate entry.
> - */
> -
> - /* current_index tracks where we are right now in the array */
> - current_child_index = be16_to_cpu(s_entry->child_index);
> -
> - /* max_index tracks how far down the array we must traverse */
> - max_index = be16_to_cpu(s_entry->child_index)
> - + be16_to_cpu(s_entry->nr_child);
> -
> - while (current_child_index < max_index) {
> - child = slca_get_entry(current_child_index);
> - if (!child)
> - return;
> -
> - if (valid_child_entry(child)) {
> - struct dt_node *node;
> -
> - node = dt_create_vpd_node(parent, child);
> - if (!node)
> - return;
> - }
> -
> - /* Skip dups -- currently we presume dups are contiguous */
> - if (child->nr_dups > 0)
> - current_child_index += child->nr_dups;
> - current_child_index++;
> - }
> - return;
> + if (vpd_find_record(fruvpd, fruvpd_sz, "OPFR", NULL))
> + vpd_opfr_parse(node, fruvpd, fruvpd_sz);
> + else
> + vpd_vini_parse(node, fruvpd, fruvpd_sz);
> }
>
> /* Create the vpd node and add its children */
> -static struct dt_node *dt_create_vpd_node(struct dt_node *parent,
> - const struct slca_entry *entry)
> +static void dt_create_vpd_node(struct dt_node *dt_vpd)
> {
> - struct dt_node *node;
> - const char *name;
> - uint64_t addr;
> + const char *name, *p_name;
> + int count, index;
> + uint64_t addr, p_addr;
> + struct HDIF_common_hdr *slca_hdr;
> + struct dt_node *parent, *node;
> + const struct slca_entry *entry, *p_entry;
> +
> + slca_hdr = get_hdif(&spira.ntuples.slca, SLCA_HDIF_SIG);
> + if (!slca_hdr) {
> + prerror("SLCA Invalid\n");
> + return;
> + }
>
> - name = vpd_map_name(entry->fru_id);
> - addr = (uint64_t)be16_to_cpu(entry->rsrc_id);
> - node = dt_new_addr(parent, name, addr);
> - if (!node) {
> - prerror("VPD: Creating node at %s@%"PRIx64" failed\n", name, addr);
> - return NULL;
> + count = HDIF_get_iarray_size(slca_hdr, SLCA_IDATA_ARRAY);
> + if (count < 0) {
> + prerror("SLCA: Can't find SLCA array size!\n");
> + return;
> }
>
> - /* Add location code */
> - slca_vpd_add_loc_code(node, be16_to_cpu(entry->my_index));
> - /* Add FRU label */
> - dt_add_property(node, "fru-type", entry->fru_id, 2);
> - /* Recursively add children */
> - vpd_add_children(node, be16_to_cpu(entry->my_index));
> + for (index = 0; index < count; index++) {
> + /* Get SLCA entry */
> + entry = slca_get_entry(index);
> + if (!entry)
> + goto next_entry;
> +
> + /*
> + * A child entry is valid if all of the following criteria is met
> + * a. SLCA_INSTALL_INSTALLED is set in s_entry->install_indic
> + * b. SLCA_VPD_COLLECTED is set in s_entry->vpd_collected
> + * c. The SLCA is not a duplicate entry.
> + */
> + if (!valid_child_entry(entry))
> + goto next_entry;
> +
> + name = vpd_map_name(entry->fru_id);
> + addr = (uint64_t)be16_to_cpu(entry->rsrc_id);
You should be able to remove the cast here. I'm not sure why the
original code has one.
> + /* Check node is already created or not */
> + if (dt_find_by_name_addr(dt_vpd, name, addr))
> + goto next_entry;
> +
> + /* Get parent node */
> + if (index == SLCA_ROOT_INDEX) {
> + parent = dt_vpd;
> + } else {
> + p_entry = slca_get_entry(entry->parent_index);
> + p_name = vpd_map_name(p_entry->fru_id);
> + p_addr = (uint64_t)be16_to_cpu(p_entry->rsrc_id);
Same here
> + parent = dt_find_by_name_addr(dt_vpd, p_name, p_addr);
> + }
> + if (!parent)
> + goto next_entry;
> +
> + node = dt_new_addr(parent, name, addr);
> + if (!node) {
> + prerror("VPD: Creating node at %s@%"PRIx64" failed\n",
> + name, addr);
> + goto next_entry;
> + }
>
> - return node;
> -}
> + /* Add location code */
> + slca_vpd_add_loc_code(node, be16_to_cpu(entry->my_index));
> + /* Add FRU label */
> + dt_add_property(node, "fru-type", entry->fru_id, 2);
>
> -void vpd_data_parse(struct dt_node *node, const void *fruvpd, u32 fruvpd_sz)
> -{
> - if (vpd_find_record(fruvpd, fruvpd_sz, "OPFR", NULL))
> - vpd_opfr_parse(node, fruvpd, fruvpd_sz);
> - else
> - vpd_vini_parse(node, fruvpd, fruvpd_sz);
> +next_entry:
> + /* Skip dups -- currently we presume dups are contiguous */
> + index += entry->nr_dups;
Is it possible to ignore the dups count and just rely on the
INSTALLED/COLLECTED flags to tell the real entry from the duplicates?
I'd be nice if we could avoid bugs due to assuming that SLCA entries
are contiguous, but I'm not sure when and why the redundant
entries are created and how you're supposed to differentiate them.
> + }
> }
>
> struct dt_node *dt_add_vpd_node(const struct HDIF_common_hdr *hdr,
> @@ -448,16 +445,7 @@ struct dt_node *dt_add_vpd_node(const struct HDIF_common_hdr *hdr,
> return NULL;
>
> if (first) {
> - entry = slca_get_entry(SLCA_ROOT_INDEX);
> - if (!entry) {
> - prerror("VPD: Could not find the slca root entry\n");
> - return NULL;
> - }
> -
> - node = dt_create_vpd_node(dt_vpd, entry);
> - if (!node)
> - return NULL;
> -
> + dt_create_vpd_node(dt_vpd);
> first = false;
> }
You could eliminate this if(first) stuff too by moving the SLCA
walking out of dt_create_vpd_node and into dt_init_vpd_node() or
similar.
>
> @@ -484,14 +472,9 @@ struct dt_node *dt_add_vpd_node(const struct HDIF_common_hdr *hdr,
> * corresponding slca entry which we would have used to populate the vpd
> * tree during the 'first' pass above so that we just need to perform
> * VINI parse and add the vpd data..
> - * Still, we consider this case and create fresh node under '/vpd' if
> - * 'node' not found.
> */
> - if (!node) {
> - node = dt_create_vpd_node(dt_vpd, entry);
> - if (!node)
> - return NULL;
> - }
> + if (!node)
> + return NULL;
>
> /* Parse VPD fields, ensure that it has not been added already */
> if (vpd_valid(fruvpd, fruvpd_sz)
> --
> 2.9.3
Reviewed-by: Oliver O'Halloran <oohall at gmail.com>
More information about the Skiboot
mailing list