| 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: <3d9b8966-8ad6-43ee-a745-073a5c82554b@amd.com>
Date: Thu, 13 Feb 2025 15:06:14 -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 3/5] pds_fwctl: initial driver framework
On 2/12/2025 4:22 AM, Jonathan Cameron wrote:
> 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.
>
>
> }
Yeah, there are a lot of ways to do this. In a lot of the ionic code we
do a bunch of the init in the declaration and add to the more dynamic
field values later in the function. Let me think on this a little...
>> +
>> + 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?
No, actually this doesn't change at runtime, we could copy it into a
local storage and drop the DMA mapping, which will relieve us of the
need to remember to clean it up later.
>
>
>> +
>> + 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.
Obviously we're still new to this pattern - I'll look at fixing these
things up.
>
>
>> + 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.
Got it.
>
>> +
>> + 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.
Sure
>
>> + }
>> +
>> + 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.
The above ident change will remove the need for this.
>
>> + 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.
Old belt-and-suspenders habits...
Since this is the common style on the rest of the interface definitions,
I'd prefer to keep it for now.
sln
>
>> +
>> +/**
>> + * 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