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

[Vasant Hegde]

On 10/09/2017 09:59 AM, Oliver wrote:
> On Sat, Oct 7, 2017 at 4:31 AM, 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


.../...

>
>> +                       p_entry = slca_get_entry(entry->parent_index);
>
> entry->parent_index needs a be16_to_cpu() around it.

Yes. fixed in V3.

>
>
>> +                       p_name = vpd_map_name(p_entry->fru_id);
>> +                       p_addr = be16_to_cpu(p_entry->rsrc_id);
>> +                       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 -- dups are contiguous */
>> +               index += entry->nr_dups;
>> +       }
>>  }
>>
>>  struct dt_node *dt_add_vpd_node(const struct HDIF_common_hdr *hdr,
>> @@ -428,7 +430,6 @@ struct dt_node *dt_add_vpd_node(const struct HDIF_common_hdr *hdr,
>>         unsigned int fruvpd_sz, fru_id_sz;
>>         const struct slca_entry *entry;
>>         struct dt_node *dt_vpd, *node;
>> -       static bool first = true;
>>         const void *fruvpd;
>>         const char *name;
>>         uint64_t addr;
>> @@ -447,20 +448,6 @@ struct dt_node *dt_add_vpd_node(const struct HDIF_common_hdr *hdr,
>>         if (!dt_vpd)
>>                 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;
>> -
>> -               first = false;
>> -       }
>> -
>>         entry = slca_get_entry(be16_to_cpu(fru_id->slca_index));
>>         if (!entry)
>>                 return NULL;
>> @@ -484,14 +471,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
>>
>
> This also breaks the `make hdata-check` tests in skiboot due to a
> bunch of phandles changing. Can you add fix up the DTS output too?

Ah yes. log of phandles are changed. Will update in v3.

>
> Seems fine otherwise. Feel free to add my reviewed-by on the next spin.

Sure.

-Vasant