Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

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

On 3/4/2025 12:30 AM, Jonathan Cameron wrote:
> On Fri, 28 Feb 2025 17:35: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>
> A few minor comments inline,
> 
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> new file mode 100644
>> index 000000000000..afc7dc283ad5
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -0,0 +1,169 @@
>> +struct pdsfc_dev {
>> +     struct fwctl_device fwctl;
>> +     struct pds_auxiliary_dev *padev;
>> +     struct pdsc *pdsc;
> 
> Not used in this patch that I can see.  I was curious why it is
> here as I would expect that to match with the padev parent.

I'll take it out.

> 
>> +     u32 caps;
>> +     struct pds_fwctl_ident ident;
>> +};
>> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
> 
> 
>> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
>> +{
>> +     struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
>> +     struct fwctl_info_pds *info;
>> +
>> +     info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +     if (!info)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     info->uctx_caps = pdsfc_uctx->uctx_caps;
> 
> What about the UID? If it is always zero, what is it for?

Early code, thought we'd use it.  I'll take it out.

> 
>> +
>> +     return info;
>> +}
> 
>> +/**
>> + * 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
> 
> 11 bytes is some extreme word padding.  I'd just call it reserved
> space.

This is the same wording we've used in the other interface files, so at 
least we're consistently odd.

I'll tweak these.

> 
>> + * @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;
> 
> ...
> 
>> +/**
>> + * struct pds_fwctl_ident - Firmware control identification structure
>> + * @features:    Supported features
> 
> Nice to have some defines or similar that give meaning to these
> features, but maybe that comes in later patches.

We don't have any defined yet, but we have found it is useful to define 
this field for future growth without breaking APIs.  I'll make that more 
clear.

> 
>> + * @version:     Interface version
>> + * @rsvd:        Word boundary padding
> 
> word size seems to be varying between structures. I'd just document
> it as "reserved"

Sure

Thanks,
sln

> 
>> + * @max_req_sz:  Maximum request size
>> + * @max_resp_sz: Maximum response size
>> + * @max_req_sg_elems:  Maximum number of request SGs
>> + * @max_resp_sg_elems: Maximum number of response SGs
>> + */
>> +struct pds_fwctl_ident {
>> +     __le64 features;
>> +     u8     version;
>> +     u8     rsvd[3];
>> +     __le32 max_req_sz;
>> +     __le32 max_resp_sz;
>> +     u8     max_req_sg_elems;
>> +     u8     max_resp_sg_elems;
>> +} __packed;
> 
> Jonathan