| 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
| ||
|
Message-ID: <20250212122215.000001a0@huawei.com>
Date: Wed, 12 Feb 2025 12:22:15 +0000
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>,
<gospo@...adcom.com>, <hch@...radead.org>, <itayavr@...dia.com>,
<jiri@...dia.com>, <kuba@...nel.org>, <lbloch@...dia.com>,
<leonro@...dia.com>, <saeedm@...dia.com>, <linux-cxl@...r.kernel.org>,
<linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>,
<brett.creeley@....com>
Subject: Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework
On Tue, 11 Feb 2025 15:48: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>
> ---
Hi Shannon,
A few comments inline
Jonathan
> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> new file mode 100644
> index 000000000000..24979fe0deea
> --- /dev/null
> +++ b/drivers/fwctl/pds/main.c
> @@ -0,0 +1,195 @@
> +
> +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 = {0};
> + 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\n");
> + return err;
> + }
> +
> + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
> + cmd.fwctl_ident.version = 0;
> + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
> + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
Could save intializing cmd above and do it here where all
the data is available.
cmd = (union pds_core_adminq_cmd) {
.fwctl_ident = {
.opcode = ...
etc. Up to you.
}
> +
> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> + if (err) {
> + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
> + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> + return err;
> + }
> +
> + pdsfc->ident = ident;
> + pdsfc->ident_pa = ident_pa;
I guess it will become clear in later patches, but I'm not immediately sure why
it makes sense to keep a copy of ident and the dma mappings live etc.
Does it change at runtime?
> +
> + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
> + ident->version, ident->max_req_sz, ident->max_resp_sz,
> + ident->max_req_sg_elems, ident->max_resp_sg_elems);
> +
> + return 0;
> +}
> +static int pdsfc_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct pdsfc_dev *pdsfc __free(pdsfc_dev);
Convention for these is to put the constructor and destructor definitions
on one line. I'm too lazy to find the email from Linus where he
specified this but Dan did add docs to cleanup.h.
https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/cleanup.h#L129
is referring to setting this to NULL, which is minimum that should be done
as future code changes might mean there is a failure path between
declaration and use. Anyhow, it argues in favor of inline as shown
below.
> + struct pds_auxiliary_dev *padev;
> + struct device *dev = &adev->dev;
> + int err = 0;
Set in all paths where it is used so no need to set it here.
> +
> + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
struct pdsfc_dev *pdsfc __free(pdsfc_dev) =
fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> + struct pdsfc_dev, fwctl);
> + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> + struct pdsfc_dev, fwctl);
> + if (!pdsfc) {
> + dev_err(dev, "Failed to allocate fwctl device struct\n");
> + return -ENOMEM;
> + }
> + pdsfc->padev = padev;
> +
> + err = pdsfc_identify(pdsfc);
> + if (err) {
> + dev_err(dev, "Failed to identify device, err %d\n", err);
> + return err;
Perhaps use return dev_err_probe() just to get the pretty printing.
Note though that it won't print for enomem cases.
> + }
> +
> + err = fwctl_register(&pdsfc->fwctl);
> + if (err) {
> + dev_err(dev, "Failed to register device, err %d\n", err);
> + return err;
> + }
> +
> + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
> +
> + return 0;
> +
> +free_ident:
Nothing goes here. Which is good as missing __free magic with gotos
is a recipe for pain.
> + pdsfc_free_ident(pdsfc);
> + return err;
> +}
> +
> +static void pdsfc_remove(struct auxiliary_device *adev)
> +{
> + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> +
> + fwctl_unregister(&pdsfc->fwctl);
> + pdsfc_free_ident(pdsfc);
> +}
> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index 4b4e9a98b37b..7fc353b63353 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h
> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
> u8 status;
> };
>
> +enum pds_fwctl_cmd_opcode {
> + PDS_FWCTL_CMD_IDENT = 70,
> +};
> +
> +/**
> + * 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;
None of these actually need to be packed given explicit padding to
natural alignment of all fields. Arguably it does no harm though
so up to you.
> +
> +/**
> + * 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
> + * @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;
Powered by blists - more mailing lists