[Skiboot] [PATCH 1/2] hdata/vpd: Rework vpd node creation logic

[Vasant Hegde]

On 10/06/2017 06:41 AM, Oliver wrote:
> 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(-)


.../...

>>
>> -       /* 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.

Agree. I just copied original code and left as is. will fix it.

>
>> +               /* 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

Yep.

>
>> +                       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.

Well, we don't have dups in BMC based system and for FSP system Venkatesh 
confirmed that it will be consecutive entries. I think I should update above 
comment.

>
>> +       }
>>  }
>>
>>  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.

Yeah. We may have to call new function from parse_hdat. .

-Vasant