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

["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 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.
Will fix in v7 by adding a config space read after pci_disable_device() 
in pdsc_teardown() before freeing deferred DMA buffers. PCI transaction 
ordering rules guarantee that a read completion cannot return until all 
prior device-initiated memory writes have completed. This ensures 
in-flight DMA has drained before buffers are freed.

> 
> 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?
Yes. When devcmd returns -EIO or -ENXIO (or any non-timeout error), the 
command has completed, the MMIO read that retrieved the completion 
status acts as a DMA fence. The immediate free is safe because command 
completion implies DMA finished.

> 
>> +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?
Will fix in v7, using list_splice_init() to detach the list under the 
lock, then iterate and free without holding the lock.

> [ ... ]
> 
>> 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?
Will update documentation in v7. The component for fw.gold is "fw". Gold 
firmware can be updated by specifying component "fw" with a goldfw 
package file.

> 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?
> 
Will fix in v7, using the PDSC_FW_COMPONENT_FULL_NAME_BUFLEN macro in 
pdsc_flash_component() for the component_name_buf array size.

> [ ... ]
> 
>> +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?
> 
The dev_warn_once message tells the user to update their firmware. 
Per-component flash will fail with -ENOENT, but the user needs to 
upgrade firmware anyway to use this feature. Returning an explicit error 
here would prevent the basic flash operation from working on old firmware.

> [ ... ]
> 
>> +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?
> 
Firmware ignores data_pa/data_len/offset on STATUS operation. Only the 
operation field is meaningful for polling completion status.

> [ ... ]
> 
>> +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.
Will fix in v7, simplified to just if (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?
Will fix in v7 by adding NL_SET_ERR_MSG_MOD calls to PLDM error paths.

> 
>> +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?
The legacy path should reject component parameters, -net fix [1]

> 
>> 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?
Firmware uses this value internally as a sentinel. The driver doesn't 
branch on it (relies on response_code), but defining it prevents future 
enum additions from colliding with 0xff.

Thanks,
Nikhil


[1] 
https://lore.kernel.org/netdev/20260708163649.128620-1-nikhil.rao@amd.com