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

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

On 2/19/2025 12:24 AM, Leon Romanovsky wrote:
> 
> On Tue, Feb 18, 2025 at 12:00:52PM -0800, Nelson, Shannon wrote:
>> 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.
> 
> How? You are passing some number which FW is not aware of it. You can
> pass any number you want there. Even it is not true, you can
> PDS_DEV_TYPE_FWCTL = 6 here, but remove rest of enums and *_STR defines.

When pds_core starts up it gets ident/config information from the 
firmware in a struct pds_core_dev_identity, which includes the 
vif_types[] array which tells us how many of each PDS_DEV_TYPE_xx are 
supported in the FW.  This is indexed by enum pds_core_vif_types.

> 
>>
>> You're right, there is no ETH or RDMA type code, they exist as historical
>> artifacts of the early interface design.
> 
> This make me wonder why netdev merged this code which has nothing to do
> with netdev subsystem at all.

The pds_core was originally brought in for supporting pds_vdpa and 
pds_vfio_pci.  At the time, it was essentially following the example of 
the mlx core module; at the time there wasn't any push back or 
suggestions of a different place to land.  Maybe further fwctl and 
"core" related discussions will suggest another approach.

sln

> 
> Thanks