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

["Nelson, Shannon" <shannon.nelson@xxxxxxx>]

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