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: <20250304163043.00006b34@huawei.com>
Date: Tue, 4 Mar 2025 16:30:43 +0800
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Shannon Nelson <shannon.nelson@....com>
CC: <jgg@...dia.com>, <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>, <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, 28 Feb 2025 17:35:52 -0800
Shannon Nelson <shannon.nelson@....com> wrote:

> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
> devices.  This sets up a simple auxiliary_bus driver that registers
> with fwctl subsystem.  It expects that a pds_core device has set up
> the auxiliary_device pds_core.fwctl
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
A few minor comments inline,

> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> new file mode 100644
> index 000000000000..afc7dc283ad5
> --- /dev/null
> +++ b/drivers/fwctl/pds/main.c
> @@ -0,0 +1,169 @@
> +struct pdsfc_dev {
> +	struct fwctl_device fwctl;
> +	struct pds_auxiliary_dev *padev;
> +	struct pdsc *pdsc;

Not used in this patch that I can see.  I was curious why it is
here as I would expect that to match with the padev parent.

> +	u32 caps;
> +	struct pds_fwctl_ident ident;
> +};
> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));


> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
> +{
> +	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
> +	struct fwctl_info_pds *info;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info->uctx_caps = pdsfc_uctx->uctx_caps;

What about the UID? If it is always zero, what is it for?

> +
> +	return info;
> +}

> +/**
> + * struct pds_fwctl_comp - Firmware control completion structure
> + * @status:     Status of the firmware control operation
> + * @rsvd:       Word boundary padding
> + * @comp_index: Completion index in little-endian format
> + * @rsvd2:      Word boundary padding

11 bytes is some extreme word padding.  I'd just call it reserved
space.

> + * @color:      Color bit indicating the state of the completion
> + */
> +struct pds_fwctl_comp {
> +	u8     status;
> +	u8     rsvd;
> +	__le16 comp_index;
> +	u8     rsvd2[11];
> +	u8     color;
> +} __packed;

...

> +/**
> + * struct pds_fwctl_ident - Firmware control identification structure
> + * @features:    Supported features

Nice to have some defines or similar that give meaning to these
features, but maybe that comes in later patches.

> + * @version:     Interface version
> + * @rsvd:        Word boundary padding

word size seems to be varying between structures. I'd just document
it as "reserved"

> + * @max_req_sz:  Maximum request size
> + * @max_resp_sz: Maximum response size
> + * @max_req_sg_elems:  Maximum number of request SGs
> + * @max_resp_sg_elems: Maximum number of response SGs
> + */
> +struct pds_fwctl_ident {
> +	__le64 features;
> +	u8     version;
> +	u8     rsvd[3];
> +	__le32 max_req_sz;
> +	__le32 max_resp_sz;
> +	u8     max_req_sg_elems;
> +	u8     max_resp_sg_elems;
> +} __packed;

Jonathan