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: <20250304193954.GT133783@nvidia.com>
Date: Tue, 4 Mar 2025 15:39:54 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Shannon Nelson <shannon.nelson@....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 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

> +/**
> + * 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.

> +/* 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.

Jason