Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

["Nelson, Shannon" <shannon.nelson@amd.com>]

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