Re: [PATCH net-next v6 4/6] pds_core: add PLDM component info display

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

On 7/2/2026 2:11 AM, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
> 
> 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://netdev-ai.bots.linux.dev/sashiko/
> ---
> pds_core: add PLDM component info display
> 
> This change adds detailed PLDM component info to devlink info on
> identity v2 devices, introduces a pdsc_fw_components_invalidate()
> helper used during firmware recovery and after firmware update, and
> maps PDS_CORE_FW_TYPE_MAIN to the generic "fw" name in the PLDM
> flash path.
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
>> index 38a2446571af..4d9ab8b74ec6 100644
>> --- a/drivers/net/ethernet/amd/pds_core/core.c
>> +++ b/drivers/net/ethernet/amd/pds_core/core.c
>> @@ -580,6 +580,8 @@ void pdsc_fw_up(struct pdsc *pdsc)
>>                return;
>>        }
>>
>> +     pdsc_fw_components_invalidate(pdsc);
>> +
>>        err = pdsc_setup(pdsc, PDSC_SETUP_RECOVERY);
>>        if (err)
>>                goto err_out;
> 
> [Medium]
> Can concurrent access to pdsc->fw_components race here?
> pdsc_fw_up() runs from pdsc_health_thread (workqueue, holding only
> pdsc->config_lock) and from pdsc_reset_done (PCI reset handler, holding
> neither), so pdsc_fw_components_invalidate() writes num_components = 0
> without the devlink instance lock.
> 
> Meanwhile, pdsc_dl_component_info_get() reads pdsc->fw_components from
> the devlink info_get callback under only the devlink lock, never
> config_lock. The two domains do not exclude each other.
> 
> In pdsc_dl_component_info_get():
> 
>          if (!pdsc->fw_components.num_components) {
>                  err = pdsc_get_component_info(pdsc);
>                  if (err || !pdsc->fw_components.num_components)
>                          return pdsc_dl_report_fw_ver(req,
>                                                      pdsc->dev_info.fw_version);
>          }
> 
>          list_info = &pdsc->fw_components;
>          num_components = min_t(u16, list_info->num_components,
>                                 le16_to_cpu(pdsc->dev_ident.max_fw_slots));
> 
> The reader can observe num_components > 0 on the first load, then
> re-read it inside min_t() and observe 0 after pdsc_fw_up() invalidates
> the cache, producing zero iterations and no fallback. Conversely, a
> stale non-zero count could be iterated over info[] entries describing
> the previous firmware.
> 
> No READ_ONCE/WRITE_ONCE or memory barriers are used. Would a lock or
> RCU be a better fit here?
Will fix in v7 using READ_ONCE/WRITE_ONCE pairing for num_components.
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
>> index 3b763ee1715e..bd234c3c50b2 100644
>> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
>> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
>> @@ -93,14 +93,110 @@ int pdsc_dl_flash_update(struct devlink *dl,
>>        return pdsc_firmware_update(pdsc, params, extack);
>>   }
>>
>> +static int pdsc_dl_report_component(struct devlink_info_req *req,
>> +                                 struct pds_core_fw_component_info *info)
>> +{
>> +     enum devlink_info_version_type ver_type;
>> +     u16 flags = le16_to_cpu(info->flags);
>> +     char *ver = info->version;
>> +     const char *name;
>> +     char buf[32];
>> +
>> +     /* Main firmware is reported as generic "fw" */
>> +     if (info->component_type == PDS_CORE_FW_TYPE_MAIN) {
>> +             if (info->slot_id == PDS_CORE_FW_SLOT_GOLD)
>> +                     snprintf(buf, sizeof(buf), "fw.gold");
>> +             else
>> +                     snprintf(buf, sizeof(buf), "fw");
>> +     } else {
>> +             name = pdsc_fw_type_to_name(info->component_type);
>> +             if (!name)
>> +                     return 0;
>> +
>> +             if (info->slot_id == PDS_CORE_FW_SLOT_GOLD)
>> +                     snprintf(buf, sizeof(buf), "fw.%s.gold", name);
>> +             else
>> +                     snprintf(buf, sizeof(buf), "fw.%s", name);
>> +     }
> 
> [Medium]
> For PDS_CORE_FW_TYPE_MAIN, only PDS_CORE_FW_SLOT_GOLD is special-cased.
> PDS_CORE_FW_SLOT_INVALID, PDS_CORE_FW_SLOT_A, and PDS_CORE_FW_SLOT_B all
> end up emitting the bare name "fw".
> 
> The legacy v1 path used distinct names fw.mainfwa and fw.mainfwb for
> these two slots, and devlink_info_version_*_put() does not deduplicate
> names. If firmware reports two MAIN components (which the v1
> pds_core_fw_list_info already exposed as A/B), will userspace see two
> entries both named "fw" with no way to distinguish them?
Both slots emit "fw" but are distinguished by version type: one is 
reported as "running", the other as "stored" if there's a pending 
update. If no pending update, the active slot is reported as both 
"running" and "stored".

V1 and V2 use different naming conventions - this is expected.

> 
> [Medium]
> Are the new component names documented anywhere? The driver introduces
> fw.bootloader, fw.cpld, fw.secure, fw.fpga, fw.suc, fw.suc.bootloader,
> fw.uboot, plus a .gold variant of each, and fw.gold.
> 
> Documentation/networking/devlink/devlink-info.rst says:
> 
>    It is expected that drivers use the following generic names ... As
>    last resort, if a component is truly unique, using driver-specific
>    names is allowed, but these should be documented in the driver-specific
>    file.
> 
> Of those names only fw and fw.bootloader correspond to documented
> generic suffixes; the rest appear to be driver-invented, and there is
> no Documentation/networking/devlink/pds_core.rst introduced by this
> patch. 
Accepted. Will move devlink documentation to 
Documentation/networking/devlink/pds_core.rst in a follow-up patch per 
devlink-info.rst requirements.

>The .gold suffix in particular is not part of the generic
> vocabulary (different copies of the same component are normally handled
> via bundle_id or by reporting the same name under both running and
> stored). 
We do report the same name under both running and stored for main 
firmware (fw appears in both sections). The .gold suffix is for recovery 
firmware, which is a different component - not another copy of the same 
firmware.

> Composite names like fw.suc.bootloader.gold do not have any
> documented dot-separation semantics either.
The .gold suffix is documented as indicating recovery firmware. 
Composite names like fw.suc.bootloader use dots to separate hierarchical 
components (SUC's bootloader).

> 
>> +
>> +     ver_type = DEVLINK_INFO_VERSION_TYPE_NONE;
>> +     if (flags & PDS_CORE_FW_COMPONENT_INFO_F_UPDATE_BY_NAME)
>> +             ver_type = DEVLINK_INFO_VERSION_TYPE_COMPONENT;
>> +
>> +     if (flags & PDS_CORE_FW_COMPONENT_INFO_F_FIXED) {
>> +             int err;
>> +
>> +             err = devlink_info_version_fixed_put(req, buf, ver);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
>> +     if (flags & PDS_CORE_FW_COMPONENT_INFO_F_RUNNING) {
>> +             int err;
>> +
>> +             err = devlink_info_version_running_put_ext(req, buf,
>> +                                                        ver, ver_type);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
>> +     if (flags & PDS_CORE_FW_COMPONENT_INFO_F_STARTUP) {
>> +             int err;
>> +
>> +             err = devlink_info_version_stored_put_ext(req, buf,
>> +                                                       ver, ver_type);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
>> +     return 0;
>> +}
> 
> [Medium]
> Is the generic running "fw" entry still guaranteed on the v2 path?
> It is produced only as a side effect of pdsc_dl_report_component()
> seeing component_type == PDS_CORE_FW_TYPE_MAIN with
> PDS_CORE_FW_COMPONENT_INFO_F_RUNNING set.
> 
> The v1 path always called:
> 
>          devlink_info_version_running_put(req,
>                                           DEVLINK_INFO_VERSION_GENERIC_FW,
>                                           pdsc->dev_info.fw_version);
> 
> unconditionally. If firmware reports MAIN only with F_STARTUP/F_FIXED
> (no F_RUNNING), or reports no MAIN component at all, does the user
> silently lose the generic running fw version that monitoring tools rely
> on to identify the active firmware?
No, Firmware always reports F_RUNNING for the currently running slot.

> 
>> +
>> +static int pdsc_dl_report_fw_ver(struct devlink_info_req *req, char *fw_ver)
>> +{
>> +     return devlink_info_version_running_put(req,
>> +                                             DEVLINK_INFO_VERSION_GENERIC_FW,
>> +                                             fw_ver);
>> +}
>> +
>> +static int pdsc_dl_component_info_get(struct devlink *dl,
>> +                                   struct devlink_info_req *req,
>> +                                   struct netlink_ext_ack *extack)
>> +{
>> +     struct pds_core_component_list_info *list_info;
>> +     struct pdsc *pdsc = devlink_priv(dl);
>> +     u8 num_components;
>> +     int err;
>> +     int i;
>> +
>> +     if (!pdsc->fw_components.num_components) {
>> +             err = pdsc_get_component_info(pdsc);
>> +             if (err || !pdsc->fw_components.num_components)
>> +                     return pdsc_dl_report_fw_ver(req,
>> +                                                 pdsc->dev_info.fw_version);
>> +     }
> 
> [High]
> Can devlink info polling accumulate deferred DMA buffers when the
> firmware is hung?
> 
> pdsc_dl_component_info_get() is reachable from any devlink dev info
> request. On -EAGAIN or -ETIMEDOUT, pdsc_get_component_info() calls
> pdsc_deferred_dma_add() and returns the error without setting
> pdsc->fw_components.num_components:
> 
>          err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout * 2);
>          if (err == -ETIMEDOUT || err == -EAGAIN) {
>                  pdsc_deferred_dma_add(pdsc, deferred, dma_addr, list_info,
>                                        PDS_PAGE_SIZE, DMA_FROM_DEVICE);
>                  return err;
>          }
> 
> The cached count therefore stays 0, so the next devlink info request
> re-enters the same path and queues another PDS_PAGE_SIZE allocation
> plus DMA mapping. The deferred list is only drained by
> pdsc_deferred_dma_free() in __pdsc_devcmd_locked() on a non-EAGAIN
> return.
> 
> Before this patch, pdsc_get_component_info() was reachable only from
> the privileged flash path. Is it intentional that any monitoring agent
> polling devlink dev info while firmware is wedged can grow the deferred
> DMA list unbounded?
If firmware is hung, health_work triggers recovery. Deferred buffers are 
cleaned up in pdsc_teardown() during recovery. Each buffer is ~4KB, and 
repeated polling during a hang accumulates minimal memory before 
recovery kicks in and cleans up.

> 
> [Medium]
> On the v2 branch, the function falls back to a single
> pdsc_dl_report_fw_ver() call whenever pdsc_get_component_info() returns
> an error or num_components is still 0 (which also happens when the
> firmware reports comp.get_component_info.ver == 0 and the function
> returns 0).
> 
> Before this patch, pdsc_dl_info_get() unconditionally invoked
> PDS_CORE_FW_GET_LIST and emitted fw.goldfw, fw.mainfwa, fw.mainfwb.
> On v2 devices that branch is unreachable, since pdsc_dl_fw_list_info_get()
> is only called from pdsc_dl_info_get_v1().
> 
> For transitional firmware that advertises identity v2 but does not yet
> support PLDM/GET_COMPONENT_INFO, would calling pdsc_dl_fw_list_info_get()
> as a v2 fallback preserve the previous four-version output rather than
> silently collapsing it to one?
V2 firmware always supports GET_COMPONENT_INFO. There is no transitional 
v2 firmware without this support

> 
>> +
>> +     list_info = &pdsc->fw_components;
>> +     num_components = min_t(u16, list_info->num_components,
>> +                            le16_to_cpu(pdsc->dev_ident.max_fw_slots));
>> +     for (i = 0; i < num_components; i++) {
>> +             err = pdsc_dl_report_component(req, &list_info->info[i]);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
>> +     return 0;
>> +}
> 
> [Medium]
> What happens if dev_ident.max_fw_slots is 0? pdsc_get_component_info()
> populates pdsc->fw_components.num_components from the firmware list
> (bounded by PDS_CORE_FW_COMPONENT_LIST_LEN), but the loop bound clamps
> that against the separate max_fw_slots identity field:
> 
>          num_components = min_t(u16, list_info->num_components,
>                                 le16_to_cpu(pdsc->dev_ident.max_fw_slots));
> 
> If firmware advertises identity v2 but reports max_fw_slots == 0, the
> earlier !pdsc->fw_components.num_components check does not fire (the
> count is non-zero), min_t() yields 0, the loop body is skipped, and
> pdsc_dl_info_get() proceeds straight to ASIC ID/REV/serial. Would
> adding a fallback to pdsc_dl_report_fw_ver() in this case avoid
> emitting devlink info with no fw entry at all?
"max_fw_slots" is the component count from firmware identity.A v2 device 
reporting max_fw_slots == 0 would be invalid firmware configuration

>> +
>>   static char *fw_slotnames[] = {
>>        "fw.goldfw",
>>        "fw.mainfwa",
>>        "fw.mainfwb",
>>   };
>>
>> -int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>> -                  struct netlink_ext_ack *extack)
>> +static int pdsc_dl_fw_list_info_get(struct devlink *dl,
>> +                                 struct devlink_info_req *req,
>> +                                 struct netlink_ext_ack *extack)
>>   {
> 
> [ ... ]
> 
>> @@ -134,12 +230,41 @@ int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>>                        return err;
>>        }
>>
>> -     err = devlink_info_version_running_put(req,
>> -                                            DEVLINK_INFO_VERSION_GENERIC_FW,
>> -                                            pdsc->dev_info.fw_version);
>> +     return 0;
>> +}
>> +
>> +static int pdsc_dl_info_get_v1(struct devlink *dl,
>> +                            struct devlink_info_req *req,
>> +                            struct netlink_ext_ack *extack)
>> +{
>> +     struct pdsc *pdsc = devlink_priv(dl);
>> +     int err;
>> +
>> +     err = pdsc_dl_fw_list_info_get(dl, req, extack);
>>        if (err)
>>                return err;
>>
>> +     /* Version 1: report fw from dev_info (running only) */
>> +     return pdsc_dl_report_fw_ver(req, pdsc->dev_info.fw_version);
>> +}
>> +
>> +int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>> +                  struct netlink_ext_ack *extack)
>> +{
>> +     struct pdsc *pdsc = devlink_priv(dl);
>> +     char buf[32];
>> +     int err;
>> +
>> +     if (pdsc->dev_ident.version >= PDS_CORE_IDENTITY_VERSION_2) {
>> +             err = pdsc_dl_component_info_get(dl, req, extack);
>> +             if (err)
>> +                     return err;
>> +     } else {
>> +             err = pdsc_dl_info_get_v1(dl, req, extack);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
> 
> [Medium]
> Is the devlink-info naming change across the identity-version boundary
> intentional, and is it captured in the commit message or a Documentation
> update?
Yes, the naming change is intentional. This is documented in the "Info 
versions (PLDM)" section of pds_core.rst, which lists all the V2 
component names separately from the legacy V1 names.

> Before this patch every device emitted stored slot names from
> fw_slotnames[] (fw.goldfw, fw.mainfwa, fw.mainfwb) via the unconditional
> PDS_CORE_FW_GET_LIST devcmd. After this patch, devices that advertise
> identity >= PDS_CORE_IDENTITY_VERSION_2 take the
> pdsc_dl_component_info_get() branch and emit a different set of names
> (fw, fw.gold, fw.bootloader, fw.uboot, fw.cpld, ...), with no alias for
> the old names.
> 
> A firmware-only upgrade can therefore silently rename the user-visible
> stored entries. Should the commit message call out this UAPI rename,
> and should the legacy names remain emitted as aliases for compatibility
> with existing fleet tooling that parses devlink dev info output?
V1 named specific physical slots (fw.mainfwa, fw.mainfwb). V2 uses a 
single fw name that appears in either the running: or stored: section. 
The concepts don't map 1:1, emitting legacy names as aliases doesn't 
make sense. Fleet tooling that upgrades firmware to PLDM-capable version 
should expect the new format.

Nikhil