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

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

On 6/15/2026 9:08 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://netdev-ai.bots.linux.dev/sashiko/
> 
> Simon says: This review seems to be a bit of a mixed bag.
>              I've annotated the issues it raises that I'd appreciate
>              you looking over, and the rest, which I don't feel fall
>              in that category.
> ---
> pds_core: add PLDM component info display
> 
> This patch reports per-component firmware versions through devlink info on
> v2+ devices, replacing the single fw running version with names derived
> from the firmware-supplied component list and slot/flag fields.
> 
> Simon says: I'm not particularly concerned about the issue raised below -
>              I'm forwarding this review because of others raised further down.

Thanks for the review. Responding to the issues you flagged as 
warranting investigation below. I'm also addressing patch reordering and 
v2-error-handling in v5.

> Simon says: However, the issue flagged below seems to warrant
>              investigation in the context of this patch-set.
> 
> Second, pdsc->fw_components is only invalidated at the end of
> pdsc_firmware_update() ("pdsc->fw_components.num_components = 0;").
> If firmware versions change through any other path (FW recovery via
> health_work, PCI reset via pci_reset_work, side-channel upgrade) the
> cached list stays around and devlink dev info reports stale versions.
> Should those reset paths also clear pdsc->fw_components?

Valid concern. Will add a pdsc_fw_components_invalidate() helper in v5 
and call it from recovery and reset paths.


> Simon says: Likewise, the one below seems to warrant investigation too.
> 
> [Medium]
> num_components has already been bounded against
> PDS_CORE_FW_COMPONENT_LIST_LEN inside pdsc_get_component_info().
> max_fw_slots looks like a slot count (gold/main-a/main-b), but the
> component list also contains cpld/bootloader/uboot/etc.  If firmware
> reports six components on a three-slot device, this min_t() silently
> truncates legitimate component entries from the devlink output.
> 
> Is this cap intentional, and if so should truncation be logged?  If
> not, can the cap simply be dropped?

The field name max_fw_slots is misleading - firmware populates it with 
the component count, not a slot count. Will clean up the comment in v5. 
The min_t() is correct - it bounds against the number of components 
firmware reports.

> 
> Simon says: Again, the one below seems to warrant investigation.
> 
> [Medium]
> Pre-patch, devlink_info_version_stored_put() failures inside the fw
> list loop returned to the devlink layer immediately.  After this
> refactor, pdsc_dl_fw_list_info_get() returns the error but
> pdsc_dl_info_get_v1() converts it to dev_warn_once() and continues to
> append the running fw version and the ASIC fields.
> 
> Does this mean a mid-loop failure (for example -EMSGSIZE from netlink
> attribute append) is now masked, leaving a partially populated devlink
> info reply that gets reported as success?

Valid bug. The refactor unintentionally changed v1 error handling. 
Pre-patch, devlink_*() errors returned immediately. Will restore 
original v1 behavior in v5 - propagate errors from 
pdsc_dl_fw_list_info_get().

> Simon says: but this one seems to be more a question of style that substance.
> 
> [Medium]
> On v2+ devices, errors from pdsc_dl_component_info_get() are swallowed
> into dev_warn_once() and the function falls through to the ASIC
> fields.  Pre-patch behavior always reported pdsc->dev_info.fw_version
> via DEVLINK_INFO_VERSION_GENERIC_FW.
> 
> After this change, transient failures inside pdsc_get_component_info()
> (DMA mapping failure, -ENOMEM, the -ETIMEDOUT/-EAGAIN deferred-DMA
> path, the v0-firmware return-0-without-populating branch, the
> num_components > LIST_LEN -ENOMEM branch), or even firmware enumerating
> components without setting PDS_CORE_FW_COMPONENT_INFO_F_RUNNING on the
> MAIN component, cause devlink dev info to show only asic.id, asic.rev
> and the serial number with no firmware version reported at all.
> 
> Could the v2 path either retain the unconditional running fw fallback
> from dev_info.fw_version, or fall through to pdsc_dl_info_get_v1() on
> failure, so devlink info never silently loses the fw running version?
> 
> Also, since dev_warn_once() suppresses the warning across the lifetime
> of the device, callers cannot tell that the netlink reply is partial:
> a partially populated message gets the ASIC fields appended and is
> reported as success.
> 

For the v2 error handling: will propagate devlink errors (so partial 
replies are discarded) and add a fallback to dev_info.fw_version if 
component info fails.

Thanks,
Nikhil