Re: [PATCH net-next v6 3/6] pds_core: add PLDM firmware update support via devlink flash

[Paolo Abeni <pabeni@redhat.com>]

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 firmware update support via devlink flash

Implements PLDM firmware update via devlink flash using the upstream
pldmfw API. Adds whole-package and per-component update paths, and
introduces a deferred-DMA list that releases buffers after timed-out
devcmds. A few questions inline.

> diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
> index b7fe9ad73349..c686f0bbbaeb 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.h
> +++ b/drivers/net/ethernet/amd/pds_core/core.h
> @@ -23,6 +23,14 @@
>  #define PDSC_SETUP_RECOVERY	false
>  #define PDSC_SETUP_INIT		true
>  
> +struct pdsc_deferred_dma {
> +	struct list_head list;
> +	dma_addr_t dma_addr;
> +	void *va;
> +	size_t size;
> +	enum dma_data_direction dir;
> +};
> +

[ ... ]

> diff --git a/drivers/net/ethernet/amd/pds_core/dev.c b/drivers/net/ethernet/amd/pds_core/dev.c
> index 5c0ca3d0b000..6082b28915db 100644
> --- a/drivers/net/ethernet/amd/pds_core/dev.c
> +++ b/drivers/net/ethernet/amd/pds_core/dev.c
> @@ -206,15 +206,53 @@ static int __pdsc_devcmd_locked(struct pdsc *pdsc, union pds_core_dev_cmd *cmd,
>  	else
>  		memcpy_fromio(comp, &pdsc->cmd_regs->comp, sizeof(*comp));
>  
> +	if (err != -ETIMEDOUT && err != -EAGAIN)
> +		pdsc_deferred_dma_free(pdsc);
> +
>  	return err;
>  }

[High]
Can the deferred-DMA scheme actually guarantee that the device has
finished accessing the buffer before it is unmapped and freed?

The trigger for releasing a deferred entry is "the next devcmd returned
something other than -ETIMEDOUT/-EAGAIN", plus pdsc_remove() and
pdsc_reset_prepare(). None of these provide a hardware-level fence that
the previous in-flight DMA has drained. In the remove/reset paths the
free runs after pci_disable_device(), which clears bus-master but does
not synchronously drain pending transactions.

The DMA_FROM_DEVICE case in pdsc_get_component_info() is particularly
exposed because the buffer is plain kernel slab memory: a late device
write after kfree would corrupt unrelated allocations.

Also note that pdsc_send_package_data(), pdsc_get_component_info(), and
pdsc_flash_component_chunk() only defer on -EAGAIN/-ETIMEDOUT and
immediately unmap+kfree on any other error (-EIO, -ENXIO, ...). Is the
device guaranteed not to have started any DMA in those cases?

> +void pdsc_deferred_dma_add(struct pdsc *pdsc, struct pdsc_deferred_dma *entry,
> +			   dma_addr_t dma_addr, void *va, size_t size,
> +			   enum dma_data_direction dir)
> +{
> +	entry->dma_addr = dma_addr;
> +	entry->va = va;
> +	entry->size = size;
> +	entry->dir = dir;
> +
> +	spin_lock(&pdsc->deferred_dma_lock);
> +	list_add_tail(&entry->list, &pdsc->deferred_dma_list);
> +	spin_unlock(&pdsc->deferred_dma_lock);
> +}
> +
> +void pdsc_deferred_dma_free(struct pdsc *pdsc)
> +{
> +	struct pdsc_deferred_dma *entry, *tmp;
> +
> +	spin_lock(&pdsc->deferred_dma_lock);
> +	list_for_each_entry_safe(entry, tmp, &pdsc->deferred_dma_list, list) {
> +		dma_unmap_single(pdsc->dev, entry->dma_addr,
> +				 entry->size, entry->dir);
> +		kfree(entry->va);
> +		list_del(&entry->list);
> +		kfree(entry);
> +	}
> +	spin_unlock(&pdsc->deferred_dma_lock);
> +}

[Medium]
Is it intentional to hold deferred_dma_lock across dma_unmap_single()
and two kfree()s per entry? This runs from __pdsc_devcmd_locked() on
every successful devcmd while devcmd_lock is also held, and
dma_unmap_single() can trigger IOMMU IOTLB invalidations. Would
list_splice_init() under the lock followed by iteration on a detached
list scale better when many entries have accumulated?

[ ... ]

> diff --git a/drivers/net/ethernet/amd/pds_core/fw.c b/drivers/net/ethernet/amd/pds_core/fw.c
> index fa626719e68d..433b0a8df4c1 100644
> --- a/drivers/net/ethernet/amd/pds_core/fw.c
> +++ b/drivers/net/ethernet/amd/pds_core/fw.c

[ ... ]

> +#define PDSC_FW_COMPONENT_PREFIX		"fw."
> +#define PDSC_FW_COMPONENT_FULL_NAME_BUFLEN \
> +	(sizeof(PDSC_FW_COMPONENT_PREFIX) + PDS_CORE_FW_COMPONENT_NAME_BUFLEN)
> +
> +/* Driver-defined component type to name mapping.
> + * PDS_CORE_FW_TYPE_MAIN is NULL - handled specially as "fw" without prefix.
> + */
> +static const char * const pdsc_fw_type_names[] = {
> +	[PDS_CORE_FW_TYPE_MAIN]      = NULL,
> +	[PDS_CORE_FW_TYPE_BOOT]      = "bootloader",
> +	[PDS_CORE_FW_TYPE_CPLD]      = "cpld",
> +	[PDS_CORE_FW_TYPE_SECURE]    = "secure",
> +	[PDS_CORE_FW_TYPE_FPGA]      = "fpga",
> +	[PDS_CORE_FW_TYPE_SUC_MAIN]  = "suc",
> +	[PDS_CORE_FW_TYPE_SUC_BOOT]  = "suc.bootloader",
> +	[PDS_CORE_FW_TYPE_UBOOT]     = "uboot",
> +};

[Low, Low]
The documentation table added in this same patch lists fw.gold as a
driver-defined component name, and the kerneldoc on
enum pds_core_fw_component_type says gold variants are "reported with
a .gold suffix (e.g., fw.gold)". The table above has no gold entry, so
pdsc_name_to_fw_type("fw.gold") returns 0 and pdsc_pldm_firmware_update()
rejects the request with -ENOENT. Should fw.gold either be wired up or
the docs clarify that gold is read-only?

PDSC_FW_COMPONENT_FULL_NAME_BUFLEN is defined here but never used;
pdsc_flash_component() uses an ad-hoc sizeof(PDSC_FW_COMPONENT_PREFIX) +
16 instead. Should this macro be wired up or dropped?

[ ... ]

> +int pdsc_get_component_info(struct pdsc *pdsc)
> +{
> +	union pds_core_dev_cmd cmd = {
> +		.get_component_info.opcode = PDS_CORE_CMD_GET_COMPONENT_INFO,
> +		.get_component_info.ver = 1,
> +	};

[ ... ]

> +	if (comp.get_component_info.ver == 0) {
> +		/* Don't support backward compatibility as version 0 has
> +		 * alignment issues, so give a hint to users to update
> +		 * their firmware
> +		 */
> +		dev_warn_once(pdsc->dev,
> +			      "Incompatible get_component_info version %u reported by firmware\n",
> +			      comp.get_component_info.ver);
> +		err = 0;
> +		goto out;
> +	}

[Low]
This branch returns 0 without populating pdsc->fw_components. The caller
pdsc_pldm_firmware_update() uses:

	if (!pdsc->fw_components.num_components) {
		err = pdsc_get_component_info(pdsc);
		if (err) { ... return err; }
	}

	if (params->component) {
		u8 type = pdsc_name_to_fw_type(params->component);

		if (!type || !pdsc_component_type_exists(pdsc, type))
			return -ENOENT;
	}

so on a device that reports ver == 0, the cache never populates and
every per-component flash returns -ENOENT. Should this branch return an
explicit error instead, or otherwise prevent the per-component lookup
from running?

[ ... ]

> +static int pdsc_devcmd_send_component(struct pdsc *pdsc,
> +				      struct pds_core_flash_component *info,
> +				      u16 info_sz, dma_addr_t addr, u32 length,
> +				      u32 offset, u16 slot_id,
> +				      union pds_core_dev_comp *comp)
> +{
> +	union pds_core_dev_cmd cmd = {
> +		.send_component.opcode = PDS_CORE_CMD_SEND_COMPONENT,
> +		.send_component.ver = 1,
> +		.send_component.operation = PDS_CORE_SEND_COMPONENT_START,
> +		.send_component.data_pa = cpu_to_le64(addr),
> +		.send_component.data_len = cpu_to_le32(length),
> +		.send_component.offset = cpu_to_le32(offset),
> +		.send_component.slot_id = slot_id,
> +	};
> +	unsigned long timeout = 300 * HZ;
> +	unsigned long start_time;
> +	unsigned long end_time;
> +	int err;
> +
> +	start_time = jiffies;
> +	end_time = start_time + timeout;
> +	do {
> +		/* prevent noisy/benign devcmd failures */
> +		err = pdsc_devcmd_with_data_nomsg(pdsc, &cmd, info, info_sz,
> +						  comp, 60);
> +		if (err != -EAGAIN)
> +			break;
> +
> +		/* if required, subsequent commands check status of
> +		 * PDS_CORE_CMD_SEND_COMPONENT command, which returns
> +		 * EAGAIN while the command is still running,
> +		 * else we get the final command status.
> +		 */
> +		cmd.send_component.operation = PDS_CORE_SEND_COMPONENT_STATUS;
> +		msleep(20);
> +	} while (time_before(jiffies, end_time));

[Low]
In the STATUS retry phase, only cmd.send_component.operation changes
between iterations; data_pa, data_len, offset, and slot_id remain set
to the original START values, and pdsc_devcmd_with_data_nomsg() also
re-issues memcpy_toio(&pdsc->cmd_regs->data, info, info_sz) on every
poll (potentially up to ~15000 iterations over the 300s timeout at 20ms
cadence). Does the firmware ignore data_pa/data_len on the STATUS
opcode, or could it re-initiate DMA from the same buffer? Could the
status poll send just the operation field?

[ ... ]

> +static int pdsc_finalize_update(struct pldmfw *context)
> +{
[ ... ]
> +	start_time = jiffies;
> +	end_time = start_time + (PDSC_FW_INSTALL_TIMEOUT * HZ);
> +	do {
> +		err = pdsc_devcmd_finalize_update(pdsc);
> +		if (!err || err != -EAGAIN)
> +			break;
> +
> +		dev_dbg(dev, "retrying finalize_update: %pe\n", ERR_PTR(err));
> +		msleep(20);
> +	} while (time_before(jiffies, end_time) && err == -EAGAIN);

[Low]
Is the !err in if (!err || err != -EAGAIN) intentional? err == 0 already
satisfies err != -EAGAIN, so the condition reduces to err != -EAGAIN.

[ ... ]

> +static int pdsc_pldm_firmware_update(struct pdsc *pdsc,
> +				     struct devlink_flash_update_params *params,
> +				     struct netlink_ext_ack *extack,
> +				     const struct firmware *fw)
> +{
> +	struct pds_core_fwu_priv priv = {};
> +	int err;
> +
> +	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;
> +		}
> +	}
> +
> +	if (params->component) {
> +		u8 type = pdsc_name_to_fw_type(params->component);
> +
> +		if (!type || !pdsc_component_type_exists(pdsc, type))
> +			return -ENOENT;
> +	}
> +
> +	INIT_LIST_HEAD(&priv.components);
> +	priv.context.ops = &pdsc_pldmfw_ops;
> +	priv.context.dev = pdsc->dev;
> +	priv.params = params;
> +	priv.pdsc = pdsc;
> +
> +	err = pldmfw_flash_image(&priv.context, fw);
> +	pdsc_free_fwu_priv(&priv);
> +
> +	return err;
> +}

[Medium]
The extack parameter is accepted here and struct pds_core_fwu_priv
declares a netlink_ext_ack *extack field, but priv.extack is never
assigned, and none of the PLDM callbacks (pdsc_match_record_descs,
pdsc_send_package_data, pdsc_send_component_table, pdsc_flash_component,
pdsc_finalize_update) call NL_SET_ERR_MSG_MOD(). The legacy path uses
extack at multiple call sites ("Firmware wait timed out", "Segment
download failed", ...). Was the intent to propagate extack diagnostics
on the PLDM path too, including for the -ENOENT returned above?

> +int pdsc_firmware_update(struct pdsc *pdsc,
> +			 struct devlink_flash_update_params *params,
> +			 struct netlink_ext_ack *extack)
> +{
> +	int err;
> +
> +	if (pdsc->dev_ident.version >= PDS_CORE_IDENTITY_VERSION_2 &&
> +	    pdsc->dev_ident.capabilities &
> +		cpu_to_le64(PDS_CORE_DEV_CAP_PLDM_FW_UPDATE))
> +		err = pdsc_pldm_firmware_update(pdsc, params, extack,
> +						params->fw);
> +	else
> +		err = pdsc_legacy_firmware_update(pdsc, params->fw, extack);
> +
> +	/* Invalidate cached component info so next info_get refreshes */
> +	pdsc->fw_components.num_components = 0;
> +
> +	return err;
> +}

[Medium]
What happens here when a user runs:

    devlink dev flash pci/0000:b5:00.0 file foo component fw.cpld

against a device that does not advertise PDS_CORE_DEV_CAP_PLDM_FW_UPDATE?
The dispatcher falls through to pdsc_legacy_firmware_update(pdsc,
params->fw, extack), which has no notion of components and proceeds to
flash the entire image.

The commit message says:

    Not all components support per-component update - devlink will
    reject the request if the specified component cannot be updated.

Should the dispatcher return -EOPNOTSUPP (or similar) when
params->component != NULL on the legacy path, to match that promise?

> diff --git a/include/linux/pds/pds_core_if.h b/include/linux/pds/pds_core_if.h
> index 619186f26b5b..5a1fafaccf20 100644
> --- a/include/linux/pds/pds_core_if.h
> +++ b/include/linux/pds/pds_core_if.h
> @@ -279,11 +299,20 @@ enum pds_core_fw_control_oper {
>  	PDS_CORE_FW_GET_LIST               = 7,
>  };
>  
> +/**
> + * enum pds_core_fw_slot - Firmware slot identifiers
> + * @PDS_CORE_FW_SLOT_INVALID: Let firmware select slot based on package metadata
> + * @PDS_CORE_FW_SLOT_A:       Primary firmware slot A
> + * @PDS_CORE_FW_SLOT_B:       Primary firmware slot B
> + * @PDS_CORE_FW_SLOT_GOLD:    Gold/recovery firmware slot
> + * @PDS_CORE_FW_SLOT_MAX:     Sentinel value indicating no slot resolved
> + */
>  enum pds_core_fw_slot {
>  	PDS_CORE_FW_SLOT_INVALID    = 0,
>  	PDS_CORE_FW_SLOT_A	    = 1,
>  	PDS_CORE_FW_SLOT_B          = 2,
>  	PDS_CORE_FW_SLOT_GOLD       = 3,
> +	PDS_CORE_FW_SLOT_MAX        = 0xff,
>  };

[Low]
PDS_CORE_FW_SLOT_MAX is added with value 0xff to this wire-protocol
enum but has no references anywhere. Should it be wired up to something
or dropped before the enum value is locked in by firmware?
-- 
This is an AI-generated review.