Re: [PATCH 4/6] pds_core: add PLDM component info display

["Rao, Nikhil" <nikhirao@amd.com>]

On 7/16/2026 12:22 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> --- Patch [4]: [PATCH 4/6] pds_core: add PLDM component info display ---
> commit 4d0cc8aef76c7e2e30f98535495312200941b646
> Author: Brett Creeley <brett.creeley@amd.com>
> 
> pds_core: add PLDM component info display
> 
> Add detailed component information display via devlink info. This
> allows users to see individual firmware components and their versions.
> Components are reported as fixed, running, or stored based on their
> firmware-provided flags.
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
>> index 3b763ee1715ee..63fe45e91f710 100644
>> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
>> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
> 
> [ ... ]
> 
>> +static int pdsc_dl_component_info_get(struct devlink *dl,
>> +                                   struct devlink_info_req *req,
>> +                                   struct netlink_ext_ack *extack)
>> +{
>> +     struct pdsc *pdsc = devlink_priv(dl);
>> +     u8 num_components;
>> +     int err;
>> +     int i;
>> +
>> +     /* Pairs with WRITE_ONCE in pdsc_fw_components_invalidate().
>> +      * Use READ_ONCE to get a consistent snapshot of num_components.
>> +      * pdsc_fw_components_invalidate() can zero it concurrently during
>> +      * firmware recovery; using the local copy avoids iterating zero
>> +      * times when we already decided the cache was valid.
>> +      */
>> +     num_components = READ_ONCE(pdsc->fw_components.num_components);
>> +     if (!num_components) {
>> +             err = pdsc_get_component_info(pdsc);
> 
> [Severity: High]
> Does this code expose uninitialized memory to concurrent readers?
> 
> If two threads execute this concurrently after the firmware component list
> has been invalidated, one thread could enter pdsc_get_component_info(),
> which updates the count before the array is populated:
> 
> drivers/net/ethernet/amd/pds_core/fw.c:pdsc_get_component_info() {
>      ...
>          pdsc->fw_components.num_components = num_components;
>          for (i = 0; i < num_components; i++) {
>                  struct pds_core_fw_component_info *info =
>                          &pdsc->fw_components.info[i];
> 
>                  memcpy(info, &list_info->info[i], sizeof(*info));
>      ...
> }
> 
> A second thread could concurrently execute pdsc_dl_component_info_get(), see
> num_components > 0 via READ_ONCE(), and proceed to iterate over the array
> while the first thread is still executing memcpy() into the array.
> 
> Could this be addressed by moving the update of num_components to after the
> array population is complete, perhaps using smp_store_release() and
> smp_load_acquire() to ensure proper ordering?
> 
>> +             if (err)
>> +                     return pdsc_dl_report_fw_ver(req,
>> +                                                 pdsc->dev_info.fw_version);
>> +             num_components = READ_ONCE(pdsc->fw_components.num_components);
>> +             if (!num_components)
>> +                     return pdsc_dl_report_fw_ver(req,
>> +                                                 pdsc->dev_info.fw_version);
>> +     }
>> +
>> +     num_components = min_t(u16, num_components,
>> +                            le16_to_cpu(pdsc->dev_ident.max_fw_slots));
>> +     for (i = 0; i < num_components; i++) {
>> +             err = pdsc_dl_report_component(req,
>> +                                            &pdsc->fw_components.info[i]);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
>> +     return 0;
>> +}
Not a bug. devl_lock serializes the .info_get
ops on the instance, so the two concurrent executions of 
pdsc_dl_component_info_get described above cannot occur

Thanks,
Nikhil