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

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