Re: [PATCH V1 4/5] accel/amd_vpci: Add Remote Management (RM) queue service APIs

[Yidong Zhang <yidong.zhang@amd.com>]

On 11/11/25 01:51, Christophe JAILLET wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
> 
> 
> Le 11/11/2025 à 02:15, David Zhang a écrit :
>> This patch introduces a set of APIs for allowing the PCIe driver submit
>> commands, transfer binary payloads and retrieve firmware metadata.
>>
>> Key features:
>> - RM queue command APIs:
>>    - create and destroy RM queue commands
>>    - Initialized command data payloads
>>    - Send and poll for command completion
>> - Service-level operations:
>>    - Retrieve firmware ID
>>    - Program accelerator and APU firmware images
>>    - Periodic health monitoring
>>
>> Co-developed-by: Nishad Saraf <nishads@amd.com>
>> Signed-off-by: Nishad Saraf <nishads@amd.com>
>> Signed-off-by: David Zhang <yidong.zhang@amd.com>
> 
> ...
> 
>> +static void rm_check_health(struct work_struct *w)
>> +{
>> +     struct rm_device *rdev = to_rdev_health_monitor(w);
>> +     u32 max_len = PAGE_SIZE;
>> +     struct rm_cmd *cmd;
>> +     int ret;
>> +
>> +     ret = rm_queue_create_cmd(rdev, RM_QUEUE_OP_GET_LOG_PAGE, &cmd);
>> +     if (ret)
>> +             return;
>> +
>> +     ret = rm_queue_payload_init(cmd, RM_CMD_LOG_PAGE_AXI_TRIP_STATUS);
>> +     if (ret)
>> +             goto destroy_cmd;
>> +
>> +     ret = rm_queue_send_cmd(cmd, RM_CMD_WAIT_CONFIG_TIMEOUT);
>> +     if (ret == -ETIME || ret == -EINVAL)
>> +             goto payload_fini;
>> +
>> +     if (ret) {
>> +             u32 log_len = cmd->cq_msg.data.page.len;
>> +
>> +             if (log_len > max_len) {
>> +                     vdev_warn(rdev->vdev, "msg size %d is greater 
>> than requested %d",
>> +                               log_len, max_len);
>> +                     log_len = max_len;
>> +             }
>> +
>> +             if (log_len) {
>> +                     char *buffer = vzalloc(log_len);
>> +
>> +                     if (!buffer)
>> +                             goto payload_fini;
>> +
>> +                     ret = rm_queue_copy_response(cmd, buffer, log_len);
>> +                     if (ret) {
>> +                             vfree(buffer);
>> +                             goto payload_fini;
>> +                     }
>> +
>> +                     vdev_err(rdev->vdev, "%s", buffer);
> 
> This looks like the normal path. is vdev_err() expected here?

This is the detail info when log_len is not 0, thus err log is expected.

> 
>> +                     vfree(buffer);
>> +
>> +             } else {
>> +                     vdev_err(rdev->vdev, "firewall check ret%d", ret);
>> +             }
>> +
>> +             rdev->firewall_tripped = 1;
>> +     }
>> +
>> +payload_fini:
>> +     rm_queue_payload_fini(cmd);
>> +destroy_cmd:
>> +     rm_queue_destroy_cmd(cmd);
>> +
>> +     vdev_dbg(rdev->vdev, "check result: %d", ret);
>> +}
> 
> ...
> 
>> +struct rm_device *versal_pci_rm_init(struct versal_pci_device *vdev)
>> +{
>> +     struct rm_header *header;
>> +     struct rm_device *rdev;
>> +     u32 status;
>> +     int ret;
>> +
>> +     rdev = devm_kzalloc(&vdev->pdev->dev, sizeof(*rdev), GFP_KERNEL);
>> +     if (!rdev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     rdev->vdev = vdev;
>> +     header = &rdev->rm_metadata;
>> +
>> +     rm_shmem_bulk_read(rdev, RM_HDR_OFF, (u32 *)header, 
>> sizeof(*header));
>> +     if (header->magic != RM_HDR_MAGIC_NUM) {
>> +             vdev_err(vdev, "Invalid RM header 0x%x", header->magic);
>> +             ret = -ENODEV;
>> +             goto err;
>> +     }
>> +
>> +     status = rm_shmem_read(rdev, header->status_off);
>> +     if (!status) {
>> +             vdev_err(vdev, "RM status %d is not ready", status);
> 
> This can be simplified, status is knwon to be 0.

Yes, I will fix this.

> 
>> +             ret = -ENODEV;
>> +             goto err;
>> +     }
> 
> ...
> 
> CJ