netdev - Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d67da285-14a2-46d4-97d1-29aad9a53e60@amd.com>
Date: Wed, 5 Mar 2025 18:05:02 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: andrew.gospodarek@...adcom.com, aron.silverton@...cle.com,
 dan.j.williams@...el.com, daniel.vetter@...ll.ch, dave.jiang@...el.com,
 dsahern@...nel.org, gregkh@...uxfoundation.org, hch@...radead.org,
 itayavr@...dia.com, jiri@...dia.com, Jonathan.Cameron@...wei.com,
 kuba@...nel.org, lbloch@...dia.com, leonro@...dia.com,
 linux-cxl@...r.kernel.org, linux-rdma@...r.kernel.org,
 netdev@...r.kernel.org, saeedm@...dia.com, brett.creeley@....com
Subject: Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

On 3/4/2025 11:39 AM, Jason Gunthorpe wrote:
> On Fri, Feb 28, 2025 at 05:35:52PM -0800, Shannon Nelson wrote:
>> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd;
>> +     struct pds_fwctl_ident *ident;
>> +     dma_addr_t ident_pa;
>> +     int err = 0;
>> +
>> +     ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev->parent, ident_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map ident buffer\n");
>> +             return err;
>> +     }
>> +
>> +     cmd = (union pds_core_adminq_cmd) {
>> +             .fwctl_ident = {
>> +                     .opcode = PDS_FWCTL_CMD_IDENT,
>> +                     .version = 0,
>> +                     .len = cpu_to_le32(sizeof(*ident)),
>> +                     .ident_pa = cpu_to_le64(ident_pa),
>> +             }
>> +     };
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err)
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u err: %d\n",
>> +                     cmd.fwctl_ident.opcode, err);
>> +     else
>> +             pdsfc->ident = *ident;
>> +
>> +     dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
>> +
>> +     return 0;
> 
> Is it intential to loose the pds_client_adminq_cmd err? Maybe needs a
> comment if so

Good catch - thanks, will fix.

> 
>> +/**
>> + * struct pds_fwctl_cmd - Firmware control command structure
>> + * @opcode: Opcode
>> + * @rsvd:   Word boundary padding
>> + * @ep:     Endpoint identifier.
>> + * @op:     Operation identifier.
>> + */
>> +struct pds_fwctl_cmd {
>> +     u8     opcode;
>> +     u8     rsvd[3];
>> +     __le32 ep;
>> +     __le32 op;
>> +} __packed;
> 
> What's your plan for the scope indication? Right now this would be
> restricted to the most restricted FWCTL_RPC_CONFIGURATION scope in FW.

As you noticed in the next patch, the scope restrictions will come from 
the FW when we request endpoint and operation information.

> 
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/* Copyright(c) Advanced Micro Devices, Inc */
>> +
>> +/*
>> + * fwctl interface info for pds_fwctl
>> + */
>> +
>> +#ifndef _UAPI_FWCTL_PDS_H_
>> +#define _UAPI_FWCTL_PDS_H_
>> +
>> +#include <linux/types.h>
>> +
>> +/*
>> + * struct fwctl_info_pds
>> + *
>> + * Return basic information about the FW interface available.
>> + */
>> +struct fwctl_info_pds {
>> +     __u32 uid;
> 
> I think Jonathon remarked, the uid should go since it isn't used.

yep

Thanks,
sln

> 
> Jason