| 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: <d0523e2f-dadb-46d3-88eb-2e9ea6682845@amd.com>
Date: Thu, 13 Feb 2025 15:13:43 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.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 4/5] pds_fwctl: add rpc and query support
On 2/12/2025 4:47 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:53 -0800
> Shannon Nelson <shannon.nelson@....com> wrote:
>
>> From: Brett Creeley <brett.creeley@....com>
>>
>> The pds_fwctl driver doesn't know what RPC operations are available
>> in the firmware, so also doesn't know what scope they might have. The
>> userland utility supplies the firmware "endpoint" and "operation" id values
>> and this driver queries the firmware for endpoints and their available
>> operations. The operation descriptions include the scope information
>> which the driver uses for scope testing.
>>
>> Signed-off-by: Brett Creeley <brett.creeley@....com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> Various comments inline. I haven't looked closely at the actual interface
> yet though as running out of time today.
>
> Jonathan
>
>> ---
>> drivers/fwctl/pds/main.c | 369 ++++++++++++++++++++++++++++++++-
>> include/linux/pds/pds_adminq.h | 187 +++++++++++++++++
>> include/uapi/fwctl/pds.h | 16 ++
>> 3 files changed, 569 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> index 24979fe0deea..b60a66ef1fac 100644
>> --- a/drivers/fwctl/pds/main.c
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -15,12 +15,22 @@
>> #include <linux/pds/pds_adminq.h>
>> #include <linux/pds/pds_auxbus.h>
>>
>> +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
>> +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
>
> I'm lost. These look same as the ones in slab.h for kfree and kvfree
> which already handle error pointers. Maybe based on an old kernel?
>
>> +static struct pds_fwctl_query_data *pdsfc_get_operations(struct pdsfc_dev *pdsfc,
>> + dma_addr_t *pa, u32 ep)
>> +{
>> + struct pds_fwctl_query_data_operation *entries = NULL;
>
> Always set before use so don't initialize here.
Sure.
>
>> + struct device *dev = &pdsfc->fwctl.dev;
>> + union pds_core_adminq_comp comp = {0};
>> + union pds_core_adminq_cmd cmd = {0};
>> + struct pds_fwctl_query_data *data;
>> + dma_addr_t data_pa;
>> + int err;
>> + int i;
>> +
>> + /* Query the operations list for the given endpoint */
>> + data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
>> + err = dma_mapping_error(dev->parent, data_pa);
>> + if (err) {
>> + dev_err(dev, "Failed to map operations list\n");
>> + return ERR_PTR(err);
>> + }
>> +
>> + cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
>> + cmd.fwctl_query.entity = PDS_FWCTL_RPC_ENDPOINT;
>> + cmd.fwctl_query.version = 0;
>> + cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
>> + cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
>> + cmd.fwctl_query.ep = cpu_to_le32(ep);
>> +
>> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> + if (err) {
>> + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> + dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
>> + return ERR_PTR(err);
>> + }
>> +
>> + *pa = data_pa;
>> +
>> + entries = (struct pds_fwctl_query_data_operation *)data->entries;
>> + dev_dbg(dev, "num_entries %d\n", data->num_entries);
>> + for (i = 0; i < data->num_entries; i++)
>> + dev_dbg(dev, "endpoint %d operation: id %x scope %d\n",
>> + ep, entries[i].id, entries[i].scope);
>> +
>> + return data;
>> +}
>
>> +
>> static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>> void *in, size_t in_len, size_t *out_len)
>> {
>> - return NULL;
>> + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
>> + struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
> In is a void * so never any need to cast it to another pointer type.
>
>> + void *out_payload __free(kfree_errptr) = NULL;
>
> Similar comment on style for these following documentation in cleanup.h
> That is tricky in this case but you can at least declare them and set
> them NULL just before they are conditionally assigned.
I'll look at these.
>
>> + void *in_payload __free(kfree_errptr) = NULL;
>> + struct device *dev = &uctx->fwctl->dev;
>> + union pds_core_adminq_comp comp = {0};
>> + dma_addr_t out_payload_dma_addr = 0;
>> + union pds_core_adminq_cmd cmd = {0};
>> + dma_addr_t in_payload_dma_addr = 0;
>> + void *out = NULL;
>> + int err;
>> +
>> + err = pdsfc_validate_rpc(pdsfc, rpc, scope);
>> + if (err) {
>> + dev_err(dev, "Invalid RPC request\n");
>> + return ERR_PTR(err);
>> + }
>> +
>> + if (rpc->in.len > 0) {
>> + in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
>> + if (!in_payload) {
>> + dev_err(dev, "Failed to allocate in_payload\n");
>> + out = ERR_PTR(-ENOMEM);
>> + goto done;
>
> As before avoid the gotos mixed with free.
> Easiest might be a little helper function for this setup of
> the input buffer and one for the output buffer.
> Probably not combined with __free that isn't giving much advantage
> here anyway.
>
> For this particular one can just return the error anyway as
> nothing to do.
Thanks
>
>> + }
>> +
>> + if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
>> + rpc->in.len)) {
>> + dev_err(dev, "Failed to copy in_payload from user\n");
>> + out = ERR_PTR(-EFAULT);
>> + goto done;
>> + }
>> +
>> + in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
>> + rpc->in.len, DMA_TO_DEVICE);
>> + err = dma_mapping_error(dev->parent, in_payload_dma_addr);
>> + if (err) {
>> + dev_err(dev, "Failed to map in_payload\n");
>> + out = ERR_PTR(err);
>> + goto done;
>> + }
>> + }
>> +
>> + if (rpc->out.len > 0) {
>> + out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
>> + if (!out_payload) {
>> + dev_err(dev, "Failed to allocate out_payload\n");
>> + out = ERR_PTR(-ENOMEM);
>> + goto done;
>> + }
>> +
>> + out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
>> + rpc->out.len, DMA_FROM_DEVICE);
>> + err = dma_mapping_error(dev->parent, out_payload_dma_addr);
>> + if (err) {
>> + dev_err(dev, "Failed to map out_payload\n");
>> + out = ERR_PTR(err);
>> + goto done;
>> + }
>> + }
>> +
>> + cmd.fwctl_rpc.opcode = PDS_FWCTL_CMD_RPC;
>> + cmd.fwctl_rpc.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP;
>> + cmd.fwctl_rpc.ep = cpu_to_le32(rpc->in.ep);
>> + cmd.fwctl_rpc.op = cpu_to_le32(rpc->in.op);
>> + cmd.fwctl_rpc.req_pa = cpu_to_le64(in_payload_dma_addr);
>> + cmd.fwctl_rpc.req_sz = cpu_to_le32(rpc->in.len);
>> + cmd.fwctl_rpc.resp_pa = cpu_to_le64(out_payload_dma_addr);
>> + cmd.fwctl_rpc.resp_sz = cpu_to_le32(rpc->out.len);
>> +
>> + dev_dbg(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d\n",
>> + __func__, rpc->in.ep, rpc->in.op,
>> + cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>> + cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems);
>> +
>> + dynamic_hex_dump("in ", DUMP_PREFIX_OFFSET, 16, 1, in_payload, rpc->in.len, true);
>> +
>> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> + if (err) {
>> + dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
>> + __func__, rpc->in.ep, rpc->in.op,
>> + cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>> + cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
>> + err);
>> + out = ERR_PTR(err);
>> + goto done;
>> + }
>> +
>> + dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
>> +
>> + dev_dbg(dev, "%s: status %d comp_index %d err %d resp_sz %d color %d\n",
>> + __func__, comp.fwctl_rpc.status, comp.fwctl_rpc.comp_index,
>> + comp.fwctl_rpc.err, comp.fwctl_rpc.resp_sz,
>> + comp.fwctl_rpc.color);
>> +
>> + if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
>> + dev_err(dev, "Failed to copy out_payload to user\n");
>> + out = ERR_PTR(-EFAULT);
>> + goto done;
>> + }
>> +
>> + rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
>> + *out_len = in_len;
>> + out = in;
>> +
>> +done:
>> + if (in_payload_dma_addr)
>> + dma_unmap_single(dev->parent, in_payload_dma_addr,
>> + rpc->in.len, DMA_TO_DEVICE);
>> +
>> + if (out_payload_dma_addr)
>> + dma_unmap_single(dev->parent, out_payload_dma_addr,
>> + rpc->out.len, DMA_FROM_DEVICE);
>> +
>> + return out;
>> }
>>
>> static const struct fwctl_ops pdsfc_ops = {
>> @@ -150,16 +504,23 @@ static int pdsfc_probe(struct auxiliary_device *adev,
>> return err;
>> }
>>
>> + err = pdsfc_init_endpoints(pdsfc);
>> + if (err) {
>> + dev_err(dev, "Failed to init endpoints, err %d\n", err);
>> + goto free_ident;
>> + }
>> +
>> err = fwctl_register(&pdsfc->fwctl);
>> if (err) {
>> dev_err(dev, "Failed to register device, err %d\n", err);
>> - return err;
>> + goto free_endpoints;
>
> Mixing the __free() magic and gotos is 'probably' ok in this case
> but high risk.
>
> https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/cleanup.h#L135
> Makes a fairly strong statement on this. I'd suggest either figuring
> out a code reorg that avoids need for gotos or stopping using __free in this
> function. This looks like similar question to earlier one of
> why are these cached as opposed to done inside open/close callbacks
> for specific RPC calls?
Thanks.
>
>> }
>> -
>
> Noise that shouldn't be here.
I'll clean the earlier patch
>
>> auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>>
>> return 0;
>>
>> +free_endpoints:
>> + pdsfc_free_endpoints(pdsfc);
>> free_ident:
>> pdsfc_free_ident(pdsfc);
>> return err;
>> @@ -170,6 +531,8 @@ 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_operations(pdsfc);
>> + pdsfc_free_endpoints(pdsfc);
>> pdsfc_free_ident(pdsfc);
>> }
>
>
>
>> +/**
>> + * struct pds_fwctl_query_data - query data structure
>> + * @version: Version of the query data structure
>> + * @rsvd: Word boundary padding
>> + * @num_entries: Number of entries in the union
>> + * @entries: Array of query data entries, depending on the entity type.
>> + */
>> +struct pds_fwctl_query_data {
>> + u8 version;
>> + u8 rsvd[3];
>> + __le32 num_entries;
>> + uint8_t entries[];
> __counted_by_le(num_entries)
> probably appropriate here.
I like the counted_by() stuff, but because this interface file is used
with FW built in a different environment, I've been hesitant to add it
in to these obvious places. I'll see if I can add a little macro magic
to that environment to allow the added syntax sugar.
>> +} __packed;
>> +
>
>> +/**
>> + * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
>> + * @addr: DMA address of SG element data buffer
>> + * @len: Length of SG element data buffer, in bytes
>> + * @rsvd: Word boundary padding
>> + */
>> +struct pds_sg_elem {
>> + __le64 addr;
>> + __le32 len;
>> + __le16 rsvd[2];
>
> Why not an __le32?
> It's reserved and naturally aligned so who cares on type ;)
I think this is a leftover from earlier implementation changes, but
doesn't make too much difference.
sln
>
>> +} __packed;
>
Powered by blists - more mailing lists