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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
pds_core: add PLDM component info display

This patch extends pdsc_dl_info_get() to report individual PLDM firmware
components (name, version, fixed/running/stored status) via devlink info
when the device identity is version 2 or newer, falling back to the
existing fw_list path otherwise.

> diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
> index 7f44e1a8d4fd..95c3d2531ef1 100644
> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
> @@ -93,14 +93,78 @@ 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;
> +	char buf[32];
> +
> +	ver_type = DEVLINK_INFO_VERSION_TYPE_NONE;
> +	snprintf(buf, sizeof(buf), "fw.%s", info->name);
> +	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)
> +		return devlink_info_version_fixed_put(req, buf, ver);

[Medium]
Are the FIXED, RUNNING and STARTUP flags meant to be mutually exclusive?

Looking at enum pds_core_component_info_flags, RUNNING, STARTUP, FIXED
and UPDATE_BY_NAME are independent bits. Returning immediately on
F_FIXED short-circuits the subsequent F_RUNNING and F_STARTUP checks,
so a fixed-but-running component (the natural case for goldfw / ROM
firmware that is currently executing) only surfaces as fixed.

The example in the commit message shows fw.goldfw under "running:":

      versions:
        running:
          fw.goldfw 1.2.3
          fw.mainfwa 1.2.4
          fw.mainfwb 1.2.3

If goldfw has F_FIXED|F_RUNNING set, would the early return here cause
the running entry for fw.goldfw to be dropped, contradicting the
example output?

> +
> +	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)
> +		return devlink_info_version_stored_put_ext(req, buf,
> +							   ver, ver_type);
> +
> +	return 0;
> +}
> +
> +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) {
> +			dev_err(pdsc->dev, "Failed to get component_info %pe\n",
> +				ERR_PTR(err));
> +			return err;
> +		}
> +	}

[Medium, Medium]
Two questions about the cache-population logic here.

First, pdsc_get_component_info() handles the ver==0 firmware-incompat
case by emitting a dev_warn and returning 0 without populating
pdsc->fw_components.num_components:

drivers/net/ethernet/amd/pds_core/fw.c:pdsc_get_component_info() {
    ...
    if (comp.get_component_info.ver == 0) {
        dev_warn(pdsc->dev,
                 "Incompatible get_component_info version %u reported by firmware\n",
                 comp.get_component_info.ver);
        err = 0;
        goto out;
    }
    ...
}

The cache-miss guard above checks num_components, which stays zero on
that firmware path. Will every devlink dev info invocation then re-run
the full PDS_CORE_CMD_GET_COMPONENT_INFO devcmd (a 4 KB kzalloc, a
dma_map_single/dma_unmap_single, a synchronous devcmd under
devcmd_lock with timeout devcmd_timeout * 2) and emit a fresh dev_warn
each time?

Second, is there a path that invalidates the cache after a successful
flash? pdsc_dl_flash_update() -> pdsc_firmware_update() ->
pdsc_pldm_firmware_update() / pdsc_legacy_firmware_update() does not
appear to clear pdsc->fw_components.num_components, and
pdsc_get_component_info() populates the info[] array in place. After a
flash, will subsequent devlink dev info calls keep showing the
pre-flash version strings until the driver is reloaded?

> +
> +	list_info = &pdsc->fw_components;
> +	num_components = min_t(u8, list_info->num_components,
> +			       le16_to_cpu(pdsc->dev_ident.max_fw_slots));

[Medium]
Is the type chosen for min_t() here correct?

max_fw_slots is __le16 in struct pds_core_dev_identity, so the wire
protocol allows values up to 65535, and min_t(u8, ...) casts both
operands to u8 before comparing.

For example, with max_fw_slots = 256, le16_to_cpu() yields 256, the
u8 cast turns it into 0, and the loop reports zero components even
when num_components is non-zero. With max_fw_slots = 257 only one
component would be reported, and so on.

Should this be min_t(u32, ...) (or similar wider type), or even just
list_info->num_components, given that pdsc_get_component_info()
already bounds num_components by PDS_CORE_FW_COMPONENT_LIST_LEN, the
actual size of list_info->info[]? Capping by max_fw_slots also seems
to silently drop valid entries when firmware reports more components
than slots.

> +	for (i = 0; i < num_components; i++) {
> +		err = pdsc_dl_report_component(req, &list_info->info[i]);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +

[ ... ]