On 2/12/2025 4:22 AM, Jonathan Cameron wrote:
On Tue, 11 Feb 2025 15:48:52 -0800
Shannon Nelson <shannon.nelson@xxxxxxx> 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@xxxxxxx>
---
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;
