Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

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

On 2/18/2025 11:28 AM, Leon Romanovsky wrote:
> 
> On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson wrote:
>> Add support for a new fwctl-based auxiliary_device for creating a
>> channel for fwctl support into the AMD/Pensando DSC.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
>>   drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
>>   drivers/net/ethernet/amd/pds_core/core.h   |  1 +
>>   drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
>>   include/linux/pds/pds_common.h             |  2 ++
>>   5 files changed, 21 insertions(+), 2 deletions(-)
> 
> <...>
> 
> My comment is only slightly related to the patch itself, but worth to
> write it anyway.
> 
>> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
>> index 5802e1deef24..b193adbe7cc3 100644
>> --- a/include/linux/pds/pds_common.h
>> +++ b/include/linux/pds/pds_common.h
>> @@ -29,6 +29,7 @@ enum pds_core_vif_types {
>>        PDS_DEV_TYPE_ETH        = 3,
>>        PDS_DEV_TYPE_RDMA       = 4,
>>        PDS_DEV_TYPE_LM         = 5,
>> +     PDS_DEV_TYPE_FWCTL      = 6,
> 
> This enum and defines below should be cleaned from unsupported types.
> I don't see any code for RDMA, LM and ETH.
> 
> Thanks

I've looked at those a few times over the life of this code, but I 
continue to leave them there because they are part of the firmware 
interface definition, whether we use them or not.

You're right, there is no ETH or RDMA type code, they exist as 
historical artifacts of the early interface design.

The LM type underlies the device used by the pds-vfio-pci driver and is 
a value that pds_core will see in the device identity information 
gathered from the firmware if there are VFIO VFs configured in the FW.

I'd rather not mess with this enum for this patchset, but I'll keep this 
in mind for future cleanup work.

Thanks,
sln

> 
>>
>>        /* new ones added before this line */
>>        PDS_DEV_TYPE_MAX        = 16   /* don't change - used in struct size */
>> @@ -40,6 +41,7 @@ enum pds_core_vif_types {
>>   #define PDS_DEV_TYPE_ETH_STR "Eth"
>>   #define PDS_DEV_TYPE_RDMA_STR        "RDMA"
>>   #define PDS_DEV_TYPE_LM_STR  "LM"
>> +#define PDS_DEV_TYPE_FWCTL_STR       "fwctl"
>>
>>   #define PDS_VDPA_DEV_NAME    PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR
>>   #define PDS_VFIO_LM_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_LM_STR "." PDS_DEV_TYPE_VFIO_STR
>> --
>> 2.17.1
>>
>>